Package: emacs;
Reported by: JD Smith <jdtsmith <at> gmail.com>
Date: Sun, 3 Dec 2023 16:56:01 UTC
Severity: normal
Done: Eli Zaretskii <eliz <at> gnu.org>
Message #119 received at 67604 <at> debbugs.gnu.org (full text, mbox):
From: JD Smith <jdtsmith <at> gmail.com> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 67604 <at> debbugs.gnu.org Subject: Re: bug#67604: Motion problems with inline images Date: Wed, 4 Jun 2025 16:47:48 -0400
[Message part 1 (text/plain, inline)]
> On May 31, 2025, at 8:18 AM, Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> So you are saying that the patch you suggested is incorrect, and we
> should do something else, and in particular make sure the call to
> set_iterator_to_next _is_ made?
Yes, the former patch was too broad and thus somewhat incorrect,
but for different reasons (see below).
I've now very carefully traced through the entire flow for the line-skip
and non-line-skip (missing the bug by one pixel) cases.
Here is a summary:
1.1 line-skip on wrapped inline images that appear to "just fit" on the prior line
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Bug #67604. This bug results when an image or stretch /just fits/ as
the last glyph within the window, and `overflow-newline-into-fringe'
is enabled. This causes an "early skip" of the image to the position
just past it, which results in `move_it_to' incorrectly returning
early at that position, with `current_x=0'. This misleads
`move_it_by_lines', which ends up moving past the "start of the line
at `VPOS'", and in some situations can move right past the next line.
This results in a line being "skipped" by `vertical-motion'.
1.1.1 Final assessment
----------------------
It's pretty head-spinning the number of possible paths through this
maze. I'll try to summarize the situation as best I can, based on all
my investigations so far, here comparing the line-skip and
no-line-skip paths (which differ by one pixel in the red image).
What is happening:
1. `Fvertical_motion' starts by moving back to the beginning of the
(real) line at `pos=1'.
2. It then asks `move_it_to' to move back to `pos=89' (the start of
the green box) using `op=MOVE_TO_POS'.
3. `move_it_to' calls `move_it_in_display_line_to' with
`TO_CHARPOS=89', which does one of two things, depending on whether
the image "just fits" and the fine-tuned overflow branch is taken:
A) If the image does not "just fit" (even by one pixel, no line
skip bug):
1) Instead of moving past the image on the first call,
`move_it_in_display_line_to' moves `IT' to `pos=89', on the
first buffer position covered by the image, leaving
`current_x=451' (the `x' position at the /start/ of the
image).
2) `move_it_to' resets `current_x' to `line_start_x=0'
("Reset/increment for the next run"), and iterates.
3) On the next iteration, `move_it_to' is still on the image
with `method=GET_FROM_IMAGE'.
4) `move_it_in_display_line_to' is therefore called with
`pos=89' /again/.
5) `IT' is /now/ moved to `pos=125', but with the correct
`current_x=108'.
B) If the image "just fits" (line skip bug):
1) On the first call to `move_it_in_display_line_to', `IT' is
moved past `pos=89' to `pos=125', just /after/ the image,
leaving `current_x=it->last_visible_x=560'.
2) `move_it_to' resets `current_x' to `line_start_x=0'
("Reset/increment for the next run").
3) On the next iteration, `move_it_to' notices that `it' is
already past the `TO_CHARPOS' and is on a `FROM_BUFFER'
position (`pos=125'), and returns, via
`skip=MOVE_POS_MATCH_OR_ZV'.
4) `it' is now at `pos=125' (past the image), but claims
`current_x=0' (because `move_it_to' explicitly reset
`current_x' to that before returning). *THIS IS INCORRECT*.
4. Summary: depending on which path has been taken above,
`move_it_to(OP=MOVE_TO_POS)' returns with `it' either holding the
correct initial `x' position (`current_x=108'), or the incorrect
position (`it->current_x = 0').
5. Starting from this position, `Fvertical_motion' calls
`move_it_by_lines' with `DVPOS=1' to move one line down, as
requested.
6. This calls `move_it_to' with `op=MOVE_TO_VPOS'.
7. `move_it_to' calls `move_it_in_display_line_to(OP=0)' to "stop at
the start of the line `TO_VPOS'". Note that the latter mentions:
,----
| /* Regardless of OP's value, stop upon reaching the end of the display line. */
`----
8. This tests whether you are still on the line by using the
accumulated glyph widths. When the starting `x' position is wrong,
point moves TOO FAR, past the end of the line and even (in the
right circumstances) onto the /next/ line.
9. `move_it_to', and hence `vertical-motion', skips a line.
Take-aways from this:
- There is not apparently an absolute contract of all the `move_*'
functions to move past images. In the first call to
`move_it_in_display_line_to' in the bug-free path (starting from
`pos=1'), the call does /not/ move past the image. Only on the
subsequent call targeting the same position (and starting from that
position) does it move past the image.
- It's fairly clear what the core of the problem is: a small feature
designed for `overflow-newline-into-fringe'[1] results in an
incorrect `current_x' at the starting point of vertical motion when
word-wrapping, for images or stretches at the end of the line which
/just fit/ into the window, activating the overflow logic.
- Less clear is /where to intervene/ to fix the bug, without
disturbing anything else. Ideally the first call to
`move_it_in_display_line_to' would not move past, even when the
glyph just fits.
[1] If you turn off `overflow-newline-into-fringe', this bug
disappears.
1.1.2 Solution
--------------
The solution is to localize as precisely as possible to prevent the
"early return" of just-fitting glyphs from
`move_it_in_display_line_to', when:
1. The last method is `GET_FROM_IMAGE' or `GET_FROM_STRETCH', and
2. `line_wrap' is `WORD_WRAP'.
> If the patch passes these two tests, feel free to install on the
> master branch.
I do not have commit privileges; whom should I contact?
Final patch attached. All return values of move_it_in_display_line_to are now
preserved, except the unwanted "early return" referenced above.
This patch only affects the rare combination of:
- moving in a display line to a position at the start of an image or
stretch
- which just fits on a line
- while word-wrapping (so it will always be wrapped)
- and `overflow-newline-into-fringe=t'
[inline-image-line-skip-innermost.patch (application/octet-stream, attachment)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.