Subject: Re: bug#67604: Motion problems with inline images

From: JD Smith <[email-masked]>
Date: Wed, 4 Jun 2025 16:47:48 -0400

> On May 31, 2025, at 8:18 AM, Eli Zaretskii <[email-masked]> 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'