From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.bugs Subject: bug#44070: 28.0.50; Minibuffer display "jumps" upon minor edit Date: Thu, 29 Oct 2020 13:54:01 -0400 Message-ID: References: <83a6wip43q.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="8993"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) Cc: 44070@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Oct 29 18:55:28 2020 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kYC91-0002En-Ul for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 29 Oct 2020 18:55:28 +0100 Original-Received: from localhost ([::1]:33142 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kYC91-0005Ec-0x for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 29 Oct 2020 13:55:27 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:45860) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kYC8d-0004ym-KF for bug-gnu-emacs@gnu.org; Thu, 29 Oct 2020 13:55:03 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:43355) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kYC8b-000054-Vy for bug-gnu-emacs@gnu.org; Thu, 29 Oct 2020 13:55:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1kYC8b-0002yM-V4 for bug-gnu-emacs@gnu.org; Thu, 29 Oct 2020 13:55:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 29 Oct 2020 17:55:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 44070 X-GNU-PR-Package: emacs Original-Received: via spool by 44070-submit@debbugs.gnu.org id=B44070.160399405211346 (code B ref 44070); Thu, 29 Oct 2020 17:55:01 +0000 Original-Received: (at 44070) by debbugs.gnu.org; 29 Oct 2020 17:54:12 +0000 Original-Received: from localhost ([127.0.0.1]:54896 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kYC7n-0002wu-EA for submit@debbugs.gnu.org; Thu, 29 Oct 2020 13:54:11 -0400 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:22840) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kYC7l-0002wc-T7 for 44070@debbugs.gnu.org; Thu, 29 Oct 2020 13:54:10 -0400 Original-Received: from pmg3.iro.umontreal.ca (localhost [127.0.0.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id 87D004412A7; Thu, 29 Oct 2020 13:54:04 -0400 (EDT) Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id 68D3D4411F0; Thu, 29 Oct 2020 13:54:02 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1603994042; bh=dHYyU/6qaIWj/XNy4k3vpK6PI+0mDYMUEGsPdHldxLw=; h=From:To:Cc:Subject:References:Date:In-Reply-To:From; b=hWq+iOHFreuOdCOEm+64sOCb/Ovbt9+8xzr6OyP4Zx5n/UEqWl8KQhV2nhci9Ryil XK905myrfBwNKJaS0ogarTs1PBtLkFUXZmklN3rP5yXJmT93Q6/rcjQ9gCsqJPWL4r v27p9D4mJSPchxhCavl48wopBSt+AQrSkz558mXqtpJ/T3iZKrvOGT1myKChi+jL/h PJeDynkNbaxd2y6/iu4Vmm79zqo67X5ejSJtq0h10LXoSKjSe4AawGLEm1NR2kwjla ywvSlvC4d1XxrWvCTK+p3UM2v+oO7QkS+hu2ElIywd3ZfPVFQp26HK1cKnBi+xR3n+ 4nmfVDCEr+D+w== Original-Received: from alfajor (unknown [157.52.9.240]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 395311201DA; Thu, 29 Oct 2020 13:54:02 -0400 (EDT) In-Reply-To: <83a6wip43q.fsf@gnu.org> (Eli Zaretskii's message of "Mon, 19 Oct 2020 19:34:01 +0300") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:191980 Archived-At: >> + (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