[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#52302: 28.0.50; [PATCH] Overlay strings should not increment vpos
From: |
Eli Zaretskii |
Subject: |
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@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@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?