[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: move_it_vertically_backward question
From: |
Eli Zaretskii |
Subject: |
Re: move_it_vertically_backward question |
Date: |
Sun, 19 Dec 2021 10:29:32 +0200 |
> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Sun, 19 Dec 2021 08:54:29 +0800
>
> >> + else if (CONSP (from))
> >> + {
> >> + start = clip_to_bounds (BEGV, fix_position (XCAR (from)), ZV);
> >> + bpos = CHAR_TO_BYTE (start);
> >> + CHECK_FIXNAT (XCDR (from));
> >> + movement = XFIXNAT (XCDR (from));
> >> + }
> >> else
> >> {
> >> start = clip_to_bounds (BEGV, fix_position (from), ZV);
>
> > There's code duplication here, which it would be good to avoid: the
> > calculation of bpos and the call to clip_to_bounds in calculation of
> > start.
>
> I don't think the duplication here is a particularly big problem.
Not a big problem, it just makes the code a tad harder to read and
comprehend.
> > Also, the assumption that this offset is always positive, and that it
> > means the position _before_ FROM, is also something that makes the
> > function less general than it could be. It would be better to have
> > positive values mean _after_ FROM and negative values to mean before
> > it. Then you could use move_it_vertically instead of
> > move_it_vertically_backward.
>
> Does `move_it_vertically' have the same limitation of
> `move_it_vertically_backwards' where the iterator might end up either
> before or after the target position?
Look at the code: when DY is non-positive, move_it_vertically
delegates to move_it_vertically_backwards.
> >> + if (movement > 0)
> >> + {
> >> + int last_y;
> >> + it.current_y = 0;
> >> +
> >> + move_it_by_lines (&it, 0);
>
> > Why is this needed?
>
> That will move the iterator to the beginning of the visual line. I
> don't think it makes any sense to move it elsewhere, as IIUC the
> behaviour of moving an iterator upwards from a non-beginning position is
> not very well defined.
I think you assume that the new argument to window-text-pixel-size is
non-nil; then indeed we should move to the beginning of the visual
line. But in general, I don't think that's true.
Anyway, I don't necessarily object that we should do this always, but
then we should document that when FROM is a cons cell, that specifies
the beginning of the screen line at that pixel offset.
> > The code that follows this, which you leave intact, will reseat the
> > iterator to the beginning of the visual line where you ended up after
> > the loop that goes backward. Is that really TRT? what if there's a
> > multi-line display or overlay string at that place? Did you test the
> > code in that case? AFAIU, you have already assured that you are at
> > the beginning of a visual line, so the reseat, and the following
> > return to the START position, might not be needed. When you return to
> > START, you may end up very far from where the loop going backward
> > ended, if START is "covered" by a display property or overlay string.
>
> Yes, that's the correct behaviour here, at least for the pixel scrolling
> code (which I tested with both overlays and multi-line display
> properties).
When window-start is inside a display property or overlay (i.e., the
first thing shown in the window is the result of that display
property/overlay), starting from the underlying buffer position will
almost definitely affect the results, because that buffer position
could be at a very different position on the screen. For example,
what happens if window-start position has a before-string, and that
string has a 'display' property specifying an image? This should
display the image as the first display element at the window
beginning, and the buffer position of window-start will then be to the
right and possibly also at a different Y coordinate.
- Re: move_it_vertically_backward question, (continued)
- Re: move_it_vertically_backward question, Po Lu, 2021/12/16
- Re: move_it_vertically_backward question, Eli Zaretskii, 2021/12/18
- Re: move_it_vertically_backward question, Po Lu, 2021/12/18
- Re: move_it_vertically_backward question, Eli Zaretskii, 2021/12/18
- Re: move_it_vertically_backward question, Po Lu, 2021/12/18
- Re: move_it_vertically_backward question, Eli Zaretskii, 2021/12/18
- Re: move_it_vertically_backward question, Po Lu, 2021/12/18
- Re: move_it_vertically_backward question, Eli Zaretskii, 2021/12/18
- Re: move_it_vertically_backward question, Po Lu, 2021/12/18
- Re: move_it_vertically_backward question, Po Lu, 2021/12/18
- Re: move_it_vertically_backward question,
Eli Zaretskii <=
- Re: move_it_vertically_backward question, Po Lu, 2021/12/19
- Re: move_it_vertically_backward question, Eli Zaretskii, 2021/12/19
- Re: move_it_vertically_backward question, Po Lu, 2021/12/19
- Re: move_it_vertically_backward question, Eli Zaretskii, 2021/12/19
- Re: move_it_vertically_backward question, Po Lu, 2021/12/19
- Re: move_it_vertically_backward question, Po Lu, 2021/12/21
- Re: move_it_vertically_backward question, Eli Zaretskii, 2021/12/21
- Re: move_it_vertically_backward question, Po Lu, 2021/12/21
- Re: move_it_vertically_backward question, Eli Zaretskii, 2021/12/22
- Re: move_it_vertically_backward question, Po Lu, 2021/12/22