Fix kitty image leaving behind images after removing image placements #4995
Conversation
…tration) Go through all cells removing the specified image. This could have extremely bad performance for large scrollbacks and a lot of updating images. The PlacementInfo should be sufficient to just remove the image from the relevant lines, but right now this PlacementInfo can become stale after resizing, which seems to indicate a bug.
There was a problem hiding this comment.
Thanks for looking into this!
The rewrap/resize case is clearly something that I didn't think all the way through for this.
I think that we should add some logic to fixup PlacementInfo on resize/rewrap; while this PR makes the removal more correct it seems like the fundamental problem is that our data is stale, and that that will impact other areas.
I think what is needed is to amend the StableRowIndex and other fields in the placement in Screen::rewrap_lines and/or Screen::resize. There is some logic to compute an adjusted cursor position that you might be able to adapt, but there are a couple of open issues around the cursor position moving in surprising ways in some edge cases when sizing smaller and then larger again, so don't trust the existing logic blindly!
| @@ -101,7 +101,11 @@ impl TerminalState { | |||
| verbosity | |||
| ); | |||
| if image_id != 0 { | |||
| self.kitty_remove_placement(image_id, placement.placement_id); | |||
| if let Some(place_id) = placement.placement_id { | |||
There was a problem hiding this comment.
minor stylistic thing here, we can remove some nesting/indenting:
if image_id != 0 && placement.placement_id.unwrap_or(0) != 0 {
self.kitty_remove_placement(image_id, placement.placement_id);
}| let range = | ||
| screen.stable_range(&(info.first_row..info.first_row + info.rows as StableRowIndex)); | ||
| for idx in range { | ||
| for idx in 0..screen.scrollback_rows() { |
There was a problem hiding this comment.
ah, yeah, this is conceptually a bummer but I don't think this is necessarily world ending... unless it is really noticeable.
FWIW, you can replace:
for idx in 0..screen.scrollback_rows() {
let line = screen.line_mut(idx);
do_something(idx, line)
}with:
screen.for_each_phys_line_mut(|idx, line| do_something(idx, line));which will eliminate the bounds checks on each call to line_mut and make things a little more efficient.
Given that we probably don't want to make this change, if you look at the original logic, that could probably be updated to use screen.with_phys_lines(range), || ...) to also avoid those bounds checks.
[DRAFT] This PR is not intended to be merged right now.
This should fix #2422, but the fix in commit 319a537 will have serious performance issues for cases with big scrollbacks and lots of image updates. The "fix" is to ignore
PlacementInfo(that might be stale) and go through all the scrollback removing the image.The main issues seems to be that
StableRowIndexcan become stale whenever a window resizing changes the amount of columns enough so as to add more lines by wrapping other lines. After wrapping, the image will get displaced (this is in kitty's implementation does not happen), and so thePlacementInfothat is stored for thatimage_idandplacement_idis no longer valid.It seems to me that it would be a better approach to keep the original way of removing the images by their
PlacementInfo(instead of going through all the scrollback), and prevent the images from being moved by lines being wrapped. The problem I see here is that this handling of the cell properties of those lines will need to be done wherever the lines are being wrapped and I'm not sure you'll be too happy about adding logic about images there (for being so unrelated).Let me know if you have any ideas or thoughts about how we could address this.
Minimal Reproduction
Kitty(reference):
Wezterm(main):
Wezterm(this PR):
Another issue tangentially related to this is that images are not occluded when a new pane appears:
In this case there isn't a way to update displayed images when the geometry of the pane changes. A way to handle these geometry changes in general would also solve the resizing.