>>>>> "EZ" == Eli Zaretskii writes: >> 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 >> 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. EZ> Please tell more about the motivation. In which use cases this EZ> change behaves better, and why? This is a delicate code, used in EZ> many places, so we need a very good understanding of what gets EZ> 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; EZ> I don't think I see the connection between the above condition and EZ> the need to increment (or not increment) VPOS. Can you elaborate on EZ> 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); EZ> Why are you replacing the assertions here? >> --- a/test/src/xdisp-tests.el >> +++ b/test/src/xdisp-tests.el EZ> What exactly is changed in this test? It looks like purely EZ> stylistic changes to me (which for some reason also lots the EZ> comments). Did I miss something?