Re: move_it_vertically_backward question

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> @@ -10866,6 +10867,13 @@ window_text_pixel_size (Lisp_Object window, 
>> Lisp_Object from, Lisp_Object to, Li
>>          break;
>>      }
>>      }
>> +  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.  The
code takes the car of FROM, and then checks that the cdr of FROM is a
positive fixnum, which is a subtle difference from the other code.

> The name 'movement' is not the best name.  I would suggest 'offset' or
> maybe even 'y_offset' instead.

Thanks.

> 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?

>> +  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.

>> +      while (-it.current_y < movement)
>> +    {
>> +      last_y = it.current_y;
>> +      move_it_vertically_backward (&it, movement + it.current_y);
>> +
>> +      if (it.current_y == last_y)
>> +        break;
>> +    }
>> +
>> +      it.current_y = 0;
>> +      start = clip_to_bounds (BEGV, IT_CHARPOS (it), ZV);
>> +    }
>> +
>>    int start_y = it.current_y;
>>    /* It makes no sense to measure dimensions of region of text that
>>       crosses the point where bidi reordering changes scan direction.

> 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).

Thanks.