GNU bug report logs - #67604
Motion problems with inline images

Previous Next

Full log

Message #83 received at 67604 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: JD Smith <jdtsmith <at> gmail.com>
Cc: 67604 <at> debbugs.gnu.org
Subject: Re: bug#67604: Motion problems with inline images
Date: Fri, 09 May 2025 19:17:05 +0300

> From: JD Smith <jdtsmith <at> gmail.com>
> Date: Thu, 8 May 2025 16:58:36 -0400
> Cc: 67604 <at> debbugs.gnu.org
> 
> At this location, I have:
> 
> (lldb) br en
> All breakpoints enabled. (1 breakpoints)
> Process 35347 stopped
> * thread #2, name = 'org.gnu.Emacs.lisp-main', stop reason = breakpoint 1.1
>     frame #0: 0x00000001000357c0 Emacs`move_it_in_display_line_to(it=0x0000000170793220, to_charpos=-1, to_x=-1, op=0x0) at xdisp.c:10118:11
>    10115    10116 Note that both for tabs and padding glyphs, all glyphs have
>    10117 the same width.  */
> -> 10118      if (it->nglyphs)
>    10119 {
>    10120  /* More than one glyph or glyph doesn't fit on line.  All
>    10121     glyphs have the same width.  */
> Target 0: (Emacs) stopped.
> (lldb) p it->current
> (display_pos) {
>   pos = (charpos = 44, bytepos = 44)
>   overlay_string_index = -1
>   string_pos = (charpos = -1, bytepos = -1)
>   dpvec_index = -1
> }
> (lldb) p it->hpos
> (int) 0
> (lldb) p it->current_x
> (int) 7
> (lldb) p it->last_visible_x
> (int) 546
> (lldb) p it->pixel_width 
> (int) 7
> (lldb) 
> 
> Note that the light gray specified space is at pos=43.  I conclude IT thinks it is at column 0 (assuming HPOS is meaningful), and has a current_x of 7 (one normal char width).  When in reality, due to the wide glyph, it got pushed ahead to the pink position above, at pos=44.

Thanks, I think you made good progress.

But you are looking at the horizontal coordinates too early: they have
not yet been updated at that point.  They are updated inside the for
loop which starts a few lines down.

> From there it iterates until new_x >= last_visible_x, and in so doing, "falls off the line".  If it falls to the final newline of the subsequent wrapped line, this results in a "line jump".

You lost me here.  I don't see "new_x >= it->last_visible_x" in the
source, so which line did you have in mind?

> GLYPH SKIP:
> ===========
> 
> Narrowing down further, here is the location where point first gets pushed ahead to the pink box in the screenshot (in indent.c:Fvertical_motion, line 2315).  Just prior to this, IT is at buffer pos=1 (having been moved there by `reseat_at_previous_visible_line_start').  After this, it's at pos=44, on the pink box.
> 
> /* When the position we started from is covered by a display
>   string, move_it_to will overshoot it, while vertical-motion
>   wants to put the cursor _before_ the display string.  So in
>   that case, we move to buffer position before the display
>   string, and avoid overshooting.  But if the position before
>   the display string is a newline, we don't do this, because
>   otherwise we will end up in a screen line that is one too
>   far back.  */
> move_it_to (&it,
>    (!disp_string_at_start_p
>     || FETCH_BYTE (IT_BYTEPOS (it)) == '\n')
>    ? PT
>    : PT - 1,
>    -1, -1, -1, MOVE_TO_POS);
> 
> This comment is very telling, and set off some bells.  That's seems to be EXACTLY what is happening here: `move_it_to(&it, PT, MOVE_TO_POS)' overshoots the display element (here, specified space), and lands at pos=44 (pink box).   
> 
> So why is this commented-upon "fix" not working?  Because  disp_string_at_start_p=false, since a display *image* or *specified space* is not a display *string*.
> 
> This flag is set above:
> 
> else if (it.method == GET_FROM_STRING)
> {
>  const char *s = SSDATA (it.string);
>  const char *e = s + SBYTES (it.string);
> 
>  disp_string_at_start_p =
>  /* If it.area is anything but TEXT_AREA, we need not bother
>     about the display string, as it doesn't affect cursor
>     positioning.  */
>    it.area == TEXT_AREA
>    && it.string_from_display_prop_p
>    /* A display string on anything but buffer text (e.g., on
>       an overlay string) doesn't affect cursor positioning.  */
>    && (it.sp > 0 && it.stack[it.sp - 1].method == GET_FROM_BUFFER);
> 
> SOLUTION(S):
> ============
> 
> It would seem the simple solution is to do the same thing for images and stretches as is done for display strings: move to the position right *before* PT. 
> 
> That could be as simple as enabling disp_string_at_start_p when starting on a (real) IMAGE or STRETCH.  That does indeed fix the bug, but unfortunately it results in a new bug: now moving vertically *upwards* across wrapped wide glyphs jumps an extra line up.  Sigh.

I'm not sure your conclusions are correct.  If they were, it would
mean that vertical-motion doesn't work at all when the first glyph on
a line is an image or a stretch glyph (what you call "specified
space").  But that is not true, is it?

I think this problem is specific to the situation when the following
happen all at once:

 . lines are wrapped (it->line_wrap = WORD_WRAP)
 . the image or the stretch glyph don't fit on their line and are
   wrapped to the next line
 . vertical-motion is invoked when point is on or after that image or
   stretch

Am I right?

If I'm right, then we should try understanding why in this particular
situation the code doesn't work, whereas it does work when, say, the
image or stretch are in a non-continuation line or are not the first
glyph.

> 1. Why do IMAGES/STRETCHES get a different overshoot count?

I don't remember, sorry.  If you really need to know (and I'm not
sure), you will have to use "git -L" to see the history of this code,
and then look up the bug reports which led to this code.

> 2. How can you tell if the position prior to the IMAGE/STRETCH is on a prior visual line?

By looking at the Y coordinate (it.current_t or it.vpos), I'd say.

> Later in `Fvertical_motion' there is this test, which looks very promising:
> 
>       else if (IT_CHARPOS (it) == PT - 1
>       && FETCH_BYTE (PT_BYTE - 1) == '\n'
>       && nlines <= 0)

This only works if the previous line ended in a newline, but will not
work with continuation lines.

> But note, it looks for an *explicit newline* before the display property, which is obviously not there for wrapped lines.  I've tried to use it.vpos to find out if you got moved by a screen line, but that seems to be more of a "delta vpos", apparently set only AFTER moving back to the starting point with move_it_to.

Yes, vpos counts from zero, where the iterator was initialized.

> 3. It seems what's relevant for wide glyphs is not only "Was there a newline behind you?", but "Was there a newline behind you, or was the position there at a lower vpos?".  Is there a way to use vpos to make a test like this?

I don't know yet, the answer should reveal itself if you step through
the code and see what happens there in this particular situation (but
not in other similar ones).

> The only idea I could think of is (assuming no newline behind):
> 
> a. First move to PT.  Store vpos.
> b. If you started on a display property (not on a newline), now move back to PT-1.  
> c. Store and compare the vpos values.
> c. If new_vpos<stored_vpos, your display entity is at the beginning of a visual line, and you'd better update your nlines/overshoot.
> 
> But I haven't gotten it to work. I'm also not sure if that will mess up the x position, etc. 

It's too early to devise solutions, because we don't understand the
root cause of the problem yet.  At least I don't.

> I think this proposed fix should apply equally to display strings, images, and stretches which have been wrapped (it.line_wrap != TRUNCATE).  

I'm not yet convinced, since vertical-motion does work in general when
images and stretch glyphs are around.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.