Fix kitty image leaving behind images after removing image placements

[jonboh]

[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 StableRowIndex can 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 the PlacementInfo that is stored for that image_id and placement_id is 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.

[wez]

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!

[wez]

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);
}

[wez]

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.