GNU bug report logs - #52302
28.0.50; [PATCH] Overlay strings should not increment vpos

Previous Next

Full log

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: dick.r.chiang <at> gmail.com
Cc: 52302 <at> debbugs.gnu.org
Subject: Re: bug#52302: 28.0.50;
 [PATCH] Overlay strings should not increment vpos
Date: Sun, 05 Dec 2021 20:34:50 +0200

> From: dick.r.chiang <at> gmail.com
> Date: Sun, 05 Dec 2021 12:37:35 -0500
> 
> >From 010b26de2993754db6bb42243b5c6c89fc5e8a50 Mon Sep 17 00:00:00 2001
> From: dickmao <dick.r.chiang <at> gmail.com>
> Date: Sun, 5 Dec 2021 12:25:32 -0500
> Subject: [PATCH] Overlay strings at `to_charpos` should not increment vpos
> 
> Previously, two calls to `move_it_vertically_backward (it, 0)` were
> required to get IT back to line start.  It should only ever
> take one call.

Please tell more about the motivation.  In which use cases this change
behaves better, and why?  This is a delicate code, used in many
places, so we need a very good understanding of what gets fixed.

> +	      else if (skip == MOVE_LINE_CONTINUED
> +		       && it->method == GET_FROM_STRING
> +		       && IT_CHARPOS (*it) == to_charpos)
> +		/* TO_CHARPOS reached, now consuming overlay string. */

it->method == GET_FROM_STRING doesn't necessarily mean we are
it->consuming an overlay string.  It could be a string from display
it->property, for example.

> -      ++it->vpos;
> +      if (! reached_continued)
> +	++it->vpos;

I don't think I see the connection between the above condition and the
need to increment (or not increment) VPOS.  Can you elaborate on that?

> @@ -10490,11 +10497,11 @@ move_it_vertically_backward (struct it *it, int dy)
>  	   || (it2.method == GET_FROM_STRING
>  	       && IT_CHARPOS (it2) == start_pos
>  	       && SREF (it2.string, IT_STRING_BYTEPOS (it2) - 1) == '\n')));
> -  eassert (IT_CHARPOS (*it) >= BEGV);
> +  eassert (IT_CHARPOS (it2) >= BEGV);
>    SAVE_IT (it3, it2, it3data);
>  
>    move_it_to (&it2, start_pos, -1, -1, -1, MOVE_TO_POS);
> -  eassert (IT_CHARPOS (*it) >= BEGV);
> +  eassert (IT_CHARPOS (it2) >= BEGV);

Why are you replacing the assertions here?

> --- a/test/src/xdisp-tests.el
> +++ b/test/src/xdisp-tests.el

What exactly is changed in this test?  It looks like purely stylistic
changes to me (which for some reason also lots the comments).  Did I
miss something?

Previous Next

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