bug#63748: 30.0.50; eshell-previous-prompt doesn't work for multiline prompts

bug#63748: 30.0.50; eshell-previous-prompt doesn't work for multiline prompts

[Tony Zorman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi,

in commit c257fd3a406d6aa83be60b96217e42b49b62cf5f, the way prompt
navigation in eshell is done was changed; it now works via a text
property search instead of searching for the prompt regexp. When
backwards searching with a prompt that spans multiple lines, however,
one doesn't get off the ground because the current prompt isn't properly
skipped.

More precisely: define a custom prompt

    (defun my/default-prompt-function ()
      (concat "┌─"
              (user-login-name) "@" (system-name)
              " " (abbreviate-file-name (eshell/pwd)) " \n"
              "└─"
              (if (zerop (user-uid)) "#" "$")
              " "))

and enable it by setting

    (setq eshell-prompt-function #'my/default-prompt-function)
    (setq eshell-prompt-regexp "└─[$#] ")  ; For good measure. 

This will result in the point not moving backwards to the last prompt on
C-c C-p (eshell-previous-prompt), but instead it gets "stuck" on the
very left of the line it is on. A solution to this (I think) is to
actually search for the beginning of the current prompt, instead of
using forward-line by itself. I.e., something like

    diff --git a/lisp/eshell/em-prompt.el b/lisp/eshell/em-prompt.el
    index 9f9e58e83d..b6c873b41d 100644
    --- a/lisp/eshell/em-prompt.el
    +++ b/lisp/eshell/em-prompt.el
    @@ -180,7 +180,8 @@ eshell-next-prompt
                       (text-property-search-forward 'field 'prompt t))
             (setq n (1- n)))
         (let (match this-match)
    -      (forward-line 0)           ; Don't count prompt on current line.
    +      ;; Don't count prompt on current line.
    +      (text-property-search-backward 'field 'prompt t)
           (while (and (< n 0)
                       (setq this-match (text-property-search-backward
                                         'field 'prompt t)))

This is a tiny change, so I suppose it can be applied immediately (if I
haven't overlooked anything, of course), or I can prepare a proper patch
(that perhaps adds a test for multiline prompts to the right place).

  Tony

-----------------------------------------------------------------------

In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-musl, cairo version
 1.16.0) of 2023-05-17 built on pbox
Repository revision: 6cb963b73c3768958e13e96b2534d1e99239a3ff
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101008
System Description: Void Linux

Configured using:
 'configure --with-x --without-x-toolkit --without-toolkit-scroll-bars
 --without-dbus --without-gconf --without-gsettings --with-modules
 --with-file-notification=inotify --with-jpeg --with-tiff --with-gif
 --with-png --with-xpm --with-rsvg --without-imagemagick --with-cairo
 --with-gnutls --with-sound --with-json --with-harfbuzz --with-gpm
 --with-native-compilation --without-compress-install --with-xinput2
 --with-small-ja-dic --without-tree-sitter 'CFLAGS=-O2 -pipe
 -march=native -mtune=native -fomit-frame-pointer''

Configured features:
CAIRO FREETYPE GIF GLIB GMP GNUTLS HARFBUZZ JPEG JSON LCMS2 LIBOTF
LIBXML2 M17N_FLT MODULES NATIVE_COMP NOTIFY INOTIFY OLDXMENU PDUMPER PNG
RSVG SECCOMP SOUND SQLITE3 THREADS TIFF WEBP X11 XDBE XIM XINPUT2 XPM
ZLIB

Important settings:
  value of $LC_ALL: en_US.UTF-8
  value of $LC_COLLATE: C
  value of $LANG: en_GB
  locale-coding-system: utf-8-unix

-- 
Tony Zorman | https://tony-zorman.com/

bug#63748: 30.0.50; eshell-previous-prompt doesn't work for multiline prompts

[Jim Porter]

On 5/27/2023 1:38 AM, Tony Zorman via Bug reports for GNU Emacs, the
>      diff --git a/lisp/eshell/em-prompt.el b/lisp/eshell/em-prompt.el
>      index 9f9e58e83d..b6c873b41d 100644
>      --- a/lisp/eshell/em-prompt.el
>      +++ b/lisp/eshell/em-prompt.el
>      @@ -180,7 +180,8 @@ eshell-next-prompt
>                         (text-property-search-forward 'field 'prompt t))
>               (setq n (1- n)))
>           (let (match this-match)
>      -      (forward-line 0)           ; Don't count prompt on current line.
>      +      ;; Don't count prompt on current line.
>      +      (text-property-search-backward 'field 'prompt t)
>             (while (and (< n 0)
>                         (setq this-match (text-property-search-backward
>                                           'field 'prompt t)))
> 
> This is a tiny change, so I suppose it can be applied immediately (if I
> haven't overlooked anything, of course), or I can prepare a proper patch
> (that perhaps adds a test for multiline prompts to the right place).

Thanks for catching this. I think the change you propose makes sense 
(though I haven't actually tried it out).

A test would be great. See "test/lisp/eshell/em-prompt-tests.el": 
another test based on 'em-prompt-test/next-previous-prompt' (and 
possibly 'em-prompt-test/forward-backward-matching-input' too) should be 
sufficient, I think.

If you want to add a test yourself, go for it, and let me know if you 
run into any issues. Otherwise if you prefer, I can write a test case 
for this.

bug#63748: [PATCH] bug#63748: 30.0.50; eshell-previous-prompt doesn't work for multiline prompts

[Tony Zorman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

On Mon, May 29 2023 22:02, Jim Porter wrote:
> A test would be great. See "test/lisp/eshell/em-prompt-tests.el": 
> another test based on 'em-prompt-test/next-previous-prompt' (and 
> possibly 'em-prompt-test/forward-backward-matching-input' too) should be 
> sufficient, I think.
>
> If you want to add a test yourself, go for it, and let me know if you 
> run into any issues. Otherwise if you prefer, I can write a test case 
> for this.

Okay, I now added some hopefully sufficient tests for this based on both
next-previous-prompt and forward-backward-matching-input (if that's not
too overkill). Let me know what you think.


[-- Attachment #2: 0001-eshell-next-prompt-More-precisely-navigate-to-the-pr.patch --]
[-- Type: text/x-patch, Size: 5136 bytes --]

From 45b4e788fb60138516ed8e58a368d72d994da4c0 Mon Sep 17 00:00:00 2001
From: Tony Zorman <soliditsallgood@mailbox.org>
Date: Sat, 3 Jun 2023 14:23:19 +0200
Subject: [PATCH] eshell-next-prompt: More precisely navigate to the prompt
 (bug#63748)

* lisp/eshell/em-prompt.el (eshell-next-prompt): Navigate to the
current prompt more accurately by using text properties instead of
going to the beginning of the line.  This is important for multiline
prompts, as they don't necessarily start at the beginning of the
current line.

* test/lisp/eshell/em-prompt-tests.el
(em-prompt-test/multiline):
Test a given function with a multiline prompt.

(em-prompt-test/next-previous-prompt-with):
(em-prompt-test/forward-backward-matching-input-with):
Helper functions for code reuse.

(em-prompt-test/forward-backward-matching-input):
(em-prompt-test/next-previous-prompt):
Rewrite in terms of the appropriate helper functions.

(em-prompt-test/next-previous-prompt-multiline):
(em-prompt-test/forward-backward-matching-input-multiline):
Add multiline variants of existing tests.
---
 lisp/eshell/em-prompt.el            |  3 +-
 test/lisp/eshell/em-prompt-tests.el | 45 ++++++++++++++++++++++++++---
 2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/lisp/eshell/em-prompt.el b/lisp/eshell/em-prompt.el
index 9f9e58e83d..42f8f273b5 100644
--- a/lisp/eshell/em-prompt.el
+++ b/lisp/eshell/em-prompt.el
@@ -180,7 +180,8 @@ eshell-next-prompt
                   (text-property-search-forward 'field 'prompt t))
         (setq n (1- n)))
     (let (match this-match)
-      (forward-line 0)           ; Don't count prompt on current line.
+      ;; Don't count the current prompt.
+      (text-property-search-backward 'field 'prompt t)
       (while (and (< n 0)
                   (setq this-match (text-property-search-backward
                                     'field 'prompt t)))
diff --git a/test/lisp/eshell/em-prompt-tests.el b/test/lisp/eshell/em-prompt-tests.el
index 257549e40f..7e3454a4d3 100644
--- a/test/lisp/eshell/em-prompt-tests.el
+++ b/test/lisp/eshell/em-prompt-tests.el
@@ -80,9 +80,27 @@ em-prompt-test/field-properties/no-highlight
                 (apply #'propertize "hello\n"
                        eshell-command-output-properties)))))))
 
-(ert-deftest em-prompt-test/next-previous-prompt ()
-  "Check that navigating forward/backward through old prompts works correctly."
+(defun em-prompt-test/multiline (test-fun)
+  "Execute TEST-FUN with a multiline prompt."
+  (funcall
+   test-fun
+   '(progn
+      (setq eshell-prompt-function
+            (lambda ()
+              (concat "┌─"
+                      (user-login-name) "@" (system-name)
+                      " " (abbreviate-file-name (eshell/pwd)) " \n"
+                      "└─"
+                      (if (zerop (user-uid)) "#" "$")
+                      " ")))
+      (setq eshell-prompt-regexp "└─[$#] "))))
+
+(defun em-prompt-test/next-previous-prompt-with (&optional more)
+  "Helper for checking forward/backward navigation of old prompts.
+Accepts an additional argument, to be executed inside of a
+temporary eshell buffer."
   (with-temp-eshell
+   (eval more)
    (eshell-insert-command "echo one")
    (eshell-insert-command "echo two")
    (eshell-insert-command "echo three")
@@ -98,9 +116,20 @@ em-prompt-test/next-previous-prompt
    (eshell-next-prompt 3)
    (should (equal (eshell-get-old-input) "echo fou"))))
 
-(ert-deftest em-prompt-test/forward-backward-matching-input ()
-  "Check that navigating forward/backward via regexps works correctly."
+(ert-deftest em-prompt-test/next-previous-prompt ()
+  "Check that navigating forward/backward through old prompts works correctly."
+  (em-prompt-test/next-previous-prompt-with))
+
+(ert-deftest em-prompt-test/next-previous-prompt-multiline ()
+  "Check old prompt forward/backward navigation for multiline prompts."
+  (em-prompt-test/multiline #'em-prompt-test/next-previous-prompt-with))
+
+(defun em-prompt-test/forward-backward-matching-input-with (&optional more)
+  "Helper for checking forward/backward navigation via regexps.
+Accepts an additional argument, to be executed inside of a
+temporary eshell buffer."
   (with-temp-eshell
+   (eval more)
    (eshell-insert-command "echo one")
    (eshell-insert-command "printnl something else")
    (eshell-insert-command "echo two")
@@ -117,4 +146,12 @@ em-prompt-test/forward-backward-matching-input
    (eshell-forward-matching-input "echo" 3)
    (should (equal (eshell-get-old-input) "echo fou"))))
 
+(ert-deftest em-prompt-test/forward-backward-matching-input ()
+  "Check that navigating forward/backward via regexps works correctly."
+  (em-prompt-test/forward-backward-matching-input-with))
+
+(ert-deftest em-prompt-test/forward-backward-matching-input-multiline ()
+  "Check forward/backward regexp navigation for multiline prompts."
+  (em-prompt-test/multiline #'em-prompt-test/forward-backward-matching-input-with))
+
 ;;; em-prompt-tests.el ends here
-- 
2.40.1


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


-- 
Tony Zorman | https://tony-zorman.com/

bug#63748: [PATCH] bug#63748: 30.0.50; eshell-previous-prompt doesn't work for multiline prompts

[Jim Porter]

On 6/3/2023 6:27 AM, Tony Zorman via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> Okay, I now added some hopefully sufficient tests for this based on both
> next-previous-prompt and forward-backward-matching-input (if that's not
> too overkill). Let me know what you think.

Thanks. You can probably simplify the multiline wrapper in your test to 
something like this:

   (defmacro em-prompt-test--with-multiline (&rest body)
     "Execute BODY with a multiline Eshell prompt."
     `(let ((eshell-prompt-function (lambda () "multiline prompt\n$ ")))
        ,@body))

Then use it like so:

   ;; Note: no arguments necessary.
   (defun em-prompt-test/forward-backward-matching-input-with ()
      ;; ...
      )

   (ert-deftest em-prompt-test/forward-backward-matching-input-multiline ()
     (em-prompt-test--with-multiline
      (em-prompt-test/forward-backward-matching-input-with)))

That should be safer than setq'ing the prompt function. (You also don't 
need to set the prompt regexp, since that's only useful if you want to 
navigate via '(forward|backward)-paragraph', and we don't test that here.)

If you prefer, I can make that change myself and then merge your patch.

bug#63748: [PATCH] bug#63748: 30.0.50; eshell-previous-prompt doesn't work for multiline prompts

[Tony Zorman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

On Sat, Jun 03 2023 12:35, Jim Porter wrote:
> You can probably simplify the multiline wrapper in your test to 
> something like this:
>
>    (defmacro em-prompt-test--with-multiline (&rest body)
>      "Execute BODY with a multiline Eshell prompt."
>      `(let ((eshell-prompt-function (lambda () "multiline prompt\n$ ")))
>         ,@body))
>
> Then use it like so:
>
>    ;; Note: no arguments necessary.
>    (defun em-prompt-test/forward-backward-matching-input-with ()
>       ;; ...
>       )
>
>    (ert-deftest em-prompt-test/forward-backward-matching-input-multiline ()
>      (em-prompt-test--with-multiline
>       (em-prompt-test/forward-backward-matching-input-with)))
>
> That should be safer than setq'ing the prompt function. (You also don't 
> need to set the prompt regexp, since that's only useful if you want to 
> navigate via '(forward|backward)-paragraph', and we don't test that here.)

oh, this is pretty neat, thanks!

Sorry for the slow response; I have attached the (hopefully properly)
corrected patch now.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-eshell-next-prompt-More-precisely-navigate-to-the-pr.patch --]
[-- Type: text/x-patch, Size: 4405 bytes --]

From 37988ca00beb26c0ce22f1c5f0d16cc4df56662c Mon Sep 17 00:00:00 2001
From: Tony Zorman <soliditsallgood@mailbox.org>
Date: Sat, 3 Jun 2023 14:23:19 +0200
Subject: [PATCH] eshell-next-prompt: More precisely navigate to the prompt
 (bug#63748)

* lisp/eshell/em-prompt.el (eshell-next-prompt): Navigate to the
current prompt more accurately by using text properties instead of
going to the beginning of the line.  This is important for multiline
prompts, as they don't necessarily start at the beginning of the
current line.

* test/lisp/eshell/em-prompt-tests.el
(em-prompt-test--with-multiline):
Execute a given body with a multiline prompt.

(em-prompt-test/next-previous-prompt-with):
(em-prompt-test/forward-backward-matching-input-with):
Helper functions for code reuse.

(em-prompt-test/forward-backward-matching-input):
(em-prompt-test/next-previous-prompt):
Rewrite in terms of the appropriate helper functions.

(em-prompt-test/next-previous-prompt-multiline):
(em-prompt-test/forward-backward-matching-input-multiline):
Add multiline variants of existing tests.
---
 lisp/eshell/em-prompt.el            |  3 ++-
 test/lisp/eshell/em-prompt-tests.el | 31 +++++++++++++++++++++++++----
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/lisp/eshell/em-prompt.el b/lisp/eshell/em-prompt.el
index 9f9e58e83d..42f8f273b5 100644
--- a/lisp/eshell/em-prompt.el
+++ b/lisp/eshell/em-prompt.el
@@ -180,7 +180,8 @@ eshell-next-prompt
                   (text-property-search-forward 'field 'prompt t))
         (setq n (1- n)))
     (let (match this-match)
-      (forward-line 0)           ; Don't count prompt on current line.
+      ;; Don't count the current prompt.
+      (text-property-search-backward 'field 'prompt t)
       (while (and (< n 0)
                   (setq this-match (text-property-search-backward
                                     'field 'prompt t)))
diff --git a/test/lisp/eshell/em-prompt-tests.el b/test/lisp/eshell/em-prompt-tests.el
index 257549e40f..93bf9d84ab 100644
--- a/test/lisp/eshell/em-prompt-tests.el
+++ b/test/lisp/eshell/em-prompt-tests.el
@@ -80,8 +80,13 @@ em-prompt-test/field-properties/no-highlight
                 (apply #'propertize "hello\n"
                        eshell-command-output-properties)))))))
 
-(ert-deftest em-prompt-test/next-previous-prompt ()
-  "Check that navigating forward/backward through old prompts works correctly."
+(defmacro em-prompt-test--with-multiline (&rest body)
+  "Execute BODY with a multiline Eshell prompt."
+  `(let ((eshell-prompt-function (lambda () "multiline prompt\n$ ")))
+     ,@body))
+
+(defun em-prompt-test/next-previous-prompt-with ()
+  "Helper for checking forward/backward navigation of old prompts."
   (with-temp-eshell
    (eshell-insert-command "echo one")
    (eshell-insert-command "echo two")
@@ -98,8 +103,17 @@ em-prompt-test/next-previous-prompt
    (eshell-next-prompt 3)
    (should (equal (eshell-get-old-input) "echo fou"))))
 
-(ert-deftest em-prompt-test/forward-backward-matching-input ()
-  "Check that navigating forward/backward via regexps works correctly."
+(ert-deftest em-prompt-test/next-previous-prompt ()
+  "Check that navigating forward/backward through old prompts works correctly."
+  (em-prompt-test/next-previous-prompt-with))
+
+(ert-deftest em-prompt-test/next-previous-prompt-multiline ()
+  "Check old prompt forward/backward navigation for multiline prompts."
+  (em-prompt-test--with-multiline
+   (em-prompt-test/next-previous-prompt-with)))
+
+(defun em-prompt-test/forward-backward-matching-input-with ()
+  "Helper for checking forward/backward navigation via regexps."
   (with-temp-eshell
    (eshell-insert-command "echo one")
    (eshell-insert-command "printnl something else")
@@ -117,4 +131,13 @@ em-prompt-test/forward-backward-matching-input
    (eshell-forward-matching-input "echo" 3)
    (should (equal (eshell-get-old-input) "echo fou"))))
 
+(ert-deftest em-prompt-test/forward-backward-matching-input ()
+  "Check that navigating forward/backward via regexps works correctly."
+  (em-prompt-test/forward-backward-matching-input-with))
+
+(ert-deftest em-prompt-test/forward-backward-matching-input-multiline ()
+  "Check forward/backward regexp navigation for multiline prompts."
+  (em-prompt-test--with-multiline
+   (em-prompt-test/forward-backward-matching-input-with)))
+
 ;;; em-prompt-tests.el ends here
-- 
2.41.0


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


-- 
Tony Zorman | https://tony-zorman.com/

bug#63748: [PATCH] bug#63748: 30.0.50; eshell-previous-prompt doesn't work for multiline prompts

[Jim Porter]

On 6/8/2023 8:11 AM, Tony Zorman via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> oh, this is pretty neat, thanks!
> 
> Sorry for the slow response; I have attached the (hopefully properly)
> corrected patch now.

Thanks (and sorry for the slow response on my end too). This all looks 
good, so I've now merged it to master as f2aae8b879b. Closing this now.