bug#28936: move_it_in_display_line_to returns MOVE_POS_MATCH_OR_ZV before ZV

bug#28936: move_it_in_display_line_to returns MOVE_POS_MATCH_OR_ZV before ZV

[Keith David Bershatsky]

A couple of observations:

1.  `move_it_in_display_line' could use an additional comment at the outset to let readers know that it is not compatible with moving to X in a horizontal scrolling and/or truncate lines situation.  The existing commentary that it was intended for external use was insufficient to deter me, and it required a learning curve on my part to better understand its limited potential use.

2.  When IT is on the last line in the buffer containing a few or more characters, `move_it_in_display_line_to' stops short of the target X and erroneously returns MOVE_POS_MATCH_OR_ZV when used as follows.  I have display-line-numbers set to a non-nil value in the event that makes a difference.  There is nothing special in terms of text-properties or overlays present in the buffer.

    int target_x = [Some arbitrary X that is a few characters before ZV.];

    move_it_in_display_line_to (it, ZV, target_x, MOVE_TO_POS | MOVE_TO_X);

The workaround is to compare the result of MOVE_POS_MATCH_OR_ZV with IT_CHARPOS to ensure that we are really at a ZV situation.

Here is a working draft of what I am using for my crosshairs (#17684).

int
move_it_in_display_line_to_x (struct it *it, int target_x)
{
  struct it saved_it;
  void *saved_data = bidi_shelve_cache ();
  enum move_it_result rc = MOVE_X_REACHED;
  int new_x, prev_x;
  /* Advance straight to `it->first_visible_x` if IT is prior thereto. */
  if (it->current_x < it->first_visible_x)
    move_it_in_display_line_to (it, ZV, it->first_visible_x, MOVE_TO_POS | MOVE_TO_X);
  while (it->current_x + it->pixel_width <= target_x)
    {
      SAVE_IT (saved_it, *it, saved_data);
      new_x = it->current_x + it->pixel_width;
      if (new_x == it->current_x)
        new_x++;
      rc = move_it_in_display_line_to (it, ZV, new_x, MOVE_TO_POS | MOVE_TO_X);
      if (ITERATOR_AT_END_OF_LINE_P (it)
          || FETCH_BYTE (IT_BYTEPOS (it)) == '\n'
          /* There is a bug in `move_it_in_display_line_to' such that it returns
          MOVE_POS_MATCH_OR_ZV before reaching ZV when the latter is at the end
          of the line:  abcdefg[EOB].  The workaround is to add an extra check
          using IT_CHARPOS and comparing it to ZV. */
          || (rc == MOVE_POS_MATCH_OR_ZV
              && IT_CHARPOS (*it) == ZV))
        break;
    }
  /* When word-wrap is on, TO_X may lie past the end of a wrapped line.
  Then it->current is the character on the next line, so backtrack to the
  space before the wrap point.  */
  if (it->line_wrap == WORD_WRAP
      && rc == MOVE_LINE_CONTINUED)
    {
      prev_x = max (it->current_x - 1, 0);
      RESTORE_IT (it, &saved_it, saved_data);
      move_it_in_display_line_to (it, -1, prev_x, MOVE_TO_X);
    }
  bidi_unshelve_cache (saved_data, true);
  return rc;
}

Thanks,

Keith

bug#28936: move_it_in_display_line_to returns MOVE_POS_MATCH_OR_ZV before ZV

[Keith David Bershatsky]

Here is a second draft move_it_in_display_line_to_x, with compatibility for word-wrap and also horizontal scrolling the current line that erroneously returns MOVE_LINE_TRUNCATED when trying to reach a target X that is about 1,000 pixels from the beginning of the line.

int
move_it_in_display_line_to_x (struct window *w, struct it *it, int target_x)
{
  struct it saved_it;
  void *saved_data = bidi_shelve_cache ();
  enum move_it_result rc = MOVE_X_REACHED;
  int new_x, prev_x;
  /* Advance straight to `it->first_visible_x` if IT is prior thereto. */
  if (it->current_x < it->first_visible_x)
    move_it_in_display_line_to (it, ZV, it->first_visible_x, MOVE_TO_POS | MOVE_TO_X);
  /* When horizontal scrolling a long line that approach or exceed an `it.current.x`
  of approximately 1000, `rc` will erroneously return early as MOVE_LINE_TRUNCATED
  without pushing on forwards until it reaches the target_x.  As a workaround, we
  ignore MOVE_LINE_TRUNCATED.  It is uncertain whether this is a bug. */
  while (it->current_x + it->pixel_width <= target_x
         && (rc == MOVE_X_REACHED
             || rc == MOVE_LINE_TRUNCATED
             || (it->line_wrap == WORD_WRAP
                 && rc == MOVE_POS_MATCH_OR_ZV)))
    {
      SAVE_IT (saved_it, *it, saved_data);
      new_x = it->current_x + it->pixel_width;
      if (new_x == it->current_x)
        new_x++;
      rc = move_it_in_display_line_to (it, ZV, new_x, MOVE_TO_POS | MOVE_TO_X);
      if (ITERATOR_AT_END_OF_LINE_P (it)
          || FETCH_BYTE (IT_BYTEPOS (*it)) == '\n'
          /* There is a bug in `move_it_in_display_line_to' such that it returns
          MOVE_POS_MATCH_OR_ZV before reaching ZV when the latter is at the end
          of the line:  abcdefg[EOB].  The workaround is to add an extra check
          using IT_CHARPOS and comparing it to ZV. */
          || (rc == MOVE_POS_MATCH_OR_ZV
              && IT_CHARPOS (*it) == ZV))
        break;
    }
  /* When word-wrap is on, TO_X may lie past the end of a wrapped line.
  Then it->current is the character on the next line, so backtrack to the
  space before the wrap point.  */
  if (it->line_wrap == WORD_WRAP
      && rc == MOVE_LINE_CONTINUED)
    {
      prev_x = max (it->current_x - 1, 0);
      RESTORE_IT (it, &saved_it, saved_data);
      move_it_in_display_line_to (it, -1, prev_x, MOVE_TO_X);
    }
  bidi_unshelve_cache (saved_data, true);
  return rc;
}

bug#28936: move_it_in_display_line_to returns MOVE_POS_MATCH_OR_ZV before ZV

[Eli Zaretskii]

> Date: Sat, 21 Oct 2017 22:14:39 -0700
> From: Keith David Bershatsky <esq@lawlist.com>
> 
> Here is a second draft move_it_in_display_line_to_x, with compatibility for word-wrap and also horizontal scrolling the current line that erroneously returns MOVE_LINE_TRUNCATED when trying to reach a target X that is about 1,000 pixels from the beginning of the line.

Once again, please show either a recipe in "emacs -Q" or a stand-alone
code snippet, which demonstrate the issues you mention.  FWIW, I
couldn't find such vulnerabilities by code inspection.

bug#28936: move_it_in_display_line_to returns MOVE_POS_MATCH_OR_ZV before ZV

[Eli Zaretskii]

> Date: Sat, 21 Oct 2017 20:02:58 -0700
> From: Keith David Bershatsky <esq@lawlist.com>
> 
> 1.  `move_it_in_display_line' could use an additional comment at the outset to let readers know that it is not compatible with moving to X in a horizontal scrolling and/or truncate lines situation.  The existing commentary that it was intended for external use was insufficient to deter me, and it required a learning curve on my part to better understand its limited potential use.

More commentary can never do any harm, but the incompatibility you
describe is news to me.  Can you show a recipe in "emacs -Q" that
would demonstrate this issue?  Or show a complete code snippet,
starting with start_display or init_iterator, where this issue
happens?

> 2.  When IT is on the last line in the buffer containing a few or more characters, `move_it_in_display_line_to' stops short of the target X and erroneously returns MOVE_POS_MATCH_OR_ZV when used as follows.  I have display-line-numbers set to a non-nil value in the event that makes a difference.  There is nothing special in terms of text-properties or overlays present in the buffer.
> 
>     int target_x = [Some arbitrary X that is a few characters before ZV.];
> 
>     move_it_in_display_line_to (it, ZV, target_x, MOVE_TO_POS | MOVE_TO_X);

You don't show how the value of 'it' was set up before this call, so
it's hard to look into this issue.  I can only tell that I've reviewed
all the code paths that lead to MOVE_POS_MATCH_OR_ZV being returned,
and they all seem to be conditioned either on reaching the position or
hitting ZV.  Maybe I missed something, but without a complete code
snippet that's the best I could do.

> The workaround is to compare the result of MOVE_POS_MATCH_OR_ZV with IT_CHARPOS to ensure that we are really at a ZV situation.

MOVE_POS_MATCH_OR_ZV doesn't necessarily mean you are at ZV, you could
also be at POS or after it.

bug#28936: move_it_in_display_line_to returns MOVE_POS_MATCH_OR_ZV before ZV

[Keith David Bershatsky]

Thank you, Eli, for looking into #28936.

With respect to `move_it_in_display_line`, I was not able to reliably land on `it.first_visible_x + lnum_pixel_width` when horizontal scrolling to the right AND text truncated on the left.  IT overshoots the target by one text character.  After trial an error, I assumed that this one text character overshoot was due to a built-in limitation of that function.  Hence, I have written `move_it_in_display_line_to_x` so that I can reach any X.

I was able to reproduce the MOVE_POS_MATCH_OR_ZV bug when word-wrap is non-nil.  I have disabled the workaround of `&& IT_CHARPOS (*IT) == ZV` so that we can see the bug in action.

I have not yet created a minimal working example to demonstrate the MOVE_LINE_TRUNCATED bug when trying to reach an X that is approximately 1000 or greater while horizontal scrolling a current line.  There is a comment about that issue below:  "When `auto-hscroll-mode' is set to ..."

Here is a recipe demonstrating the MOVE_POS_MATCH_OR_ZV bug:

STEP #1:  Add the two functions below at an appropriate location in xdisp.c.

STEP #2:  Launch the Emacs containing the new functions.

STEP #3:  In a scratch buffer, type:  Hello world!

[ZV must immediately follow "!" for this example to work.]

STEP #4:  Place the cursor somewhere on the word "world", and evaluate: (bug-28936 (selected-window))

STEP #5:  Observe that the result is "1" when it should be "2".

int
move_it_in_display_line_to_x (struct window *w, struct it *it, int target_x)
{
  struct it saved_it;
  void *saved_data = bidi_shelve_cache ();
  enum move_it_result rc = MOVE_X_REACHED;
  int new_x, prev_x;
  /* Advance straight to `it->first_visible_x` if IT is prior thereto. */
  if (it->current_x < it->first_visible_x)
    move_it_in_display_line_to (it, ZV, it->first_visible_x, MOVE_TO_POS | MOVE_TO_X);
  /* When `auto-hscroll-mode' is set to `current-line` and we are horizontal scrolling
  a long line that approaches or exceeds an `it.current.x` of approximately 1000, `rc`
  will erroneously return early with a MOVE_LINE_TRUNCATED indicator  without pushing
  forwards until IT reaches the target_x.  As a workaround, ignore MOVE_LINE_TRUNCATED. */
  while (it->current_x + it->pixel_width <= target_x
         && (rc == MOVE_X_REACHED
             || rc == MOVE_LINE_TRUNCATED
             || (it->line_wrap == WORD_WRAP
                 && rc == MOVE_POS_MATCH_OR_ZV)))
    {
      SAVE_IT (saved_it, *it, saved_data);
      new_x = it->current_x + it->pixel_width;
      if (new_x == it->current_x)
        new_x++;
      rc = move_it_in_display_line_to (it, ZV, new_x, MOVE_TO_POS | MOVE_TO_X);
      if (ITERATOR_AT_END_OF_LINE_P (it)
          || FETCH_BYTE (IT_BYTEPOS (*it)) == '\n'
          /* There is a bug in `move_it_in_display_line_to' such that it returns
          MOVE_POS_MATCH_OR_ZV before reaching ZV when the latter is at the end
          of the line:  abcdefg[EOB].  The workaround is to add an extra check
          using IT_CHARPOS and comparing it to ZV. */
          || (rc == MOVE_POS_MATCH_OR_ZV
              /* && IT_CHARPOS (*it) == ZV */
              ))
        break;
    }
  /* When word-wrap is on, TO_X may lie past the end of a wrapped line.
  Then it->current is the character on the next line, so backtrack to the
  space before the wrap point.  */
  if (it->line_wrap == WORD_WRAP
      && rc == MOVE_LINE_CONTINUED)
    {
      prev_x = max (it->current_x - 1, 0);
      RESTORE_IT (it, &saved_it, saved_data);
      move_it_in_display_line_to (it, -1, prev_x, MOVE_TO_X);
    }
  bidi_unshelve_cache (saved_data, true);
  return rc;
}

DEFUN ("bug-28936", Fbug_28936, Sbug_28936, 1, 1, 0,
       doc: /* Demonstrate the Emacs bug # 28936. */)
  (Lisp_Object window)
{
  struct window *w = decode_live_window (window);
  struct it it;
  void *itdata = bidi_shelve_cache ();
  struct text_pos start_text_position;
  int target_x;
  enum move_it_result rc;
  struct buffer *b = XBUFFER (w->contents);
  b->word_wrap_ = Qt;
  SET_TEXT_POS_FROM_MARKER (start_text_position, w->start);
  start_display (&it, w, start_text_position);
  move_it_to (&it, PT, it.last_visible_x, it.last_visible_y - 1, -1, MOVE_TO_POS | MOVE_TO_X | MOVE_TO_Y);
  target_x = it.current_x;
  move_it_by_lines (&it, 0);
  rc = move_it_in_display_line_to_x (w, &it, target_x);
  bidi_unshelve_cache (itdata, false);
  return make_number (rc);
}

bug#28936: move_it_in_display_line_to returns MOVE_POS_MATCH_OR_ZV before ZV

[Eli Zaretskii]

> Date:  Sun, 22 Oct 2017 11:05:45 -0700
> From:  Keith David Bershatsky <esq@lawlist.com>
> Cc:  28936@debbugs.gnu.org
> 
> Thank you, Eli, for looking into #28936.

I don't think there's any bug here.

> With respect to `move_it_in_display_line`, I was not able to reliably land on `it.first_visible_x + lnum_pixel_width` when horizontal scrolling to the right AND text truncated on the left.  IT overshoots the target by one text character.

This means you have an off-by-one error, probably because you are
not computing the target X coordinate correctly.  The mistake I show
below is probably the same mistake you do elsewhere.

>   while (it->current_x + it->pixel_width <= target_x

This condition will cause the iterator to attempt to get to the
character _after_ point, where it will hit ZV.  So what you see, viz.:

> STEP #5:  Observe that the result is "1" when it should be "2".

is expected.  You should change the condition to this:

   while (it->current_x < target_x

bug#28936: move_it_in_display_line_to returns MOVE_POS_MATCH_OR_ZV before ZV

[Eli Zaretskii]

> Date:  Sun, 22 Oct 2017 11:05:45 -0700
> From:  Keith David Bershatsky <esq@lawlist.com>
> Cc:  28936@debbugs.gnu.org
> 
>   SET_TEXT_POS_FROM_MARKER (start_text_position, w->start);
>   start_display (&it, w, start_text_position);
>   move_it_to (&it, PT, it.last_visible_x, it.last_visible_y - 1, -1, MOVE_TO_POS | MOVE_TO_X | MOVE_TO_Y);
>   target_x = it.current_x;
>   move_it_by_lines (&it, 0);

Did you verify that it.current_x is zero after this line?  If not, you
will not get to the right X coordnate here:

>   rc = move_it_in_display_line_to_x (w, &it, target_x);

bug#28936: nRe: bug#28936: move_it_in_display_line_to returns MOVE_POS_MATCH_OR_ZV before ZV

[Keith David Bershatsky]

The issue is observable at a distance of approximately up to five (5) text characters prior to ZV, rather than a distance of just one (1) it->pixel_width.  And, the issue is only present when word-wrap is non-nil.  In the example of a buffer containing only "Hello world!", the wrong result is observable on the letter "w", and on the letter "o", and on the letter "r" and on the letter "l", and on the letter "d", and on the exclamation point.  If my recollection is collect when trying out the example, the wrong result was also observable on the space between "hello" and "world".

The code that I borrowed is from xdisp.c at approximately 22680:  "while (it.current_x + it.pixel_width <= target_x".

I am running out the door for a few hours, and can try changing the while loop condition when I return -- however, I am fairly confident that copying the code at 22680 for this particular use case is what is needed.

Thanks,

Keith

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

DATE:  [10-22-2017 11:30:45] <22 Oct 2017 21:30:45 +0300>
FROM:  Eli Zaretskii <eliz@gnu.org>
> 
> * * *
> 
> > With respect to `move_it_in_display_line`, I was not able to reliably land on `it.first_visible_x + lnum_pixel_width` when horizontal scrolling to the right AND text truncated on the left.  IT overshoots the target by one text character.
> 
> This means you have an off-by-one error, probably because you are
> not computing the target X coordinate correctly.  The mistake I show
> below is probably the same mistake you do elsewhere.
> 
> >   while (it->current_x + it->pixel_width <= target_x
> 
> This condition will cause the iterator to attempt to get to the
> character _after_ point, where it will hit ZV.  So what you see, viz.:
> 
> > STEP #5:  Observe that the result is "1" when it should be "2".
> 
> is expected.  You should change the condition to this:
> 
>    while (it->current_x < target_x

bug#28936: nRe: bug#28936: move_it_in_display_line_to returns MOVE_POS_MATCH_OR_ZV before ZV

[Eli Zaretskii]

> Date:  Sun, 22 Oct 2017 12:22:26 -0700
> From:  Keith David Bershatsky <esq@lawlist.com>
> Cc:  28936@debbugs.gnu.org
> 
> The issue is observable at a distance of approximately up to five (5) text characters prior to ZV, rather than a distance of just one (1) it->pixel_width.  And, the issue is only present when word-wrap is non-nil.  In the example of a buffer containing only "Hello world!", the wrong result is observable on the letter "w", and on the letter "o", and on the letter "r" and on the letter "l", and on the letter "d", and on the exclamation point.  If my recollection is collect when trying out the example, the wrong result was also observable on the space between "hello" and "world".

There's something you didn't tell, because otherwise what you describe
is done all the time by the display engine, and it is certainly
incorrect that it's impossible to get to a given X coordinate, with or
without word-wrap.

It is possible that you didn't reset current_x before the iteration,
or that the window-start point is not where you think it is.  Or
something else.  This code works, it's the workhorse of many Emacs
display operations.

> The code that I borrowed is from xdisp.c at approximately 22680:  "while (it.current_x + it.pixel_width <= target_x".

That doesn't mean anything: the code you copied tries to find the
character _before_ target_x, whereas you are trying to get to target_x
itself.

bug#28936: move_it_in_display_line_to returns MOVE_POS_MATCH_OR_ZV before ZV

[Keith David Bershatsky]

I was able to verify this evening that it.current_x is indeed 0 immediately following a call to `move_it_by_lines (&it, 0)` when the issue is present.

It may be that move_it_in_display_line_to is reaching the correct X, but is merely throwing the wrong label/result in this situation -- i.e., it is not really reaching a POS or ZV.  Since my loop was relying on the label/result, the loop exited too early.  To test the correct X and wrong label/result theory, would take some time for me to work on because I would like to find the precise location inside move_it_in_display_line_to where the label/result is being returned.  I will continue to think about this and tinker ...

Keith

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

DATE:  [10-22-2017 12:53:47] <22 Oct 2017 22:53:47 +0300>
FROM:  Eli Zaretskii <eliz@gnu.org>
> 
> > Date:  Sun, 22 Oct 2017 11:05:45 -0700
> > From:  Keith David Bershatsky <esq@lawlist.com>
> > Cc:  28936@debbugs.gnu.org
> > 
> >   SET_TEXT_POS_FROM_MARKER (start_text_position, w->start);
> >   start_display (&it, w, start_text_position);
> >   move_it_to (&it, PT, it.last_visible_x, it.last_visible_y - 1, -1, MOVE_TO_POS | MOVE_TO_X | MOVE_TO_Y);
> >   target_x = it.current_x;
> >   move_it_by_lines (&it, 0);
> 
> Did you verify that it.current_x is zero after this line?  If not, you
> will not get to the right X coordnate here:
> 
> >   rc = move_it_in_display_line_to_x (w, &it, target_x);

bug#28936: move_it_in_display_line_to returns MOVE_POS_MATCH_OR_ZV before ZV

[Eli Zaretskii]

> Date:  Sun, 22 Oct 2017 22:20:32 -0700
> From:  Keith David Bershatsky <esq@lawlist.com>
> Cc:  28936@debbugs.gnu.org
> 
> I was able to verify this evening that it.current_x is indeed 0 immediately following a call to `move_it_by_lines (&it, 0)` when the issue is present.

Then perhaps the problem happens because of some factors you didn't
include in your recipe.  The comments talk about adding
lnum_pixel_width, and about using auto-hscroll = current-line mode,
but the recipe mentions nothing about that.  Is the recipe complete?

> It may be that move_it_in_display_line_to is reaching the correct X, but is merely throwing the wrong label/result in this situation -- i.e., it is not really reaching a POS or ZV.

No, that's extremely improbable, to say the least.  Like I said: these
functions work in Emacs all the time, and if they'd fail like you
describe, we'd have gazillion of display bugs.

bug#28936: enhancement request: remove vertical scroll bar automatically when not needed

[Keith David Bershatsky]

Here is a first draft with a simple test (modifying xdisp.c), which probably nukes more than just the selected window's scroll bars when removing them, but it may be sufficient to revive this enhancement request in the event anyone is interested.

 finish_scroll_bars:

   if ((WINDOW_HAS_VERTICAL_SCROLL_BAR (w) || WINDOW_HAS_HORIZONTAL_SCROLL_BAR (w))
        && ZV - BEGV > BUF_Z (XBUFFER (w->contents)) - w->window_end_pos - marker_position (w->start))
    {
      if (WINDOW_HAS_VERTICAL_SCROLL_BAR (w))
	/* Set the thumb's position and size.  */
	set_vertical_scroll_bar (w);

      if (WINDOW_HAS_HORIZONTAL_SCROLL_BAR (w))
	/* Set the thumb's position and size.  */
	set_horizontal_scroll_bar (w);

      /* Note that we actually used the scroll bar attached to this
	 window, so it shouldn't be deleted at the end of redisplay.  */
      if (FRAME_TERMINAL (f)->redeem_scroll_bar_hook)
        (*FRAME_TERMINAL (f)->redeem_scroll_bar_hook) (w);
    }
    else
      {
        (*FRAME_TERMINAL (f)->condemn_scroll_bars_hook) (f);
        (*FRAME_TERMINAL (f)->judge_scroll_bars_hook) (f);
      }


Keith

bug#28936: enhancement request: remove vertical scroll bar automatically when not needed

[Keith David Bershatsky]

The previous message should have been sent to 16475, rather than 28936.  If a moderator would like to delete the last message, please feel free to do so.  I have already forwarded a duplicate of the previous message to 16475.

Thanks,

Keith