From: Eli Zaretskii <eliz@gnu.org>
To: dick.r.chiang@gmail.com
Cc: 52302@debbugs.gnu.org
Subject: bug#52302: 28.0.50; [PATCH] Overlay strings should not increment vpos
Date: Sun, 05 Dec 2021 20:34:50 +0200 [thread overview]
Message-ID: <83lf0y3np1.fsf@gnu.org> (raw)
In-Reply-To: <87v903hs0w.fsf@dick> (dick.r.chiang@gmail.com)
> 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?
next prev parent reply other threads:[~2021-12-05 18:34 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-05 17:37 bug#52302: 28.0.50; [PATCH] Overlay strings should not increment vpos dick.r.chiang
2021-12-05 18:34 ` Eli Zaretskii [this message]
2021-12-05 18:52 ` dick
2021-12-05 19:06 ` dick
2021-12-05 19:47 ` Eli Zaretskii
2021-12-05 22:10 ` dick
2021-12-06 19:47 ` dick
2021-12-06 20:02 ` Eli Zaretskii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=83lf0y3np1.fsf@gnu.org \
--to=eliz@gnu.org \
--cc=52302@debbugs.gnu.org \
--cc=dick.r.chiang@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).