bug#63778: [PATCH] Use comint-pager in eshell

bug#63778: [PATCH] Use comint-pager in eshell

[Morgan Smith]

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

Hello!

I know many people set PAGER=cat for eshell so I wanted to add support
to eshell for this recent feature that was added (comint-pager).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Use-comint-pager-in-eshell.patch --]
[-- Type: text/x-patch, Size: 888 bytes --]

From 9c22f6cc362d3ddb9fedffe4def4d80ddbe96598 Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Sun, 28 May 2023 18:40:49 -0400
Subject: [PATCH] Use comint-pager in eshell

* lisp/eshell/esh-var.el (eshell-variable-aliases-list): Set $PAGER
environment variable from comint-pager variable
---
 lisp/eshell/esh-var.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index 7dcaff1e24f..fd2f92a0c31 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -162,6 +162,7 @@ if they are quoted with a backslash."
     ("COLUMNS" ,(lambda () (window-body-width nil 'remap)) t t)
     ("LINES" ,(lambda () (window-body-height nil 'remap)) t t)
     ("INSIDE_EMACS" eshell-inside-emacs t)
+    ("PAGER" comint-pager t)
     ("UID" ,(lambda () (file-user-uid)) nil t)
 
     ;; for esh-ext.el
-- 
2.40.1


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



Thanks,

Morgan

bug#63778: [PATCH] Use comint-pager in eshell

[Jim Porter]

On 5/28/2023 3:45 PM, Morgan Smith wrote:
> I know many people set PAGER=cat for eshell so I wanted to add support
> to eshell for this recent feature that was added (comint-pager).

Thanks for the patch. While I'd use this myself, I'm not sure it should 
be the default though. Eshell lets you define visual commands 
('eshell-visual-commands', 'eshell-visual-subcommands') that will open 
up in M-x term. That way, you can use "less" (or whatever your default 
pager is) as usual. If you set PAGER to "cat" or something similar, then 
you likely wouldn't get the benefit of this visual command support.

Maybe we could do something where we set PAGER like in your patch, but 
only if Eshell doesn't think the command is visual? Perhaps you could 
keep the definition of PAGER in esh-var.el as you've done here, but then 
in em-term.el, add an appropriate override for it? That way, a user who 
disables the eshell-term module will always get PAGER=cat. Users who do 
use eshell-term would get a behavior compatible with visual commands.

bug#63778: [PATCH] Use comint-pager in eshell

[Morgan Smith]

This is my very official petition to change gnus defaults.  I have added
this to my config to avoid future mess-ups.

(keymap-set gnus-summary-mode-map "R" #'gnus-summary-very-wide-reply-with-original)

Morgan Smith <Morgan.J.Smith@outlook.com> writes:

> Jim Porter <jporterbugs@gmail.com> writes:
>
>> On 5/28/2023 3:45 PM, Morgan Smith wrote:
>>> I know many people set PAGER=cat for eshell so I wanted to add support
>>> to eshell for this recent feature that was added (comint-pager).
>>
>> Thanks for the patch. While I'd use this myself, I'm not sure it should be the
>> default though. Eshell lets you define visual commands
>> ('eshell-visual-commands', 'eshell-visual-subcommands') that will open up in
>> M-x term. That way, you can use "less" (or whatever your default pager is) as
>> usual. If you set PAGER to "cat" or something similar, then you likely wouldn't
>> get the benefit of this visual command support.
>>
>> Maybe we could do something where we set PAGER like in your patch, but only if
>> Eshell doesn't think the command is visual? Perhaps you could keep the
>> definition of PAGER in esh-var.el as you've done here, but then in em-term.el,
>> add an appropriate override for it? That way, a user who disables the
>> eshell-term module will always get PAGER=cat. Users who do use eshell-term
>> would get a behavior compatible with visual commands.
>>
>
> I haven't the time left in the day to investigate the conflict between
> visual commands and this patch (which I will do) but I would like to
> comment that I believe most users will use one or the other, not both.
> Unless pagers offer features I'm unaware of, I believe it is a better
> user experience to simply dump everything into the current buffer and
> use Emacs as a pager.  For very visual command like pulsemixer that use
> ncurses stuff I don't believe having $PAGER set would really affect
> anything.  So I would like to ask, does anyone actually want to use the
> visual command stuff just for paging stuff?
>
> I would also like to point out that the default value for comint-pager
> is nil so people would have to turn this on manually.  If the usecase
> for comint-pager is significantly different between eshell and other
> comint stuff we could consider a new variable specifically for eshell.
> Personally I don't see a big difference between say ielm and eshell so I
> would like to avoid this but I'll bring it up regardless.

bug#63778: [PATCH] Use comint-pager in eshell

[Jim Porter]

On 5/28/2023 7:26 PM, Morgan Smith wrote:
>> I haven't the time left in the day to investigate the conflict between
>> visual commands and this patch (which I will do) but I would like to
>> comment that I believe most users will use one or the other, not both.
>> Unless pagers offer features I'm unaware of, I believe it is a better
>> user experience to simply dump everything into the current buffer and
>> use Emacs as a pager.  For very visual command like pulsemixer that use
>> ncurses stuff I don't believe having $PAGER set would really affect
>> anything.  So I would like to ask, does anyone actually want to use the
>> visual command stuff just for paging stuff?

I'm not personally a fan of visual commands in Eshell, but the docstring 
for 'eshell-visual-subcommands' suggests adding the various Git 
subcommands that invoke a pager into that list, so it seems that Eshell 
*assumes* people would want this, at least...

>> I would also like to point out that the default value for comint-pager
>> is nil so people would have to turn this on manually.  If the usecase
>> for comint-pager is significantly different between eshell and other
>> comint stuff we could consider a new variable specifically for eshell.
>> Personally I don't see a big difference between say ielm and eshell so I
>> would like to avoid this but I'll bring it up regardless.

Ah, in that case, then I think you'd want to change the logic in 
esh-var.el so that, when 'comint-pager' is nil, $PAGER returns the real 
value of PAGER from the environment. Since this behavior is opt-in, I 
think it would be enough to just make this fix (and ignore the visual 
command stuff), though special handling for visual commands would still 
be nice to have.

bug#63778: [PATCH] Use comint-pager in eshell

[Morgan Smith]

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

Jim Porter <jporterbugs@gmail.com> writes:

>
> Ah, in that case, then I think you'd want to change the logic in esh-var.el so
> that, when 'comint-pager' is nil, $PAGER returns the real value of PAGER from
> the environment. Since this behavior is opt-in, I think it would be enough to
> just make this fix (and ignore the visual command stuff), though special
> handling for visual commands would still be nice to have.
>

I decided to work on this instead of sleeping so I apologize if these
patches are of poor quality.

I still haven't looked into how everything interacts with the visual
commands but it took some real effort (maybe because I'm tired :P) to
get what I got so far and I think it's good enough.

My main pain point was trying to figure out how to maintain the ability
to set/unset the PAGER variable.  These current patches allow you to
set/unset the PAGER variable iff you don't set comint-pager.  It even
allows you to do stuff like 'PAGER=cat git log' (see
eshell-handle-local-variables) without modifying buffer state (as it
should).  Maintaining those capabilities when comint-pager is set seems
very difficult so I gave up.

Trying to setq-local comint-pager in the set function might honestly be
a better user experience for those that set comint-pager but then doing
'PAGER=cat git log' would cause a permanent buffer local change.

So currently everything works 100% great and as expected if comint-pager
is nil.  If comint-pager is not-nil then you cannot set PAGER in a way
that will take any affect.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-infinite-loop-in-eshell-get-set-variable.patch --]
[-- Type: text/x-patch, Size: 1299 bytes --]

From 506849752916ac016457d891f4378d7d2116b0df Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Mon, 29 May 2023 02:04:42 -0400
Subject: [PATCH 1/2] Fix infinite loop in eshell-{get|set}-variable

* lisp/eshell/esh-var.el (eshell-get-variable, eshell-set-variable):
Short circuit if the name and target are the same.
---
 lisp/eshell/esh-var.el | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index 7dcaff1e24f..ecb0dfe274b 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -682,6 +682,8 @@ If QUOTED is non-nil, this was invoked inside double-quotes."
                 (funcall target indices)))))
          ((symbolp target)
           (eshell-apply-indices (symbol-value target) indices quoted))
+         ((and (equal name target))
+          (getenv target))
          (t
           (eshell-get-variable target indices quoted))))
     (unless (stringp name)
@@ -724,6 +726,8 @@ to a Lisp variable)."
          ((and eshell-prefer-lisp-variables
                (stringp target))
           (eshell-set-variable (intern target) value))
+         ((and (equal name target))
+          (setenv target value))
          (t
           (eshell-set-variable target value))))
     (cond
-- 
2.40.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Use-comint-pager-in-eshell.patch --]
[-- Type: text/x-patch, Size: 941 bytes --]

From 01a9f036988bfa3d00b9d837dbacb2666918bc2e Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Mon, 29 May 2023 02:06:48 -0400
Subject: [PATCH 2/2] Use comint-pager in eshell

* lisp/eshell/esh-var.el (eshell-variable-aliases-list): Set $PAGER
environment variable from comint-pager variable
---
 lisp/eshell/esh-var.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index ecb0dfe274b..b036024f065 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -162,6 +162,7 @@ if they are quoted with a backslash."
     ("COLUMNS" ,(lambda () (window-body-width nil 'remap)) t t)
     ("LINES" ,(lambda () (window-body-height nil 'remap)) t t)
     ("INSIDE_EMACS" eshell-inside-emacs t)
+    ("PAGER" (,(lambda () (or comint-pager (getenv "PAGER"))) . "PAGER") t t)
     ("UID" ,(lambda () (file-user-uid)) nil t)
 
     ;; for esh-ext.el
-- 
2.40.1


[-- Attachment #4: Type: text/plain, Size: 17 bytes --]


Thanks,

Morgan

bug#63778: [PATCH] Use comint-pager in eshell

[Eli Zaretskii]

> Date: Sun, 28 May 2023 16:06:18 -0700
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 5/28/2023 3:45 PM, Morgan Smith wrote:
> > I know many people set PAGER=cat for eshell so I wanted to add support
> > to eshell for this recent feature that was added (comint-pager).
> 
> Thanks for the patch. While I'd use this myself, I'm not sure it should 
> be the default though.

Yes, doing this by default would be an incompatible change, so we must
make this optional.

bug#63778: [PATCH] Use comint-pager in eshell

[Jim Porter]

On 5/28/2023 11:23 PM, Morgan Smith wrote:
> My main pain point was trying to figure out how to maintain the ability
> to set/unset the PAGER variable.  These current patches allow you to
> set/unset the PAGER variable iff you don't set comint-pager.  It even
> allows you to do stuff like 'PAGER=cat git log' (see
> eshell-handle-local-variables) without modifying buffer state (as it
> should).  Maintaining those capabilities when comint-pager is set seems
> very difficult so I gave up.

Thanks. I'll think this over for a bit and try some stuff out locally 
too. Looking at your second patch, I think I see where the pain lies: 
when getting PAGER, it always treats 'comint-pager' as taking precedence 
over the real env var, but when setting PAGER, it only sets the env var. 
Therefore, with 'comint-pager' set, setting PAGER won't have the 
intended effect (though maybe this doesn't apply to local variables).

There's probably a nice way to do this, but it might involve some tweaks 
to how Eshell handles variable aliases in general. I'll look into it more.

bug#63778: [PATCH] Use comint-pager in eshell

[Jim Porter]

Version: 30.1

On 5/29/2023 10:14 PM, Jim Porter wrote:
> There's probably a nice way to do this, but it might involve some tweaks 
> to how Eshell handles variable aliases in general. I'll look into it more.

Sorry for completely forgetting about this bug for a few months; I had 
some trouble getting a patch to work, then got busy with other things, 
and never picked it back up.

I've now merged an updated version of this patch as 08901e93797, so 
marking this done.

This implementation should be 100% backwards compatible, since users 
have to opt in to setting the new 'comint-pager' option, and even if you 
do that, it's easy enough to opt back out for just Eshell (set 
'comint-pager' buffer-locally to nil in 'eshell-mode-hook' or similar).