GNU bug report logs - #63778
[PATCH] Use comint-pager in eshell

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 63778 in the body.
You can then email your comments to 63778 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Morgan Smith <Morgan.J.Smith <at> outlook.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Use comint-pager in eshell
Date: Sun, 28 May 2023 18:45:22 -0400

[Message part 1 (text/plain, inline)]

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).

[0001-Use-comint-pager-in-eshell.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

Thanks,

Morgan

Message #8 received at 63778 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Morgan Smith <Morgan.J.Smith <at> outlook.com>, 63778 <at> debbugs.gnu.org
Subject: Re: bug#63778: [PATCH] Use comint-pager in eshell
Date: Sun, 28 May 2023 16:06:18 -0700

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.

Message #11 received at 63778 <at> debbugs.gnu.org (full text, mbox):

From: Morgan Smith <Morgan.J.Smith <at> outlook.com>
To: Jim Porter <jporterbugs <at> gmail.com>, 63778 <at> debbugs.gnu.org
Subject: Re: bug#63778: [PATCH] Use comint-pager in eshell
Date: Sun, 28 May 2023 22:26:14 -0400

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 <at> outlook.com> writes:

> Jim Porter <jporterbugs <at> 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.

Message #14 received at 63778 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Morgan Smith <Morgan.J.Smith <at> outlook.com>, 63778 <at> debbugs.gnu.org
Subject: Re: bug#63778: [PATCH] Use comint-pager in eshell
Date: Sun, 28 May 2023 19:41:40 -0700

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.

Message #17 received at 63778 <at> debbugs.gnu.org (full text, mbox):

From: Morgan Smith <Morgan.J.Smith <at> outlook.com>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 63778 <at> debbugs.gnu.org
Subject: Re: bug#63778: [PATCH] Use comint-pager in eshell
Date: Mon, 29 May 2023 02:23:31 -0400

[Message part 1 (text/plain, inline)]

Jim Porter <jporterbugs <at> 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.

[0001-Fix-infinite-loop-in-eshell-get-set-variable.patch (text/x-patch, attachment)]

[0002-Use-comint-pager-in-eshell.patch (text/x-patch, attachment)]

[Message part 4 (text/plain, inline)]

Thanks,

Morgan

Message #20 received at 63778 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: Morgan.J.Smith <at> outlook.com, 63778 <at> debbugs.gnu.org
Subject: Re: bug#63778: [PATCH] Use comint-pager in eshell
Date: Mon, 29 May 2023 14:46:37 +0300

> Date: Sun, 28 May 2023 16:06:18 -0700
> From: Jim Porter <jporterbugs <at> 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.

Message #23 received at 63778 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Morgan Smith <Morgan.J.Smith <at> outlook.com>
Cc: 63778 <at> debbugs.gnu.org
Subject: Re: bug#63778: [PATCH] Use comint-pager in eshell
Date: Mon, 29 May 2023 22:14:18 -0700

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.

Message #28 received at 63778-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Morgan Smith <Morgan.J.Smith <at> outlook.com>
Cc: 63778-done <at> debbugs.gnu.org
Subject: Re: bug#63778: [PATCH] Use comint-pager in eshell
Date: Wed, 23 Aug 2023 16:58:07 -0700

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).

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.