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>
View this message in rfc822 format
From: JD Smith <jdtsmith <at> gmail.com> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 67604 <at> debbugs.gnu.org Subject: bug#67604: Motion problems with inline images Date: Thu, 8 May 2025 16:58:36 -0400
[Message part 1 (text/plain, inline)]
> On May 7, 2025, at 8:00 AM, Eli Zaretskii <eliz <at> gnu.org> wrote: > > >> >> From: JD Smith <jdtsmith <at> gmail.com> >> Date: Tue, 6 May 2025 23:30:02 -0400 >> Cc: 67604 <at> debbugs.gnu.org >> >>> I've eventually succeeded in reproducing it. I will get to it when I >>> have time; however, with the current tempest on emacs-devel and other >>> urgent issues, I don't know when will that be. >> >> This bug remains present in Emacs v30. > > Sorry, I couldn't find the time to look at this. No worries at all. You juggle many things and it was a good impetus to get a src debugging setup working (I wish realgud worked better with lldb, but command line is ok for now). It’s also a fairly subtle and specific bug, even though I think it underlies many(/all?) small movement oddities I’ve noticed over the years with inline wrapped wide glyphs. >> What I'm looking for is a reason why a wide glyph (only) at the very start of a wrapped visual display line would have its pixel width incorrectly stored, as if it were just a regular width character. >> >> I tried to get a sense of where this IT pixel_width is getting set, but it's referenced all over xdisp.c, and I have only a rudimentary understanding of the iterator structure (and its glyph rows, etc.). >> >> If you have any hunches where this mistaken it->pixel_width might be sneaking in for wrapped wide glyphs, I'd be very happy to investigate further using my debug setup here. > > it->pixel_width comes from the call to PRODUCE_GLYPHS (which is a > macro that calls gui_produce_glyphs). For characters, > gui_produce_glyphs does the job itself, under this condition: > > if (it->what == IT_CHARACTER) > > For images, it calls produce_image_glyph. > > I hope this is the information you were looking for. If not, please > ask more specific questions. I appreciate these tips. I took a long look, and my initial assessment was incorrect — the glyph pixel sizes are correct, but the iterator is confused about where the line start is. IT thinks it is at the beginning of the line, but has in fact over-shot, just to the right of the wrapped wide glyph. Here's a snapshot of the problem. There are no images in the buffer any longer — the gray boxes are both simple specified space applied on single SPC chars. The behavior is the same as before. Point jumps from the beginning of the 2nd line on the light gray stretch right to JUMPS HERE. Other forms of incorrect motion are evident, but this is the most dramatic, so I stick with it.
[PastedGraphic-6.png (image/png, inline)]
[Message part 3 (text/plain, inline)]
Here is the stack I'm investigating:
* 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
frame #1: 0x00000001000314b0 Emacs`move_it_to(it=0x0000000170793220, to_charpos=-1, to_x=-1, to_y=-1, to_vpos=1, op=4) at xdisp.c:10667:10
frame #2: 0x000000010002cadc Emacs`move_it_by_lines(it=0x0000000170793220, dvpos=1) at xdisp.c:11240:7
frame #3: 0x00000001001d787c Emacs`Fvertical_motion(lines=(EMACS_INT) $4 = 1, window=(struct Lisp_Symbol *) $22 = 0x0000000100a73070, cur_col=(struct Lisp_Symbol *) $43 = 0x0000000100a73070) at indent.c:2406:4
frame #4: 0x0000000100240adc Emacs`funcall_subr_1(subr=0x000000010045aae0, numargs=1, args=(struct Lisp_Symbol *) $72 = 0x0000000218bbb270) at eval.c:3176:15
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. 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".
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've tried various approaches, but have struggled to come up with a full fix.
A few questions that might get me closer:
1. Why do IMAGES/STRETCHES get a different overshoot count?
else
it_overshoot_count =
/* If image_id is negative, it's a fringe bitmap, which by
definition doesn't affect display in the text area. */
!((it.method == GET_FROM_IMAGE && it.image_id >= 0)
|| it.method == GET_FROM_STRETCH);
A (real) IMAGE or STRETCH at the starting position of the vertical movement changes the overshoot to 0 (it seems to be 1 by default). Yet no allowance is made for reseat -> move_it_to skipping past the IMAGE/STRETCH, as is done for display *strings*. I don't see why the mere presence of an IMAGE/STRETCH should tell you anything concrete about the overshoot.
2. How can you tell if the position prior to the IMAGE/STRETCH is on a prior visual line?
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)
{
/* The position we started from was covered by a display
property, so we moved to position before the string, and
backed up one line, because the character at PT - 1 is
a newline. So we need one less line to go up (or exactly
one line to go down if nlines == 0). */
nlines++;
/* But we still need to record that one line, in order to
return the correct value to the caller. */
vpos_init = -1;
overshoot_handled = 1;
}
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.
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?
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.
I think this proposed fix should apply equally to display strings, images, and stretches which have been wrapped (it.line_wrap != TRUNCATE).
Thoughts/ideas?
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.