fix kitty image protocol display parameters

term/src/terminalstate/image.rs

@@ -22,17 +22,19 @@ pub struct ImageAttachParams {
     pub image_height: u32,
 
     /// Dimensions of the area of the image to be displayed, in pixels
-    pub source_width: u32,
+    pub source_width: Option<u32>,
-    pub source_height: u32,
+    pub source_height: Option<u32>,
 
     /// Origin of the source data region, top left corner in pixels
     pub source_origin_x: u32,
     pub source_origin_y: u32,
 
     /// When rendering in the cell, use this offset from the top left
-    /// of the cell
+    /// of the cell. This is only used in the Kitty image protocol.
-    pub padding_left: u16,
+    /// This should be smaller than the size of the cell. Larger values will
-    pub padding_top: u16,
+    /// be truncated.
+    pub cell_padding_left: u16,
+    pub cell_padding_top: u16,
 
     /// Plane on which to display the image
     pub z_index: i32,
@@ -68,45 +70,59 @@ impl TerminalState {
         let physical_rows = self.screen().physical_rows;
         let cell_pixel_width = self.pixel_width / physical_cols;
         let cell_pixel_height = self.pixel_height / physical_rows;
-
+        let cell_padding_left = params
-        let avail_width = params.image_width.saturating_sub(params.source_origin_x);
+            .cell_padding_left
-        let avail_height = params.image_height.saturating_sub(params.source_origin_y);
+            .min(cell_pixel_width.saturating_sub(1) as u16);
-        let source_width = params.source_width.min(params.image_width).min(avail_width);
+        let cell_padding_top = params
-        let source_height = params
+            .cell_padding_top
+            .min(cell_pixel_height.saturating_sub(1) as u16);
+        //NOTE: review conflicting origin vs drawing going over image
+        let image_max_width = params.image_width.saturating_sub(params.source_origin_x);
+        let image_max_height = params.image_height.saturating_sub(params.source_origin_y);
+        let draw_width = params
+            .source_width
+            .unwrap_or(image_max_width)
+            .min(image_max_width);
+        let draw_height = params
             .source_height
-            .min(params.image_height)
+            .unwrap_or(image_max_height)
-            .min(avail_height);
+            .min(image_max_height);
 
-        let aspect = source_width as f32 / source_height as f32;
+        let (fullcells_width, remainder_width_cell, x_delta_divisor) = params
-
-        let width_in_cells = params
             .columns
-            .unwrap_or_else(|| (source_width as f32 / cell_pixel_width as f32).ceil() as usize);
+            .map(|cols| {
-        let height_in_cells = params
+                (
+                    cols,
+                    0,
+                    (cols * cell_pixel_width) as u32 * params.image_width / draw_width,
+                )
+            })
+            .unwrap_or_else(|| {
+                (
+                    draw_width as usize / cell_pixel_width,
+                    draw_width as usize % cell_pixel_width,
+                    params.image_width,
+                )
+            });
+        let (fullcells_height, remainder_height_cell, y_delta_divisor) = params
             .rows
-            .unwrap_or_else(|| (source_height as f32 / cell_pixel_height as f32).ceil() as usize);
+            .map(|rows| {
-
+                (
-        // Figure out the desired pixel dimensions, respecting the original
+                    rows,
-        // aspect of the picture if they specific rows/columns as the max size.
+                    0,
-        let target_pixel_width = if params.columns.is_some() {
+                    (rows * cell_pixel_height) as u32 * params.image_height / draw_height,
-            if source_width > source_height {
+                )
-                width_in_cells * cell_pixel_width
+            })
-            } else {
+            .unwrap_or_else(|| {
-                ((height_in_cells * cell_pixel_height) as f32 * aspect).ceil() as usize
+                (
-            }
+                    draw_height as usize / cell_pixel_height,
-        } else {
+                    draw_height as usize % cell_pixel_height,
-            source_width as usize
+                    params.image_height,
-        };
+                )
-        let target_pixel_height = if params.rows.is_some() {
+            });
-            if source_height > source_width {
-                height_in_cells * cell_pixel_height
-            } else {
-                ((width_in_cells * cell_pixel_width) as f32 / aspect).ceil() as usize
-            }
-        } else {
-            source_height as usize
-        };
 
+        let target_pixel_width = fullcells_width * cell_pixel_width + remainder_width_cell;
+        let target_pixel_height = fullcells_height * cell_pixel_height + remainder_height_cell;
         let first_row = self.screen().visible_row_to_stable_row(self.cursor.y);
 
         let mut ypos = NotNan::new(params.source_origin_y as f32 / params.image_height as f32)
@@ -115,6 +131,15 @@ impl TerminalState {
             .context("computing xpos")?;
 
         let cursor_x = self.cursor.x;
+
+        let width_in_cells = fullcells_width + (remainder_width_cell > 0) as usize;
+        let height_in_cells = fullcells_height + (remainder_height_cell > 0) as usize;
+        let height_in_cells = if params.do_not_move_cursor {
+            height_in_cells.min(self.screen().physical_rows - self.cursor.y as usize)
+        } else {
+            height_in_cells
+        };
+
         log::debug!(
             "image is {}x{} cells (cell is {}x{}), target pixel dims {}x{}, {:?}, (term is {}x{}@{}x{})",
             width_in_cells,
@@ -130,16 +155,10 @@ impl TerminalState {
             self.pixel_height
         );
 
-        let height_in_cells = if params.do_not_move_cursor {
+        let mut remain_y = target_pixel_height;
-            height_in_cells.min(self.screen().physical_rows - self.cursor.y as usize)
-        } else {
-            height_in_cells
-        };
-
-        let mut remain_y = target_pixel_height as usize;
         for y in 0..height_in_cells {
             let padding_bottom = cell_pixel_height.saturating_sub(remain_y) as u16;
-            let y_delta = (remain_y.min(cell_pixel_height) as f32) / (target_pixel_height as f32);
+            let y_delta = (remain_y.min(cell_pixel_height) as f32) / y_delta_divisor as f32;
             remain_y = remain_y.saturating_sub(cell_pixel_height);
 
             let mut xpos = start_xpos;
@@ -152,22 +171,22 @@ impl TerminalState {
                 "setting cells for y={} x=[{}..{}]",
                 cursor_y,
                 cursor_x,
-                cursor_x + width_in_cells
+                cursor_x + fullcells_width
             );
-            let mut remain_x = target_pixel_width as usize;
+            let mut remain_x = target_pixel_width;
             for x in 0..width_in_cells {
                 let padding_right = cell_pixel_width.saturating_sub(remain_x) as u16;
-                let x_delta = (remain_x.min(cell_pixel_width) as f32) / (target_pixel_width as f32);
+                let x_delta = (remain_x.min(cell_pixel_width) as f32) / x_delta_divisor as f32;
+                remain_x = remain_x.saturating_sub(cell_pixel_width);
                 log::debug!(
                     "x_delta {} ({} px), y_delta {} ({} px), padding_right={}, padding_bottom={}",
                     x_delta,
-                    x_delta * source_width as f32,
+                    x_delta * x_delta_divisor as f32,
                     y_delta,
-                    y_delta * source_width as f32,
+                    y_delta * y_delta_divisor as f32,
                     padding_right,
                     padding_bottom
                 );
-                remain_x = remain_x.saturating_sub(cell_pixel_width);
                 let mut cell = self
                     .screen_mut()
                     .get_cell(cursor_x + x, cursor_y)
@@ -178,8 +197,8 @@ impl TerminalState {
                     TextureCoordinate::new(xpos + x_delta, ypos + y_delta),
                     params.data.clone(),
                     params.z_index,
-                    params.padding_left,
+                    cell_padding_left,
-                    params.padding_top,
+                    cell_padding_top,
                     padding_right,
                     padding_bottom,
                     params.image_id,
@@ -202,6 +221,11 @@ impl TerminalState {
             }
         }
 
+        // adjust cursor position if the drawn cells move beyond current cell
+        let x_padding_shift = (draw_width as usize + cell_padding_left as usize
+            > cell_pixel_width * width_in_cells) as i64;
+        let y_padding_shift = (draw_height as usize + cell_padding_top as usize
+            > cell_pixel_height * height_in_cells) as i64;
         if !params.do_not_move_cursor {
             // Sixel places the cursor under the left corner of the image,
             // unless sixel_scrolls_right is enabled.
@@ -213,8 +237,8 @@ impl TerminalState {
 
             if bottom_right {
                 self.set_cursor_pos(
-                    &Position::Relative(width_in_cells as i64),
+                    &Position::Relative(width_in_cells as i64 + x_padding_shift),
-                    &Position::Relative(0),
+                    &Position::Relative(y_padding_shift),
                 );
             }
         }

term/src/terminalstate/iterm.rs

@@ -132,12 +132,12 @@ impl TerminalState {
         if let Err(err) = self.assign_image_to_cells(ImageAttachParams {
             image_width: width as u32,
             image_height: height as u32,
-            source_width: width as u32,
+            source_width: None,
-            source_height: height as u32,
+            source_height: None,
             source_origin_x: 0,
             source_origin_y: 0,
-            padding_left: 0,
+            cell_padding_left: 0,
-            padding_top: 0,
+            cell_padding_top: 0,
             z_index: 0,
             columns: None,
             rows: None,

term/src/terminalstate/kitty.rs

@@ -35,7 +35,9 @@ impl KittyImageState {
     }
 
     fn record_id_to_data(&mut self, image_id: u32, data: Arc<ImageData>) {
+        if image_id != 0 {
             self.remove_data_for_id(image_id);
+        }
         self.prune_unreferenced();
         self.used_memory += data.len();
         self.id_to_data.insert(image_id, data);
@@ -98,7 +100,9 @@ impl TerminalState {
             placement,
             verbosity
         );
+        if image_id != 0 {
             self.kitty_remove_placement(image_id, placement.placement_id);
+        }
         let img = Arc::clone(self.kitty_img.id_to_data.get(&image_id).ok_or_else(|| {
             anyhow::anyhow!(
                 "no matching image id {} in id_to_data for image_number {:?}",
@@ -112,12 +116,12 @@ impl TerminalState {
         let info = self.assign_image_to_cells(ImageAttachParams {
             image_width,
             image_height,
-            source_width: placement.w.unwrap_or(image_width),
+            source_width: placement.w,
-            source_height: placement.h.unwrap_or(image_height),
+            source_height: placement.h,
             source_origin_x: placement.x.unwrap_or(0),
             source_origin_y: placement.y.unwrap_or(0),
-            padding_left: placement.x_offset.unwrap_or(0) as u16,
+            cell_padding_left: placement.x_offset.unwrap_or(0) as u16,
-            padding_top: placement.y_offset.unwrap_or(0) as u16,
+            cell_padding_top: placement.y_offset.unwrap_or(0) as u16,
             data: img,
             style: ImageAttachStyle::Kitty,
             z_index: placement.z_index.unwrap_or(0),

term/src/terminalstate/sixel.rs

@@ -132,14 +132,14 @@ impl TerminalState {
         if let Err(err) = self.assign_image_to_cells(ImageAttachParams {
             image_width: width,
             image_height: height,
-            source_width: width,
+            source_width: None,
-            source_height: height,
+            source_height: None,
             rows: None,
             columns: None,
             source_origin_x: 0,
             source_origin_y: 0,
-            padding_left: 0,
+            cell_padding_left: 0,
-            padding_top: 0,
+            cell_padding_top: 0,
             data: image_data,
             style: ImageAttachStyle::Sixel,
             z_index: 0,

test-data/kitty-png.py

@@ -24,5 +24,29 @@ def write_chunked(**cmd):
       sys.stdout.flush()
       cmd.clear()
 
+def just_print(img):
+    write_chunked(a='T', f=100, data=img)
+
+def test_x_y_w_h_c_r(img):
+    write_chunked(a='T', f=100, y=150, h=105, C=1, data=img)
+    write_chunked(a='T', f=100, y=200, w=1, data=img)
+    write_chunked(a='T', f=100, y=200, h=1, data=img)
+    write_chunked(a='T', f=100, x=300, y=100, h=10, w=10, data=img)
+    write_chunked(a='T', f=100, x=300, y=100, h=10, w=10, r=15, data=img)
+    write_chunked(a='T', f=100, x=300, y=100, h=10, w=10, c=1, data=img)
+    write_chunked(a='T', f=100, x=300, y=100, h=10, w=10, r=1, data=img)
+    write_chunked(a='T', f=100, x=300, y=100, h=10, w=10, r=15, c=20, data=img)
+
+
+def test_cell_offsets(img):
+    write_chunked(a='T', f=100, h=10, w=10, X=2, Y=2, data=img)
+    write_chunked(a='T', f=100, h=20, w=10, X=2, Y=2, data=img)
+    write_chunked(a='T', f=100, h=2, Y=20, data=img)
+    write_chunked(a='T', f=100, h=38, w=2, X=19, data=img)
+
+if __name__ == "__main__":
     with open(sys.argv[-1], 'rb') as f:
-   write_chunked(a='T', f=100, data=f.read())
+        img = f.read()
+    just_print(img)
+    # test_x_y_w_h_c_r(img)
+    # test_cell_offsets(img)