* bug#52302: 28.0.50; [PATCH] Overlay strings should not increment vpos @ 2021-12-05 17:37 dick.r.chiang 2021-12-05 18:34 ` Eli Zaretskii 2021-12-06 19:47 ` dick 0 siblings, 2 replies; 8+ messages in thread From: dick.r.chiang @ 2021-12-05 17:37 UTC (permalink / raw) To: 52302 [-- 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)) ^ permalink raw reply related [flat|nested] 8+ messages in thread
* bug#52302: 28.0.50; [PATCH] Overlay strings should not increment vpos 2021-12-05 17:37 bug#52302: 28.0.50; [PATCH] Overlay strings should not increment vpos dick.r.chiang @ 2021-12-05 18:34 ` Eli Zaretskii 2021-12-05 18:52 ` dick 2021-12-05 19:06 ` dick 2021-12-06 19:47 ` dick 1 sibling, 2 replies; 8+ messages in thread From: Eli Zaretskii @ 2021-12-05 18:34 UTC (permalink / raw) To: dick.r.chiang; +Cc: 52302 > 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? ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#52302: 28.0.50; [PATCH] Overlay strings should not increment vpos 2021-12-05 18:34 ` Eli Zaretskii @ 2021-12-05 18:52 ` dick 2021-12-05 19:06 ` dick 1 sibling, 0 replies; 8+ messages in thread From: dick @ 2021-12-05 18:52 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 52302 [-- 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? ^ permalink raw reply related [flat|nested] 8+ messages in thread
* bug#52302: 28.0.50; [PATCH] Overlay strings should not increment vpos 2021-12-05 18:34 ` Eli Zaretskii 2021-12-05 18:52 ` dick @ 2021-12-05 19:06 ` dick 2021-12-05 19:47 ` Eli Zaretskii 1 sibling, 1 reply; 8+ messages in thread From: dick @ 2021-12-05 19:06 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 52302 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#52302: 28.0.50; [PATCH] Overlay strings should not increment vpos 2021-12-05 19:06 ` dick @ 2021-12-05 19:47 ` Eli Zaretskii 2021-12-05 22:10 ` dick 0 siblings, 1 reply; 8+ messages in thread From: Eli Zaretskii @ 2021-12-05 19:47 UTC (permalink / raw) To: dick; +Cc: 52302 > 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? ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#52302: 28.0.50; [PATCH] Overlay strings should not increment vpos 2021-12-05 19:47 ` Eli Zaretskii @ 2021-12-05 22:10 ` dick 0 siblings, 0 replies; 8+ messages in thread From: dick @ 2021-12-05 22:10 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 52302 > 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#52302: 28.0.50; [PATCH] Overlay strings should not increment vpos 2021-12-05 17:37 bug#52302: 28.0.50; [PATCH] Overlay strings should not increment vpos dick.r.chiang 2021-12-05 18:34 ` Eli Zaretskii @ 2021-12-06 19:47 ` dick 2021-12-06 20:02 ` Eli Zaretskii 1 sibling, 1 reply; 8+ messages in thread From: dick @ 2021-12-06 19:47 UTC (permalink / raw) To: 52302 [-- 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. ^ permalink raw reply related [flat|nested] 8+ messages in thread
* bug#52302: 28.0.50; [PATCH] Overlay strings should not increment vpos 2021-12-06 19:47 ` dick @ 2021-12-06 20:02 ` Eli Zaretskii 0 siblings, 0 replies; 8+ messages in thread From: Eli Zaretskii @ 2021-12-06 20:02 UTC (permalink / raw) To: dick; +Cc: 52302 > 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-12-06 20:02 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-12-05 17:37 bug#52302: 28.0.50; [PATCH] Overlay strings should not increment vpos dick.r.chiang 2021-12-05 18:34 ` Eli Zaretskii 2021-12-05 18:52 ` dick 2021-12-05 19:06 ` dick 2021-12-05 19:47 ` Eli Zaretskii 2021-12-05 22:10 ` dick 2021-12-06 19:47 ` dick 2021-12-06 20:02 ` Eli Zaretskii
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.