Package: emacs;
Reported by: Stefan Monnier <monnier <at> iro.umontreal.ca>
Date: Sun, 18 Oct 2020 22:11:01 UTC
Severity: normal
Found in version 28.0.50
Done: Stefan Monnier <monnier <at> iro.umontreal.ca>
Bug is archived. No further changes may be made.
Message #11 received at 44070 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 44070 <at> debbugs.gnu.org Subject: Re: bug#44070: 28.0.50; Minibuffer display "jumps" upon minor edit Date: Thu, 29 Oct 2020 13:54:01 -0400
>> + (recenter (if (window-minibuffer-p) -1 -3)))))
> This should have a comment that explains the reason for the
> difference.
Good point. And applies to the other changes as well.
I believe the addition of a config vars takes care of it in the patch below.
> (Btw, does this DTRT when the text in the minibuffer has
> a newline at the end?)
It does, yes.
>> /* Try to scroll by specified few lines. */
>> if ((0 < scroll_conservatively
>> + || MINI_WINDOW_P (w)
>> || 0 < emacs_scroll_step
[...]
>> int ss = try_scrolling (window, just_this_one_p,
>> - scroll_conservatively,
>> + (MINI_WINDOW_P (w)
>> + ? SCROLL_LIMIT + 1
>> + : scroll_conservatively),
>> emacs_scroll_step,
>
> If we want the minibuffer behave as if scroll-conservatively was set,
> why not simply set scroll-conservatively in each minibuffer?
That was my initial thought as well, but when I tried to implement it,
it quickly turned into a scavenge hunt trying to find all the places
where it needs to be set (and re-set after a kill-all-local-variables).
So in the end, the "simply" qualifier didn't apply at all.
Another option I considered was to do it directly inside
`reset_buffer_local_variables`, but then we need to pass the info about
"this is a buffer meant for the mini-windows" through several layers (or
worse: do it based on the buffer's name), which is again unworthy of the
"simply" qualifier.
At this point I stopped and realized that my motivation was to change
the behavior in some particular *windows* rather than in some particular
*buffers*, so I think setting it buffer-locally in those buffers used as
minibuffers, while being a valid option, is not better than the simpler
patch I sent.
> We could then have a user option, by default on, to do that, and let
> users who like the current (mis)behavior continue having that.
We could add such an option, indeed.
> As a nice bonus, we will then be sure the change doesn't affect
> echo-area messages, only editing in the minibuffer.
Indeed, it makes it easier to test whether a change is due to
this modification.
How 'bout the patch below, then?
Stefan
diff --git a/lisp/simple.el b/lisp/simple.el
index 2e40e3261c..8c1761797b 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -1134,7 +1134,11 @@ end-of-buffer
;; If the end of the buffer is not already on the screen,
;; then scroll specially to put it near, but not at, the bottom.
(overlay-recenter (point))
- (recenter -3))))
+ ;; FIXME: Arguably if `scroll-conservatively' is set, then
+ ;; we should always pass -1 to `recenter'.
+ (recenter (if (and minibuffer-scroll-conservatively
+ (window-minibuffer-p))
+ -1 -3)))))
(defcustom delete-active-region t
"Whether single-char deletion commands delete an active region.
diff --git a/src/xdisp.c b/src/xdisp.c
index 5c80e37581..fb8719628b 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -18820,6 +18820,7 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
/* Try to scroll by specified few lines. */
if ((0 < scroll_conservatively
+ || (minibuffer_scroll_conservatively && MINI_WINDOW_P (w))
|| 0 < emacs_scroll_step
|| temp_scroll_step
|| NUMBERP (BVAR (current_buffer, scroll_up_aggressively))
@@ -18830,7 +18831,10 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
/* The function returns -1 if new fonts were loaded, 1 if
successful, 0 if not successful. */
int ss = try_scrolling (window, just_this_one_p,
- scroll_conservatively,
+ ((minibuffer_scroll_conservatively
+ && MINI_WINDOW_P (w))
+ ? SCROLL_LIMIT + 1
+ : scroll_conservatively),
emacs_scroll_step,
temp_scroll_step, last_line_misfit);
switch (ss)
@@ -34538,7 +34542,14 @@ syms_of_xdisp (void)
DEFSYM (Qredisplay_internal_xC_functionx, "redisplay_internal (C function)");
- DEFVAR_BOOL("inhibit-message", inhibit_message,
+ DEFVAR_BOOL ("minibuffer-scroll-conservatively",
+ minibuffer_scroll_conservatively,
+ doc: /* Non-nil means scroll conservatively in minibuffer windows.
+When the value is nil, scrolling in minibuffer windows obeys the
+settings of `scroll-conservatively'. */);
+ minibuffer_scroll_conservatively = true; /* bug#44070 */
+
+ DEFVAR_BOOL ("inhibit-message", inhibit_message,
doc: /* Non-nil means calls to `message' are not displayed.
They are still logged to the *Messages* buffer.
@@ -34546,7 +34557,7 @@ syms_of_xdisp (void)
disable messages everywhere, including in I-search and other
places where they are necessary. This variable is intended to
be let-bound around code that needs to disable messages temporarily. */);
- inhibit_message = 0;
+ inhibit_message = false;
message_dolog_marker1 = Fmake_marker ();
staticpro (&message_dolog_marker1);
diff --git a/test/src/xdisp-tests.el b/test/src/xdisp-tests.el
index 95c39dacc3..fad90fad53 100644
--- a/test/src/xdisp-tests.el
+++ b/test/src/xdisp-tests.el
@@ -21,34 +21,55 @@
(require 'ert)
+(defmacro xdisp-tests--in-minibuffer (&rest body)
+ (declare (debug t) (indent 0))
+ `(catch 'result
+ (minibuffer-with-setup-hook
+ (lambda ()
+ (let ((redisplay-skip-initial-frame nil)
+ (executing-kbd-macro nil)) ;Don't skip redisplay
+ (throw 'result (progn . ,body))))
+ (let ((executing-kbd-macro t)) ;Force real minibuffer in `read-string'.
+ (read-string "toto: ")))))
+
(ert-deftest xdisp-tests--minibuffer-resizing () ;; bug#43519
- ;; FIXME: This test returns success when run in batch but
- ;; it's only a lucky accident: it also returned success
- ;; when bug#43519 was not fixed.
(should
(equal
t
- (catch 'result
- (minibuffer-with-setup-hook
- (lambda ()
- (insert "hello")
- (let ((ol (make-overlay (point) (point)))
- (redisplay-skip-initial-frame nil)
- (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)
- (let ((executing-kbd-macro nil)) ;Don't skip redisplay
- (redisplay 'force))
- (throw 'result
- ;; Make sure we do the see "hello" text.
- (prog1 (equal (window-start) (point-min))
- ;; (list (window-start) (window-end) (window-width))
- (delete-overlay ol)))))
- (let ((executing-kbd-macro t)) ;Force real minibuffer in `read-string'.
- (read-string "toto: ")))))))
+ (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)))))))
+
+(ert-deftest xdisp-tests--minibuffer-scroll () ;; bug#44070
+ (let ((posns
+ (xdisp-tests--in-minibuffer
+ (let ((max-mini-window-height 4))
+ (dotimes (_ 80) (insert "\nhello"))
+ (beginning-of-buffer)
+ (redisplay 'force)
+ (end-of-buffer)
+ ;; A simple edit like removing the last `o' shouldn't cause
+ ;; the rest of the minibuffer's text to move.
+ (list
+ (progn (redisplay 'force) (window-start))
+ (progn (delete-char -1)
+ (redisplay 'force) (window-start))
+ (progn (goto-char (point-min)) (redisplay 'force)
+ (goto-char (point-max)) (redisplay 'force)
+ (window-start)))))))
+ (should (equal (nth 0 posns) (nth 1 posns)))
+ (should (equal (nth 1 posns) (nth 2 posns)))))
;;; xdisp-tests.el ends here
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.