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

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

[dick.r.chiang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-Overlay-strings-at-to_charpos-should-not-increment-v.patch --]
[-- Type: text/x-diff, Size: 6021 bytes --]

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.

* 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             | 28 ++++++++++++----------------
 test/src/xdisp-tests.el | 30 +++++++++++++-----------------
 2 files changed, 25 insertions(+), 33 deletions(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index 0ff6286af74..e5498bbbb1b 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,11 @@ 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
+		       && 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)
@@ -10381,7 +10387,8 @@ move_it_to (struct it *it, ptrdiff_t to_charpos, int to_x, int to_y, int to_vpos
       it->hpos = 0;
       it->line_number_produced_p = false;
       it->current_y += it->max_ascent + it->max_descent;
-      ++it->vpos;
+      if (! reached_continued)
+	++it->vpos;
       last_height = it->max_ascent + it->max_descent;
       it->max_ascent = it->max_descent = 0;
     }
@@ -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);
   /* H is the actual vertical distance from the position in *IT
      and the starting position.  */
   h = it2.current_y - it->current_y;
@@ -12218,7 +12225,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 +12278,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


[-- Attachment #2: Type: text/plain, Size: 8688 bytes --]




In Commercial Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.30, cairo version 1.15.10)
 of 2021-12-04 built on dick
Repository revision: 9d9fe19c6ceb78c6f14d4bfb1fb85d6356b7e0f6
Repository branch: dev
Windowing system distributor 'The X.Org Foundation', version 11.0.11906000
System Description: Ubuntu 18.04.4 LTS

Configured using:
 'configure --prefix=/home/dick/.local --enable-checking
 --with-tree-sitter --enable-dumping-overwrite CC=gcc-10 'CFLAGS=-g3 -Og
 -I/home/dick/.local/include/' LDFLAGS=-L/home/dick/.local/lib
 PKG_CONFIG_PATH=/home/dick/.local/lib/pkgconfig CXX=gcc-10'
Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
TREE-SITTER LCMS2 LIBSELINUX LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG
RSVG SECCOMP SOUND THREADS TIFF TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM
XPM GTK3 ZLIB
Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Magit

Minor modes in effect:
  async-bytecomp-package-mode: t
  global-git-commit-mode: t
  shell-dirtrack-mode: t
  projectile-mode: t
  flx-ido-mode: t
  override-global-mode: t
  global-hl-line-mode: t
  winner-mode: t
  tooltip-mode: t
  show-paren-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
/home/dick/gomacro-mode/gomacro-mode hides /home/dick/.emacs.d/elpa/gomacro-mode-20200326.1103/gomacro-mode
/home/dick/.emacs.d/elpa/hydra-20170924.2259/lv hides /home/dick/.emacs.d/elpa/lv-20191106.1238/lv
/home/dick/.emacs.d/elpa/magit-3.3.0/magit-section-pkg hides /home/dick/.emacs.d/elpa/magit-section-3.3.0/magit-section-pkg
/home/dick/org-gcal.el/org-gcal hides /home/dick/.emacs.d/elpa/org-gcal-0.3/org-gcal
/home/dick/.emacs.d/elpa/tree-sitter-0.15.2/tree-sitter hides /home/dick/.local/share/emacs/28.0.50/lisp/tree-sitter
/home/dick/.emacs.d/lisp/json hides /home/dick/.local/share/emacs/28.0.50/lisp/json
/home/dick/.emacs.d/elpa/transient-0.3.6/transient hides /home/dick/.local/share/emacs/28.0.50/lisp/transient
/home/dick/.emacs.d/elpa/hierarchy-20171221.1151/hierarchy hides /home/dick/.local/share/emacs/28.0.50/lisp/emacs-lisp/hierarchy

Features:
(shadow emacsbug nndoc debbugs-gnu debbugs soap-client rng-xsd rng-dt
rng-util xsd-regexp shortdoc debug backtrace flow-fill ag vc-svn
find-dired tree-sitter-extras pixel-scroll tree-sitter-query scheme
tree-sitter-load tree-sitter-cli tsc tsc-dyn tsc-dyn-get dired-aux
tsc-obsolete cl-print canlock gnus-html help-fns radix-tree sh-script
executable cl shr-color pulse ivy delsel colir ivy-overlay ffap
dumb-jump f magit-extras mule-util face-remap magit-patch-changelog
magit-patch magit-submodule magit-obsolete magit-popup async-bytecomp
async magit-blame magit-stash magit-reflog magit-bisect magit-push
magit-pull magit-fetch magit-clone magit-remote magit-commit
magit-sequence magit-notes magit-worktree magit-tag magit-merge
magit-branch magit-reset magit-files magit-refs magit-status magit
magit-repos magit-apply magit-wip magit-log which-func imenu magit-diff
git-commit log-edit pcvs-util add-log magit-core magit-margin
magit-transient magit-process with-editor server magit-mode transient
tramp-archive tramp-gvfs tramp-cache zeroconf rust-utils rust-mode
rust-rustfmt rust-playpen rust-compile rust-cargo org-element avl-tree
ol-eww eww xdg url-queue ol-rmail ol-mhe ol-irc ol-info ol-gnus nnselect
gnus-search eieio-opt speedbar ezimage dframe ol-docview doc-view
image-mode exif ol-bibtex ol-bbdb ol-w3m ol-doi org-link-doi org-tempo
tempo org org-macro org-footnote org-pcomplete org-list org-faces
org-entities org-version ob-R ob-emacs-lisp ob-ein ein-cell
ein-shared-output ein-output-area ein-kernel ein-ipdb ein-query
ein-events ein-websocket websocket bindat ein-node ewoc ein-log
ein-classes ein-core ein ein-utils deferred ob ob-tangle org-src ob-ref
ob-lob ob-table ob-exp ob-comint ob-core ob-eval org-table oc-basic
bibtex ol org-keys oc org-compat org-macs org-loaddefs find-func
cal-menu calendar cal-loaddefs vc-git vc vc-dispatcher bug-reference
cc-mode cc-fonts cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine
cc-vars cc-defs misearch multi-isearch supercite regi bbdb-message
sendmail footnote qp sort smiley smerge-mode diff diff-mode jka-compr
gnus-async gnus-ml gravatar dns mail-extr gnus-notifications gnus-fun
notifications gnus-kill gnus-dup disp-table utf-7 mm-archive url-cache
nnrss nnfolder nndiscourse benchmark rbenv nnhackernews nntwitter
nntwitter-api bbdb-gnus gnus-demon nntp nnmairix nnml nnreddit
gnus-topic url-http url-auth url-gw network-stream gnutls nsm request
virtualenvwrapper gud s json-rpc python tramp-sh tramp tramp-loaddefs
trampver tramp-integration files-x tramp-compat shell pcomplete ls-lisp
format-spec gnus-score score-mode gnus-bcklg gnus-srvr gnus-cite
anaphora bbdb-mua bbdb-com bbdb bbdb-site timezone gnus-delay gnus-draft
gnus-cache gnus-agent gnus-msg gnus-art mm-uu mml2015 mm-view mml-smime
smime dig gnus-sum shr pixel-fill kinsoku svg dom nndraft nnmh
gnus-group mm-url gnus-undo use-package use-package-delight
use-package-diminish gnus-start gnus-dbus dbus xml gnus-cloud nnimap
nnmail mail-source utf7 netrc nnoo parse-time iso8601 gnus-spec gnus-int
gnus-range message yank-media rmc puny dired-x dired dired-loaddefs
rfc822 mml mml-sec epa epg rfc6068 epg-config mm-decode mm-bodies
mm-encode mailabbrev gmm-utils mailheader gnus-win paredit-ext paredit
subed subed-vtt subed-srt subed-common subed-mpv subed-debug
subed-config inf-ruby ruby-mode smie company pcase
haskell-interactive-mode haskell-presentation-mode haskell-process
haskell-session haskell-compile haskell-mode haskell-cabal haskell-utils
haskell-font-lock haskell-indentation haskell-string
haskell-sort-imports haskell-lexeme haskell-align-imports
haskell-complete-module haskell-ghc-support noutline outline
flymake-proc flymake warnings etags fileloop generator xref project
dabbrev haskell-customize hydra lv use-package-ensure solarized-theme
solarized-definitions projectile lisp-mnt mail-parse rfc2231 ibuf-ext
ibuffer ibuffer-loaddefs thingatpt magit-autorevert autorevert
filenotify magit-git magit-section magit-utils crm dash rx grep compile
comint ansi-color gnus nnheader gnus-util rmail rmail-loaddefs rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils text-property-search
time-date flx-ido flx google-translate-default-ui
google-translate-core-ui facemenu color ido google-translate-core
google-translate-tk google-translate-backend use-package-bind-key
bind-key auto-complete easy-mmode advice edmacro kmacro popup cus-edit
pp cus-load wid-edit emms-player-mplayer emms-player-simple emms
emms-compat cl-extra help-mode use-package-core derived hl-line winner
ring finder-inf json-reformat-autoloads json-snatcher-autoloads
sml-mode-autoloads tornado-template-mode-autoloads info package
browse-url url url-proxy url-privacy url-expand url-methods url-history
url-cookie url-domsuf url-util mailcap url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache json map url-vars seq gv subr-x byte-opt bytecomp
byte-compile cconv cl-loaddefs cl-lib iso-transl tooltip eldoc paren
electric uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tree-sitter tabulated-list replace newcomment
text-mode lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow
isearch easymenu timer select scroll-bar mouse jit-lock font-lock syntax
font-core term/tty-colors frame minibuffer cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite emoji-zwj charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice button loaddefs faces cus-face macroexp files
window text-properties overlay sha1 md5 base64 format env code-pages
mule custom widget keymap hashtable-print-readable backquote threads
dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting cairo move-toolbar gtk x-toolkit x multi-tty
make-network-process emacs)

Memory information:
((conses 16 2879491 657658)
 (symbols 48 58370 84)
 (strings 32 303691 70262)
 (string-bytes 1 11177834)
 (vectors 16 130394)
 (vector-slots 8 3291490 317821)
 (floats 8 3244 4938)
 (intervals 56 456021 5959)
 (buffers 992 71))

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

[Eli Zaretskii]

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

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

[dick]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-Overlay-strings-at-to_charpos-should-not-increment-v.patch --]
[-- Type: text/x-diff, Size: 6294 bytes --]

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


[-- Attachment #2: Type: text/plain, Size: 2084 bytes --]


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

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

[dick]

Just want to get `move_it_to` right on a (MOVE_TO_VPOS | MOVE_TO_POS) to
a line ending in an invisible string.  This is nicely tested in
`xdisp-tests--minibuffer-resizing` in reference to bug#43519, but the
erstwhile fix was a paper-over (calling move_it_vertically_backward with
arg 0 as many times as it took to get back to line start, then adding a
long-winded justification -- no dearth of those).

If you're not interested or not wanting the FUD, I understand.  It's not
a showstopper.

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

[Eli Zaretskii]

> From: dick <dick.r.chiang@gmail.com>
> Cc: 52302@debbugs.gnu.org
> Date: Sun, 05 Dec 2021 14:06:14 -0500
> 
> Just want to get `move_it_to` right on a (MOVE_TO_VPOS | MOVE_TO_POS) to
> a line ending in an invisible string.

Are you using "invisible" here as in "text with the invisible
property"?  Or does "invisible" mean "not shown on display"?  If the
latter, then I don't understand what does visibility have to do with
coordinates maintained by move_it_to -- that function doesn't care
whether the text it traverses is or isn't shown.

> This is nicely tested in
> `xdisp-tests--minibuffer-resizing` in reference to bug#43519, but the
> erstwhile fix was a paper-over (calling move_it_vertically_backward with
> arg 0 as many times as it took to get back to line start, then adding a
> long-winded justification -- no dearth of those).

I see just one call to move_it_vertically_backward, not "as many as".
And the long comment explains why we call move_it_to (also a single
call), not move_it_vertically_backward.  So I don't understand what
you are saying here.

And the overall motivation is also not clear -- is it just to save us
one call to move_it_vertically_backward?

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

[dick]

> Are you using "invisible" here as in "text with the invisible
> property"?

Good question.  I'm also annoyed when comments use "invisible"
interchangeably.  I have come to refer to display prop strings as
"unvisible," so my bad.

> I see just one call to move_it_vertically_backward, not "as many as".

Two:

move_it_vertically (&it, unit - bottom_y); // unit - bottom_y is zero
move_it_by_lines (&it, 0); // that's the second one

> And the overall motivation is also not clear -- is it just to save us
> one call to move_it_vertically_backward?

I want to live in an xdisp world where calling move_it_vertically_backward(0)
once gets me to line start without exception.

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

[dick]

[-- Attachment #1: Type: text/plain, Size: 122 bytes --]

A more dangerous change like the below would break the cycle of fixes
that minimize impact while maximizing obfuscation.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-A-small-change-with-disastrous-potential.patch --]
[-- Type: text/x-diff, Size: 808 bytes --]

From 5f734c1d50953836dda444b8371642c6f20d4065 Mon Sep 17 00:00:00 2001
From: dickmao <dick.r.chiang@gmail.com>
Date: Mon, 6 Dec 2021 14:30:17 -0500
Subject: [PATCH] A small change with disastrous potential

* src/xdisp.c (move_it_in_display_line_to): How this function has
managed to get by without a notion of visibility is a real mystery.
---
 src/xdisp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index 0ff6286af74..1522c6b3193 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -9561,7 +9561,7 @@ #define IT_RESET_X_ASCENT_DESCENT(IT)			\
 
       PRODUCE_GLYPHS (it);
 
-      if (it->area != TEXT_AREA)
+      if (it->area != TEXT_AREA || it->method == GET_FROM_STRING)
 	{
 	  prev_method = it->method;
 	  if (it->method == GET_FROM_BUFFER)
-- 
2.26.2


[-- Attachment #3: Type: text/plain, Size: 83 bytes --]


I'll explore this more deeply in my longlines rewrite.  In the meantime, closing.

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

[Eli Zaretskii]

> From: dick <dick.r.chiang@gmail.com>
> Date: Mon, 06 Dec 2021 14:47:45 -0500
> 
> A more dangerous change like the below would break the cycle of fixes
> that minimize impact while maximizing obfuscation.
> 
> >From 5f734c1d50953836dda444b8371642c6f20d4065 Mon Sep 17 00:00:00 2001
> From: dickmao <dick.r.chiang@gmail.com>
> Date: Mon, 6 Dec 2021 14:30:17 -0500
> Subject: [PATCH] A small change with disastrous potential
> 
> * src/xdisp.c (move_it_in_display_line_to): How this function has
> managed to get by without a notion of visibility is a real mystery.
> ---
>  src/xdisp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/xdisp.c b/src/xdisp.c
> index 0ff6286af74..1522c6b3193 100644
> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -9561,7 +9561,7 @@ #define IT_RESET_X_ASCENT_DESCENT(IT)			\
>  
>        PRODUCE_GLYPHS (it);
>  
> -      if (it->area != TEXT_AREA)
> +      if (it->area != TEXT_AREA || it->method == GET_FROM_STRING)
>  	{
>  	  prev_method = it->method;
>  	  if (it->method == GET_FROM_BUFFER)

I don't understand this change.  Glyphs delivered from display and
overlay strings are subject to wrapping and truncation exactly like
glyphs delivered from buffer text.  Only glyphs that are written into
the display margins are silently truncated.  So I don't think your
additional condition is correct.  For example, what happens if there's
a display or overlay string that is very long, so that it overflows
the window width?  This additional condition will cause the code
behave as if the window had infinite width, for the whole time it
traverses the string.