bug#27405: 25.2; Make eshell-next-prompt more reliable

bug#27405: 25.2; Make eshell-next-prompt more reliable

[Pierre Neidhardt]

Current definition of `eshell-next-prompt' merely skips a
paragraph. This won't produce the right result as soon the output
contains a paragraph separator (typically an empty line).

I have been using the following redefinition for a while and it works much
better for me:

	(defun eshell-next-prompt (n)
	  "Move to end of Nth next prompt in the buffer.
	See `eshell-prompt-regexp'."
	  (interactive "p")
	  (re-search-forward eshell-prompt-regexp nil t n)
	  (when eshell-highlight-prompt
	    (while (not (get-text-property (line-beginning-position) 'read-only) )
	      (re-search-forward eshell-prompt-regexp nil t n)))
	  (eshell-skip-prompt))

	(defun eshell-previous-prompt (n)
	  "Move to end of Nth previous prompt in the buffer.
	See `eshell-prompt-regexp'."
	  (interactive "p")
	  (backward-char)
	  (eshell-next-prompt (- n))))

Quite naturally, I search for the `eshell-prompt-regexp'. If that prompt
is too simple (say "^\$ "), some output could easily match the regexp
and the function would move the point there instead of its right
location. To work around that case, I check if the text is read-only,
which is a property of the prompt (if `eshell-highlight-prompt' is `t')
but not of the output.

What do you think? Would you accept a patch?


In GNU Emacs 25.2.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.22.10)
 of 2017-04-22 built on juergen
Windowing system distributor 'The X.Org Foundation', version 11.0.11903000
System Description:	Arch Linux

bug#27405: 25.2; Make eshell-next-prompt more reliable

[npostavs]

severity 27405 minor
tags 27405 + confirmed
quit

Pierre Neidhardt <ambrevar@gmail.com> writes:

> Current definition of `eshell-next-prompt' merely skips a
> paragraph. This won't produce the right result as soon the output
> contains a paragraph separator (typically an empty line).
>
> I have been using the following redefinition for a while and it works much
> better for me:

Seems reasonable.

> 	(defun eshell-next-prompt (n)
> 	  "Move to end of Nth next prompt in the buffer.
> 	See `eshell-prompt-regexp'."
> 	  (interactive "p")
> 	  (re-search-forward eshell-prompt-regexp nil t n)
> 	  (when eshell-highlight-prompt
> 	    (while (not (get-text-property (line-beginning-position) 'read-only) )
> 	      (re-search-forward eshell-prompt-regexp nil t n)))
> 	  (eshell-skip-prompt))
>
> 	(defun eshell-previous-prompt (n)
> 	  "Move to end of Nth previous prompt in the buffer.
> 	See `eshell-prompt-regexp'."
> 	  (interactive "p")
> 	  (backward-char)
> 	  (eshell-next-prompt (- n))))

What's the backward-char for?

bug#27405: 25.2; Make eshell-next-prompt more reliable

[Pierre Neidhardt]

> > 	(defun eshell-previous-prompt (n)
> > 	  "Move to end of Nth previous prompt in the buffer.
> > 	See `eshell-prompt-regexp'."
> > 	  (interactive "p")
> > 	  (backward-char)
> > 	  (eshell-next-prompt (- n))))
>
> What's the backward-char for?

That would have definitely deserved a comment...

Without it, if point is right after the prompt, it won't move. That would be
disturbing for the user.

Detailed rules:

- If point is on the command line _after_ eshell-bol, then go back to eshell-bol.

- If point is between eshell-bol (included) and _after_ eshell-bol of the last
prompt (excluded), go to the last prompt.

--
Pierre Neidhardt

bug#27405: 25.2; Make eshell-next-prompt more reliable

[npostavs]

Pierre Neidhardt <ambrevar@gmail.com> writes:

>> > 	(defun eshell-previous-prompt (n)
>> > 	  "Move to end of Nth previous prompt in the buffer.
>> > 	See `eshell-prompt-regexp'."
>> > 	  (interactive "p")
>> > 	  (backward-char)
>> > 	  (eshell-next-prompt (- n))))
>>
>> What's the backward-char for?
>
> That would have definitely deserved a comment...
>
> Without it, if point is right after the prompt, it won't move. That would be
> disturbing for the user.

Hmm, maybe beginning-of-line would make more sense?  If I have point on
the same line of the prompt I can get to the beginning of the prompted
line with C-a, so C-c C-p probably means I want to go farther back.

bug#27405: 25.2; Make eshell-next-prompt more reliable

[Pierre Neidhardt]

On 17-06-18 09:52:13, npostavs@users.sourceforge.net wrote:
> Pierre Neidhardt <ambrevar@gmail.com> writes:
>
> >> > 	(defun eshell-previous-prompt (n)
> >> > 	  "Move to end of Nth previous prompt in the buffer.
> >> > 	See `eshell-prompt-regexp'."
> >> > 	  (interactive "p")
> >> > 	  (backward-char)
> >> > 	  (eshell-next-prompt (- n))))
> >>
> >> What's the backward-char for?
> >
> > That would have definitely deserved a comment...
> >
> > Without it, if point is right after the prompt, it won't move. That would be
> > disturbing for the user.
>
> Hmm, maybe beginning-of-line would make more sense?  If I have point on
> the same line of the prompt I can get to the beginning of the prompted
> line with C-a, so C-c C-p probably means I want to go farther back.

Agreed. Please make the change and add a comment.

--
Pierre Neidhardt

bug#27405: 25.2; Make eshell-next-prompt more reliable

[npostavs]

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

tags 27405 + patch
quit

Pierre Neidhardt <ambrevar@gmail.com> writes:

>>
>> Hmm, maybe beginning-of-line would make more sense?  If I have point on
>> the same line of the prompt I can get to the beginning of the prompted
>> line with C-a, so C-c C-p probably means I want to go farther back.
>
> Agreed. Please make the change and add a comment.

I think also that doing the text property check following the regexp
search doesn't really make sense, one or the other should be used, not
both:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 1848 bytes --]

From 1dd9644f1322d3b40ba98482d4aca0105db56d81 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Tue, 11 Jul 2017 22:11:19 -0400
Subject: [PATCH v2] Make eshell-next-prompt more reliable (Bug#27405)

* lisp/eshell/em-prompt.el (eshell-next-prompt): Search for
`eshell-prompt-regexp' (and `read-only' text-property if
`eshell-highlight-prompt' is set) rather than trying to use
`forward-paragraph'.
(eshell-previous-prompt): Don't count prompt on current line.
---
 lisp/eshell/em-prompt.el | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/lisp/eshell/em-prompt.el b/lisp/eshell/em-prompt.el
index 8c81b43b1f..2fd1db2113 100644
--- a/lisp/eshell/em-prompt.el
+++ b/lisp/eshell/em-prompt.el
@@ -161,14 +161,25 @@ (defun eshell-next-prompt (n)
   "Move to end of Nth next prompt in the buffer.
 See `eshell-prompt-regexp'."
   (interactive "p")
-  (forward-paragraph n)
+  (if eshell-highlight-prompt
+      (progn
+        (while (< n 0)
+          (while (and (re-search-backward eshell-prompt-regexp nil t)
+                      (not (get-text-property (match-beginning 0) 'read-only))))
+          (setq n (1+ n)))
+        (while (> n 0)
+          (while (and (re-search-forward eshell-prompt-regexp nil t)
+                      (not (get-text-property (match-beginning 0) 'read-only))))
+          (setq n (1- n))))
+    (re-search-forward eshell-prompt-regexp nil t n))
   (eshell-skip-prompt))
 
 (defun eshell-previous-prompt (n)
   "Move to end of Nth previous prompt in the buffer.
 See `eshell-prompt-regexp'."
   (interactive "p")
-  (eshell-next-prompt (- (1+ n))))
+  (beginning-of-line)            ; Don't count prompt on current line.
+  (eshell-next-prompt (- n)))
 
 (defun eshell-skip-prompt ()
   "Skip past the text matching regexp `eshell-prompt-regexp'.
-- 
2.11.1

bug#27405: 25.2; Make eshell-next-prompt more reliable

[Pierre Neidhardt]

Agreed.  If `eshell-highlight-prompt', then search for the first
read-only text, otherwise search for the regexp (less reliable).  No
need to bother with the regexp.  I don't think that anything but the
prompt would be read-only, but I may be wrong.

bug#27405: 25.2; Make eshell-next-prompt more reliable

[Noam Postavsky]

On Wed, Jul 12, 2017 at 9:06 AM, Pierre Neidhardt <ambrevar@gmail.com> wrote:
> Agreed.  If `eshell-highlight-prompt', then search for the first
> read-only text, otherwise search for the regexp (less reliable).  No
> need to bother with the regexp.  I don't think that anything but the
> prompt would be read-only, but I may be wrong.

Nothing else will be read-only by default, but I think it's safer not
to assume things will always be that way.

bug#27405: 25.2; Make eshell-next-prompt more reliable

[npostavs]

tags 27405 fixed
close 27405 26.1
quit

I pushed to master

[1: 7a0ca227af]: 2017-07-20 22:40:34 -0400
  Make eshell-next-prompt more reliable (Bug#27405)
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=7a0ca227af1081ca7ada2e82a87b1a575ef04759>