bug#42862: 28.0.50; {previous,next}-line-or-history-element ignores line-move-visual

bug#42862: 28.0.50; {previous,next}-line-or-history-element ignores line-move-visual

[Michael Welsh Duggan]

Run:

    emacs -Q --exec "(setq-default line-move-visual nil)"

Visit a file in a directory where the directory name is not long enough
to wrap in the minibuffer, but the filename is long enough that,
appended to that directory, the minibuffer would wrap.

Example:
/tmp/this-is-a-very-long-directory-name-that-will-not-wrap/but-this-file-name-will-make-it-wrap

Specifically, visit this file such that it will enter the minibuffer
history for file visiting.

Type `C-h f'.  When you hit the up-arrow (not `M-p'!) you should end up
with the very long file name in your minibuffer, with the point at the
end of the first visual line.  This is intended behavior so that
multiple invocations of up-arrow will scroll through the history instead
of moving up a visible line.  (See comments in
`{previous,next}-line-or-history-element' to that effect.)

It is my belief that if `line-move-visual' is nil,
`previous-line-or-history-element' should move to the end of the first
logical line instead of the first visual line.  (Similarly for
`next-line-or-history-element'.)


In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo version 1.16.0)
 of 2020-07-12 built on maru2
Repository revision: 0d5e17b549a223e21ae893c455c0f00753af251c
Repository branch: md5i
Windowing system distributor 'The X.Org Foundation', version 11.0.12008000
System Description: Debian GNU/Linux bullseye/sid

Configured using:
 'configure --with-modules --without-toolkit-scroll-bars
 --with-x-toolkit=lucid --with-wide-int --with-gameuser=:staff
 'CFLAGS=-Og -ggdb3''

Configured features:
XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB NOTIFY
INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF
ZLIB LUCID X11 XDBE XIM MODULES THREADS LIBSYSTEMD JSON PDUMPER LCMS2

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix


-- 
Michael Welsh Duggan
(md5i@md5i.com)

bug#42862: 28.0.50; {previous,next}-line-or-history-element ignores line-move-visual

[Juri Linkov]

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

> Run:
>
>     emacs -Q --exec "(setq-default line-move-visual nil)"
>
> Visit a file in a directory where the directory name is not long enough
> to wrap in the minibuffer, but the filename is long enough that,
> appended to that directory, the minibuffer would wrap.
>
> Example:
> /tmp/this-is-a-very-long-directory-name-that-will-not-wrap/but-this-file-name-will-make-it-wrap
>
> Specifically, visit this file such that it will enter the minibuffer
> history for file visiting.
>
> Type `C-h f'.

I can't reproduce your bug report with `C-h f', but with `C-x C-f' I see
where is the problem.

> It is my belief that if `line-move-visual' is nil,
> `previous-line-or-history-element' should move to the end of the first
> logical line instead of the first visual line.

The patch below should fix this.

> (Similarly for `next-line-or-history-element'.)

Are you sure that `next-line-or-history-element' needs the same fix?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: previous-line-or-history-element.patch --]
[-- Type: text/x-diff, Size: 1981 bytes --]

diff --git a/lisp/simple.el b/lisp/simple.el
index 1cb93c5722..9f3b131a11 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2402,7 +2402,7 @@ previous-line-or-history-element
        (setq temporary-goal-column 0)
        ;; Restore the original goal column on the first line
        ;; of possibly multi-line input.
-       (goto-char (minibuffer-prompt-end))
+       (goto-char (minibuffer-prompt-end)) ; FIXME: maybe remove this line?
        (if old-column
 	   (if (= (line-number-at-pos) 1)
 	       (move-to-column (+ old-column
@@ -2410,15 +2410,17 @@ previous-line-or-history-element
 				    (goto-char (1- (minibuffer-prompt-end)))
 				    (current-column))))
 	     (move-to-column old-column))
-	 ;; Put the cursor at the end of the visual line instead of the
-	 ;; logical line, so the next `previous-line-or-history-element'
-	 ;; would move to the previous history element, not to a possible upper
-	 ;; visual line from the end of logical line in `line-move-visual' mode.
-	 (end-of-visual-line)
-	 ;; Since `end-of-visual-line' puts the cursor at the beginning
-	 ;; of the next visual line, move it one char back to the end
-	 ;; of the first visual line (bug#22544).
-	 (unless (eolp) (backward-char 1)))))))
+	 (if (not line-move-visual)     ; (bug#42862)
+             (goto-char (point-max))
+	   ;; Put the cursor at the end of the visual line instead of the
+	   ;; logical line, so the next `previous-line-or-history-element'
+	   ;; would move to the previous history element, not to a possible upper
+	   ;; visual line from the end of logical line in `line-move-visual' mode.
+	   (end-of-visual-line)
+	   ;; Since `end-of-visual-line' puts the cursor at the beginning
+	   ;; of the next visual line, move it one char back to the end
+	   ;; of the first visual line (bug#22544).
+	   (unless (eolp) (backward-char 1))))))))
 
 (defun next-complete-history-element (n)
   "Get next history element that completes the minibuffer before the point.

bug#42862: 28.0.50; {previous,next}-line-or-history-element ignores line-move-visual

[Michael Welsh Duggan]

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

Tags: patch

Here is a patch I threw together that seems to address this.  (In
testing I found that this did not really affect
`next-line-or-history-element'.)  I marked it as a small change, since
it's only two lines if you don't consider whitespace and comment
re-flowing changes.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Consider-visual-line-mode-when-switching-minibuffer-.patch --]
[-- Type: text/x-diff, Size: 2014 bytes --]

From ca17ee22f54a89cefa26108d5f7b3fbbd431ddd1 Mon Sep 17 00:00:00 2001
From: Michael Welsh Duggan <mwd@md5i.com>
Date: Sat, 15 Aug 2020 23:04:14 -0400
Subject: [PATCH] Consider `visual-line-mode' when switching minibuffer history

Put the cursor at the end of the logical line when `visual-line-mode'
is nil (Bug#42862).
* lisp/simple.el (previous-line-or-history-element): Point movement
behavior depends on `visual-line-mode'.

Copyright-paperwork-exempt: yes
---
 lisp/simple.el | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 1cb93c5722..6689e73cf0 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2410,15 +2410,21 @@ previous-line-or-history-element
 				    (goto-char (1- (minibuffer-prompt-end)))
 				    (current-column))))
 	     (move-to-column old-column))
-	 ;; Put the cursor at the end of the visual line instead of the
-	 ;; logical line, so the next `previous-line-or-history-element'
-	 ;; would move to the previous history element, not to a possible upper
-	 ;; visual line from the end of logical line in `line-move-visual' mode.
+
+         (if visual-line-mode
+             (progn
+               ;; Put the cursor at the end of the visual line instead
+               ;; of the logical line, so the next
+               ;; `previous-line-or-history-element' would move to the
+               ;; previous history element, not to a possible upper
+               ;; visual line from the end of logical line in
+               ;; `line-move-visual' mode.
 	 (end-of-visual-line)
 	 ;; Since `end-of-visual-line' puts the cursor at the beginning
 	 ;; of the next visual line, move it one char back to the end
 	 ;; of the first visual line (bug#22544).
-	 (unless (eolp) (backward-char 1)))))))
+               (unless (eolp) (backward-char 1)))
+           (end-of-line)))))))
 
 (defun next-complete-history-element (n)
   "Get next history element that completes the minibuffer before the point.
-- 
2.28.0


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


-- 
Michael Welsh Duggan
(md5i@md5i.com)

bug#42862: 28.0.50; {previous,next}-line-or-history-element ignores line-move-visual

[Michael Welsh Duggan]

Juri Linkov <juri@linkov.net> writes:

>> Run:
>>
>>     emacs -Q --exec "(setq-default line-move-visual nil)"
>>
>> Visit a file in a directory where the directory name is not long enough
>> to wrap in the minibuffer, but the filename is long enough that,
>> appended to that directory, the minibuffer would wrap.
>>
>> Example:
>> /tmp/this-is-a-very-long-directory-name-that-will-not-wrap/but-this-file-name-will-make-it-wrap
>>
>> Specifically, visit this file such that it will enter the minibuffer
>> history for file visiting.
>>
>> Type `C-h f'.
>
> I can't reproduce your bug report with `C-h f', but with `C-x C-f' I see
> where is the problem.

That was a glitch between my head and my fingers.  (Or maybe in my
head.   The jury is out.)

>> It is my belief that if `line-move-visual' is nil,
>> `previous-line-or-history-element' should move to the end of the first
>> logical line instead of the first visual line.
>
> The patch below should fix this.
>
>> (Similarly for `next-line-or-history-element'.)
>
> Are you sure that `next-line-or-history-element' needs the same fix?
>
> diff --git a/lisp/simple.el b/lisp/simple.el
> index 1cb93c5722..9f3b131a11 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -2402,7 +2402,7 @@ previous-line-or-history-element
>         (setq temporary-goal-column 0)
>         ;; Restore the original goal column on the first line
>         ;; of possibly multi-line input.
> -       (goto-char (minibuffer-prompt-end))
> +       (goto-char (minibuffer-prompt-end)) ; FIXME: maybe remove this line?
>         (if old-column
>  	   (if (= (line-number-at-pos) 1)
>  	       (move-to-column (+ old-column
> @@ -2410,15 +2410,17 @@ previous-line-or-history-element
>  				    (goto-char (1- (minibuffer-prompt-end)))
>  				    (current-column))))
>  	     (move-to-column old-column))
> -	 ;; Put the cursor at the end of the visual line instead of the
> -	 ;; logical line, so the next `previous-line-or-history-element'
> -	 ;; would move to the previous history element, not to a possible upper
> -	 ;; visual line from the end of logical line in `line-move-visual' mode.
> -	 (end-of-visual-line)
> -	 ;; Since `end-of-visual-line' puts the cursor at the beginning
> -	 ;; of the next visual line, move it one char back to the end
> -	 ;; of the first visual line (bug#22544).
> -	 (unless (eolp) (backward-char 1)))))))
> +	 (if (not line-move-visual)     ; (bug#42862)
> +             (goto-char (point-max))

I would recommend (end-of-line) instead, for when the prompt (in
history) is actually a multi-line prompt, otherwise it will fall prey to
the same problem that `end-of-visual-line' is attempting to solve here.

> +	   ;; Put the cursor at the end of the visual line instead of the
> +	   ;; logical line, so the next `previous-line-or-history-element'
> +	   ;; would move to the previous history element, not to a possible upper
> +	   ;; visual line from the end of logical line in `line-move-visual' mode.
> +	   (end-of-visual-line)
> +	   ;; Since `end-of-visual-line' puts the cursor at the beginning
> +	   ;; of the next visual line, move it one char back to the end
> +	   ;; of the first visual line (bug#22544).
> +	   (unless (eolp) (backward-char 1))))))))
>  
>  (defun next-complete-history-element (n)
>    "Get next history element that completes the minibuffer before the point.
>

-- 
Michael Welsh Duggan
(md5i@md5i.com)

bug#42862: 28.0.50; {previous,next}-line-or-history-element ignores line-move-visual

[Michael Welsh Duggan]

Juri Linkov <juri@linkov.net> writes:

> Are you sure that `next-line-or-history-element' needs the same fix?
>

It does not.  I had assumed it would, but experimentation seems to show
that it does not have the same problem.

-- 
Michael Welsh Duggan
(md5i@md5i.com)

bug#42862: 28.0.50; {previous,next}-line-or-history-element ignores line-move-visual

[Juri Linkov]

>> Are you sure that `next-line-or-history-element' needs the same fix?
>
> It does not.  I had assumed it would, but experimentation seems to show
> that it does not have the same problem.

Ah, I see now that you already addressed this point in your later message, sorry.

I see that you use `visual-line-mode' in your patch.  It's still confusing to me
the difference between `visual-line-mode' and `line-move-visual', and which one
should be checked in `previous-line-or-history-element'.  As I understand,
`line-move-visual' is more low-level, right?  Then it could cover more cases.

>> +	 (if (not line-move-visual)     ; (bug#42862)
>> +             (goto-char (point-max))
>
> I would recommend (end-of-line) instead, for when the prompt (in
> history) is actually a multi-line prompt, otherwise it will fall prey to
> the same problem that `end-of-visual-line' is attempting to solve here.

Right, let's use `end-of-line' to move to the end of the first logical line.
Thanks for your help.

bug#42862: 28.0.50; {previous,next}-line-or-history-element ignores line-move-visual

[Michael Welsh Duggan]

Juri Linkov <juri@linkov.net> writes:

>>> Are you sure that `next-line-or-history-element' needs the same fix?
>>
>> It does not.  I had assumed it would, but experimentation seems to show
>> that it does not have the same problem.
>
> Ah, I see now that you already addressed this point in your later
> message, sorry.
>
> I see that you use `visual-line-mode' in your patch.  It's still confusing to me
> the difference between `visual-line-mode' and `line-move-visual', and which one
> should be checked in `previous-line-or-history-element'.  As I understand,
> `line-move-visual' is more low-level, right?  Then it could cover more cases.

I agree.  Using `line-move-visual' is more correct.


>>> +	 (if (not line-move-visual)     ; (bug#42862)
>>> +             (goto-char (point-max))
>>
>> I would recommend (end-of-line) instead, for when the prompt (in
>> history) is actually a multi-line prompt, otherwise it will fall prey to
>> the same problem that `end-of-visual-line' is attempting to solve here.
>
> Right, let's use `end-of-line' to move to the end of the first logical line.
> Thanks for your help.

No problem.

-- 
Michael Welsh Duggan
(md5i@md5i.com)

bug#42862: 28.0.50; {previous,next}-line-or-history-element ignores line-move-visual

[Juri Linkov]

tags 42862 fixed
close 42862 28.0.50
thanks

>>>> +	 (if (not line-move-visual)     ; (bug#42862)
>>>> +             (goto-char (point-max))
>>>
>>> I would recommend (end-of-line) instead, for when the prompt (in
>>> history) is actually a multi-line prompt, otherwise it will fall prey to
>>> the same problem that `end-of-visual-line' is attempting to solve here.
>>
>> Right, let's use `end-of-line' to move to the end of the first logical line.
>> Thanks for your help.
>
> No problem.

Now this is fixed by using end-of-line.