bug#52302: 28.0.50; [PATCH] Overlay strings should not increment vpos

[dick]

>From 390e5866888a740bca2daaa9b528d1905df3ffd7 Mon Sep 17 00:00:00 2001
From: dickmao <dick.r.chiang@gmail.com>
Date: Sun, 5 Dec 2021 13:46:59 -0500
Subject: [PATCH] Overlay strings at `to_charpos` should not increment vpos

* src/xdisp.c (move_it_to): Recognize the invisible overlay
string should not increment vpos.
(move_it_vertically_backward): Rectify assertions.
(resize_mini_window): Should not need to call
move_it_vertically_backward (it, 0) twice.
* test/src/xdisp-tests.el (xdisp-tests--minibuffer-resizing):
Test.
---
 src/xdisp.c             | 41 ++++++++++++++++++++---------------------
 test/src/xdisp-tests.el | 30 +++++++++++++-----------------
 2 files changed, 33 insertions(+), 38 deletions(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index 0ff6286af74..c264d7a3be1 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -10081,6 +10081,7 @@ move_it_to (struct it *it, ptrdiff_t to_charpos, int 
to_x, int to_y, int to_vpos
 
   for (;;)
     {
+      bool reached_continued = false;
       orig_charpos = IT_CHARPOS (*it);
       orig_method = it->method;
       if (op & MOVE_TO_VPOS)
@@ -10125,6 +10126,12 @@ move_it_to (struct it *it, ptrdiff_t to_charpos, int 
to_x, int to_y, int to_vpos
                      break;
                    }
                }
+             else if (skip == MOVE_LINE_CONTINUED
+                      && op & MOVE_TO_POS
+                      && it->method == GET_FROM_STRING
+                      && IT_CHARPOS (*it) == to_charpos)
+               /* TO_CHARPOS reached, now consuming overlay string. */
+               reached_continued = true;
            }
        }
       else if (op & MOVE_TO_Y)
@@ -10377,13 +10384,16 @@ move_it_to (struct it *it, ptrdiff_t to_charpos, int 
to_x, int to_y, int to_vpos
       /* Reset/increment for the next run.  */
       recenter_overlay_lists (current_buffer, IT_CHARPOS (*it));
       it->current_x = line_start_x;
-      line_start_x = 0;
-      it->hpos = 0;
-      it->line_number_produced_p = false;
-      it->current_y += it->max_ascent + it->max_descent;
-      ++it->vpos;
       last_height = it->max_ascent + it->max_descent;
-      it->max_ascent = it->max_descent = 0;
+      if (! reached_continued)
+       {
+         line_start_x = 0;
+         it->hpos = 0;
+         it->line_number_produced_p = false;
+         it->current_y += it->max_ascent + it->max_descent;
+         ++it->vpos;
+         it->max_ascent = it->max_descent = 0;
+       }
     }
 
  out:
@@ -10490,11 +10500,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);
   /* H is the actual vertical distance from the position in *IT
      and the starting position.  */
   h = it2.current_y - it->current_y;
@@ -12218,7 +12228,6 @@ resize_mini_window (struct window *w, bool exact_p)
       struct it it;
       int unit = FRAME_LINE_HEIGHT (f);
       int height, max_height;
-      struct text_pos start;
       struct buffer *old_current_buffer = NULL;
       int windows_height = FRAME_INNER_HEIGHT (f);
 
@@ -12272,25 +12281,15 @@ resize_mini_window (struct window *w, bool exact_p)
            {
              init_iterator (&it, w, ZV, ZV_BYTE, NULL, DEFAULT_FACE_ID);
              move_it_vertically_backward (&it, height - unit);
-              /* The following move is usually a no-op when the stuff
-                 displayed in the mini-window comes entirely from buffer
-                 text, but it is needed when some of it comes from overlay
-                 strings, especially when there's an after-string at ZV.
-                 This happens with some completion packages, like
-                 icomplete, ido-vertical, etc.  With those packages, if we
-                 don't force w->start to be at the beginning of a screen
-                 line, important parts of the stuff in the mini-window,
-                 such as user prompt, will be hidden from view.  */
-              move_it_by_lines (&it, 0);
-              start = it.current.pos;
               /* Prevent redisplay_window from recentering, and thus from
                  overriding the window-start point we computed here.  */
               w->start_at_line_beg = false;
-              SET_MARKER_FROM_TEXT_POS (w->start, start);
+              SET_MARKER_FROM_TEXT_POS (w->start, it.current.pos);
             }
        }
       else
        {
+         struct text_pos start;
          SET_TEXT_POS (start, BEGV, BEGV_BYTE);
           SET_MARKER_FROM_TEXT_POS (w->start, start);
         }
diff --git a/test/src/xdisp-tests.el b/test/src/xdisp-tests.el
index ae4aacd9c7c..d1d7262665a 100644
--- a/test/src/xdisp-tests.el
+++ b/test/src/xdisp-tests.el
@@ -34,23 +34,19 @@ xdisp-tests--in-minibuffer
 
 (ert-deftest xdisp-tests--minibuffer-resizing () ;; bug#43519
   (should
-   (equal
-    t
-    (xdisp-tests--in-minibuffer
-      (insert "hello")
-      (let ((ol (make-overlay (point) (point)))
-            (max-mini-window-height 1)
-            (text 
"askdjfhaklsjdfhlkasjdfhklasdhflkasdhflkajsdhflkashdfkljahsdlfkjahsdlfkjhasldkfhalskdjfhalskdfhlaksdhfklasdhflkasdhflkasdhflkajsdhklajsdgh"))
-        ;; (save-excursion (insert text))
-        ;; (sit-for 2)
-        ;; (delete-region (point) (point-max))
-        (put-text-property 0 1 'cursor t text)
-        (overlay-put ol 'after-string text)
-        (redisplay 'force)
-        ;; Make sure we do the see "hello" text.
-        (prog1 (equal (window-start) (point-min))
-          ;; (list (window-start) (window-end) (window-width))
-          (delete-overlay ol)))))))
+   (xdisp-tests--in-minibuffer
+    (insert "hello")
+    (let ((ol (make-overlay (point) (point)))
+          (max-mini-window-height 1)
+          (text (let ((s ""))
+                  (dotimes (i 137)
+                    (setq s (concat s (char-to-string (+ (% i 26) ?a)))))
+                  s)))
+      (put-text-property 0 1 'cursor t text)
+      (overlay-put ol 'after-string text)
+      (redisplay)
+      (prog1 (equal (window-start) (point-min))
+        (delete-overlay ol))))))
 
 (ert-deftest xdisp-tests--minibuffer-scroll () ;; bug#44070
   (let ((posns
-- 
2.26.2

>>>>> "EZ" == Eli Zaretskii <eliz@gnu.org> 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 <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.

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?