GNU bug report logs - #71284
30.0.50; [PATCH] Add support for outline-minor-mode to Eshell

Previous Next

To reply to this bug, email your comments to 71284 AT debbugs.gnu.org.

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

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Cc: juri <at> linkov.net
Subject: 30.0.50; [PATCH] Add support for outline-minor-mode to Eshell
Date: Thu, 30 May 2024 22:18:44 -0700

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

This patch adds support for outline-minor-mode to Eshell. When enabled, 
this will just add outline buttons to the prompt (as the top level), and 
one to the start of the output (as level 2).

The Eshell side is hopefully fairly straightforward, but the outline.el 
part probably warrants a close review. The changes in outline.el are all 
there to support outline headings that are just a newline and nothing 
else. This is important for both the output (you might call a command 
that prints a newline as the first character), and the prompt (the first 
character of your prompt could be a newline as a way of separating the 
prompt from the output just above it).[1]

Mainly, the changes in outline.el are about checking to see if the 
character *before* point is invisible (i.e. part of a collapsed node). 
This way, if point is at the beginning of the output, and you collapse 
that node, outline.el still considers you to be on a heading. (Without 
these changes, cycling a node doesn't work correctly.)

I'm open to adding regression tests here since the logic is pretty 
subtle, but I didn't see any existing ones for outline.el and it's not 
immediately obvious the best way to test its behavior. Let me know if 
anyone has ideas though.

[1] In the prompt case, a different way to solve this would be to treat 
the start of the heading as the first non-empty line. That doesn't work 
for output though since *all* lines could be empty.

[0001-Add-support-for-outline-minor-mode-in-Eshell.patch (text/plain, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 71284 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#71284: 30.0.50;
 [PATCH] Add support for outline-minor-mode to Eshell
Date: Fri, 31 May 2024 09:05:30 +0300

> Cc: juri <at> linkov.net
> Date: Thu, 30 May 2024 22:18:44 -0700
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> This patch adds support for outline-minor-mode to Eshell. When enabled, 
> this will just add outline buttons to the prompt (as the top level), and 
> one to the start of the output (as level 2).

Thanks.

> diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
> index 873d14aff32..071a2e59191 100644
> --- a/doc/misc/eshell.texi
> +++ b/doc/misc/eshell.texi
> @@ -2967,12 +2967,6 @@ Bugs and ideas
>  
>  @item Support zsh's ``Parameter Expansion'' syntax, i.e., @samp{$@{@var{name}:-@var{val}@}}
>  
> -@item Create a mode @code{eshell-browse}
> -
> -It would treat the Eshell buffer as an outline.  Collapsing the outline
> -hides all of the output text.  Collapsing again would show only the
> -first command run in each directory
> -
>  @item Allow other revisions of a file to be referenced using @samp{file@{rev@}}
>  
>  This would be expanded by @code{eshell-expand-file-name} (see above).
> diff --git a/etc/NEWS b/etc/NEWS
> index 3c672ffed8f..b5d4c9cfe6e 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -877,6 +877,9 @@ can make it executable like other shell scripts:
>  
>      #!/usr/bin/env -S emacs --batch -f eshell-batch-file
>  
> +---
> +*** Eshell now supports 'outline-minor-mode'.

This is what I call "anti-documentation": the text you removed, which
described a non-existing feature, told more about the feature than the
text you added after implementing it ;-)

Seriously, though: since it is completely un-obvious what would
outline-minor-mode mean for command-line-oriented modes like Eshell,
both NEWS and the Eshell manual should say at least a couple of words
about what that does.  We cannot get away by just mentioning the
support and keeping silent about the rest.

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

From: Juri Linkov <juri <at> linkov.net>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 71284 <at> debbugs.gnu.org
Subject: Re: 30.0.50; [PATCH] Add support for outline-minor-mode to Eshell
Date: Fri, 31 May 2024 09:51:28 +0300

> This patch adds support for outline-minor-mode to Eshell. When enabled,
> this will just add outline buttons to the prompt (as the top level), and
> one to the start of the output (as level 2).

I'm not familiar with Eshell, so I tried in shell-mode to set
(setq-local outline-regexp "^.*\\$ ") and it works nicely
(for the real use it should be based on 'shell-prompt-pattern').

But I believe you had a good reason to prefer outline-search-function
and more changes in outline.el to support outlines in Eshell.

> The Eshell side is hopefully fairly straightforward, but the outline.el
> part probably warrants a close review. The changes in outline.el are all
> there to support outline headings that are just a newline and nothing
> else. This is important for both the output (you might call a command that
> prints a newline as the first character), and the prompt (the first
> character of your prompt could be a newline as a way of separating the
> prompt from the output just above it).[1]

I don't understand why do you need an outline heading for the empty line.
Is it really useful for hide/show an empty line?

> Mainly, the changes in outline.el are about checking to see if the
> character *before* point is invisible (i.e. part of a collapsed node). This
> way, if point is at the beginning of the output, and you collapse that
> node, outline.el still considers you to be on a heading. (Without these
> changes, cycling a node doesn't work correctly.)
>
> I'm open to adding regression tests here since the logic is pretty subtle,
> but I didn't see any existing ones for outline.el and it's not immediately
> obvious the best way to test its behavior. Let me know if anyone has ideas
> though.

Adding regression tests would be nice since the changes in outline.el
don't look trivial.  I guess tests for outline.el should do two basic things:
call various outline commands (perhaps just outline-cycle and outline-cycle-buffer
should be sufficient) with various settings of outline variables,
then 'invisible-p' could check if outline visibility is satisfied.
Or we could prefer a lazy approach to install changes, then to add tests
only in case when a regression will be discovered later.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 71284 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#71284: 30.0.50; [PATCH] Add support for outline-minor-mode to
 Eshell
Date: Fri, 31 May 2024 12:33:32 -0700

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

On 5/30/2024 11:05 PM, Eli Zaretskii wrote:
> Seriously, though: since it is completely un-obvious what would
> outline-minor-mode mean for command-line-oriented modes like Eshell,
> both NEWS and the Eshell manual should say at least a couple of words
> about what that does.  We cannot get away by just mentioning the
> support and keeping silent about the rest.

Good point. I'd tried to write some more docs previously but was very 
unsatisfied with my attempt. After sleeping on it, I've tried again (see 
attached).

I added a preliminary commit here to move some of the existing docs into 
a new "Interaction" section, since I think the manual should really 
split out the documentation of commands and their syntax from how to use 
Eshell *interactively*. (I split this out since the diff is fairly 
large, but the changes aren't all that exciting: it just moves things 
from one spot to another, and adds some new index entries.)

That provides a better spot to explain this new outline-minor-mode 
support. I also filled in documentation for a few of the existing 
navigation commands available in Eshell.

[0001-Add-an-Interaction-chapter-to-the-Eshell-manual.patch (text/plain, attachment)]

[0002-Add-support-for-outline-minor-mode-in-Eshell.patch (text/plain, attachment)]

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 71284 <at> debbugs.gnu.org
Subject: Re: bug#71284: 30.0.50; [PATCH] Add support for outline-minor-mode to
 Eshell
Date: Fri, 31 May 2024 13:02:00 -0700

On 5/30/2024 11:51 PM, Juri Linkov wrote:
>> This patch adds support for outline-minor-mode to Eshell. When enabled,
>> this will just add outline buttons to the prompt (as the top level), and
>> one to the start of the output (as level 2).
> 
> I'm not familiar with Eshell, so I tried in shell-mode to set
> (setq-local outline-regexp "^.*\\$ ") and it works nicely
> (for the real use it should be based on 'shell-prompt-pattern').
> 
> But I believe you had a good reason to prefer outline-search-function
> and more changes in outline.el to support outlines in Eshell.

Yeah, I've been trying to reduce Eshell's reliance on simple regexps 
like that to determine the structure of the content. Since Eshell runs 
entirely within Emacs, we have the knowledge to authoritatively 
determine what parts of the buffer are what (you can do this in Eshell 
by examining the 'field' property). Regexps always run a (small?) risk 
of false-positives, and would additionally require users to update the 
regexp when they update their prompt.

>> The Eshell side is hopefully fairly straightforward, but the outline.el
>> part probably warrants a close review. The changes in outline.el are all
>> there to support outline headings that are just a newline and nothing
>> else. This is important for both the output (you might call a command that
>> prints a newline as the first character), and the prompt (the first
>> character of your prompt could be a newline as a way of separating the
>> prompt from the output just above it).[1]
> 
> I don't understand why do you need an outline heading for the empty line.
> Is it really useful for hide/show an empty line?

The main case I'm trying to account for is collapsing the command 
output. I think the most consistent way for Eshell is to make the 
"heading" always be the first line of the output, even if that line is 
empty. That way, the arrow button is always in the same spot 
(immediately after the prompt/input), and if the output had many leading 
empty lines, you can collapse all of them to save space (to be fair, 
this case probably isn't super-common).

For prompts, this isn't as important, since a single-line prompt should 
always have some visible text. For multi-line prompts, it would be 
possible to treat the heading as the first non-empty line, but that 
would be additional work on the Eshell side, and I think we'd still need 
the outline.el changes to handle collapsing the command output. 
(Improving heading-detection for multi-line prompts could always be done 
in a later bug, too.)

>> I'm open to adding regression tests here since the logic is pretty subtle,
>> but I didn't see any existing ones for outline.el and it's not immediately
>> obvious the best way to test its behavior. Let me know if anyone has ideas
>> though.
> 
> Adding regression tests would be nice since the changes in outline.el
> don't look trivial.  I guess tests for outline.el should do two basic things:
> call various outline commands (perhaps just outline-cycle and outline-cycle-buffer
> should be sufficient) with various settings of outline variables,
> then 'invisible-p' could check if outline visibility is satisfied.
> Or we could prefer a lazy approach to install changes, then to add tests
> only in case when a regression will be discovered later.

Thanks for the ideas. I think it would be good to add some tests here, 
so if you agree with my reasoning for the outline.el changes, I'll start 
writing a few regression tests.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 71284 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#71284: 30.0.50; [PATCH] Add support for outline-minor-mode to
 Eshell
Date: Sat, 01 Jun 2024 08:58:00 +0300

> Date: Fri, 31 May 2024 12:33:32 -0700
> Cc: 71284 <at> debbugs.gnu.org, juri <at> linkov.net
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> On 5/30/2024 11:05 PM, Eli Zaretskii wrote:
> > Seriously, though: since it is completely un-obvious what would
> > outline-minor-mode mean for command-line-oriented modes like Eshell,
> > both NEWS and the Eshell manual should say at least a couple of words
> > about what that does.  We cannot get away by just mentioning the
> > support and keeping silent about the rest.
> 
> Good point. I'd tried to write some more docs previously but was very 
> unsatisfied with my attempt. After sleeping on it, I've tried again (see 
> attached).
> 
> I added a preliminary commit here to move some of the existing docs into 
> a new "Interaction" section, since I think the manual should really 
> split out the documentation of commands and their syntax from how to use 
> Eshell *interactively*. (I split this out since the diff is fairly 
> large, but the changes aren't all that exciting: it just moves things 
> from one spot to another, and adds some new index entries.)
> 
> That provides a better spot to explain this new outline-minor-mode 
> support. I also filled in documentation for a few of the existing 
> navigation commands available in Eshell.

Thanks, LGTM.

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

From: Juri Linkov <juri <at> linkov.net>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 71284 <at> debbugs.gnu.org
Subject: Re: bug#71284: 30.0.50; [PATCH] Add support for outline-minor-mode
 to Eshell
Date: Sun, 02 Jun 2024 09:37:08 +0300

>>> This patch adds support for outline-minor-mode to Eshell. When enabled,
>>> this will just add outline buttons to the prompt (as the top level), and
>>> one to the start of the output (as level 2).
> ...
> The main case I'm trying to account for is collapsing the command
> output. I think the most consistent way for Eshell is to make the "heading"
> always be the first line of the output, even if that line is empty. That
> way, the arrow button is always in the same spot (immediately after the
> prompt/input), and if the output had many leading empty lines, you can
> collapse all of them to save space (to be fair, this case probably isn't
> super-common).

Sorry, I still don't understand why do you need two levels.
Is it because in Eshell prompts are often multi-line?

IIUC, with two levels, the case when the output is empty has such problems
that one line will contain two outline headers, that also means
two conflicting margin arrows on the same line?

> For prompts, this isn't as important, since a single-line prompt should
> always have some visible text. For multi-line prompts, it would be possible
> to treat the heading as the first non-empty line, but that would be
> additional work on the Eshell side, and I think we'd still need the
> outline.el changes to handle collapsing the command output. (Improving
> heading-detection for multi-line prompts could always be done in a later
> bug, too.)

So the outline.el changes are required only to handle empty output lines?
That essentially means adding support for two outline headers
on the same line?

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 71284 <at> debbugs.gnu.org
Subject: Re: bug#71284: 30.0.50; [PATCH] Add support for outline-minor-mode to
 Eshell
Date: Sun, 2 Jun 2024 21:34:14 -0700

On 6/1/2024 11:37 PM, Juri Linkov wrote:
> Sorry, I still don't understand why do you need two levels.
> Is it because in Eshell prompts are often multi-line?

I'm not sure if Eshell prompts are *often* multi-line, but personally I 
use multi-line prompts everywhere I can. Maybe I'm just over-optimizing 
for my own personal preferences here.

> IIUC, with two levels, the case when the output is empty has such problems
> that one line will contain two outline headers, that also means
> two conflicting margin arrows on the same line?

The way I implemented this, this problem wouldn't come up: if the output 
is completely empty, there's no second-level node in the outline for 
that command.

>> For prompts, this isn't as important, since a single-line prompt should
>> always have some visible text. For multi-line prompts, it would be possible
>> to treat the heading as the first non-empty line, but that would be
>> additional work on the Eshell side, and I think we'd still need the
>> outline.el changes to handle collapsing the command output. (Improving
>> heading-detection for multi-line prompts could always be done in a later
>> bug, too.)
> 
> So the outline.el changes are required only to handle empty output lines?
> That essentially means adding support for two outline headers
> on the same line?

To be more precise, the outline.el changes would be required to handle 
the case where a command's output *begins* with one or more newlines. So 
the total output isn't empty, but the first *line* of it is.

In any case, the more I think about this, the more my current patch 
seems like the wrong way to go about this. Even just describing the 
user-facing behavior in all scenarios is pretty complex, so I think it 
might be better to keep it simple and have a single outline level.

That said, for the multi-line prompt case, I wonder if it would make 
sense for outline.el to support multi-line headers. If I could mark the 
entire prompt + command input as a "header", then collapsing it would 
look better: users would still see all of their input in the collapsed 
node. It would look something like so:

  v /home/user/dir
    $ cat some-file.txt
    output
    output
    output

  > /home/user/dir
    $ cat some-file.txt...

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

From: Juri Linkov <juri <at> linkov.net>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 71284 <at> debbugs.gnu.org
Subject: Re: bug#71284: 30.0.50; [PATCH] Add support for outline-minor-mode
 to Eshell
Date: Mon, 03 Jun 2024 09:45:37 +0300

> In any case, the more I think about this, the more my current patch seems
> like the wrong way to go about this. Even just describing the user-facing
> behavior in all scenarios is pretty complex, so I think it might be better
> to keep it simple and have a single outline level.
>
> That said, for the multi-line prompt case, I wonder if it would make sense
> for outline.el to support multi-line headers. If I could mark the entire
> prompt + command input as a "header", then collapsing it would look better:
> users would still see all of their input in the collapsed node.

The multi-line headers have such disadvantage that the outlines
are not compact anymore.  Also multi-line headers might have
more technial issues with displaying an ellipsis at the end.

> It would look something like so:
>
>   v /home/user/dir
>     $ cat some-file.txt
>     output
>     output
>     output
>
>   > /home/user/dir
>     $ cat some-file.txt...

This is a known problem.  Recently I had to add a special handling
in c-ts-mode--outline-predicate to put the outline heading on the
line with the function name instead of the first line with types:

    static void
  v bset_mode_name (struct buffer *b, Lisp_Object val)
    {
      b->mode_name_ = val;
    }

    static void
  v bset_name (struct buffer *b, Lisp_Object val)
    {
      b->name_ = val;
    }

In your case you could do something similar in eshell-outline-search
to put the outline heading on the meaningful line:

    /home/user/dir
  v $ cat some-file.txt
    output
    output
    output

This is not ideal either since the first line belongs to the
previous outline.  This is a known problem too.  For example,
in etc/NEWS, the top +++ is not part of the outline, e.g.

  +++
  *** 'outline-minor-mode' is supported in tree-sitter major modes.
  It can be used in all tree-sitter major modes that set either the
  variable 'treesit-simple-imenu-settings' or 'treesit-outline-predicate'.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 71284 <at> debbugs.gnu.org
Subject: Re: bug#71284: 30.0.50; [PATCH] Add support for outline-minor-mode to
 Eshell
Date: Wed, 5 Jun 2024 18:52:15 -0700

On 6/2/2024 11:45 PM, Juri Linkov wrote:
>> In any case, the more I think about this, the more my current patch seems
>> like the wrong way to go about this. Even just describing the user-facing
>> behavior in all scenarios is pretty complex, so I think it might be better
>> to keep it simple and have a single outline level.
>>
>> That said, for the multi-line prompt case, I wonder if it would make sense
>> for outline.el to support multi-line headers. If I could mark the entire
>> prompt + command input as a "header", then collapsing it would look better:
>> users would still see all of their input in the collapsed node.
> 
> The multi-line headers have such disadvantage that the outlines
> are not compact anymore.  Also multi-line headers might have
> more technial issues with displaying an ellipsis at the end.

Given that you mentioned a few other cases where multi-line headers 
might be nice (assuming the number of lines is small), maybe it would 
make sense to see what an implementation of that looks like. I'll see 
about writing a patch for this.

(Another interesting thing I might try is to see if we could provide 
some custom single-line abbreviation for multi-line headers. That would 
let us have compact headers when they're collapsed, even if the "header" 
part is really multiple lines when expanded.)

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

From: Juri Linkov <juri <at> linkov.net>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 71284 <at> debbugs.gnu.org
Subject: Re: bug#71284: 30.0.50; [PATCH] Add support for outline-minor-mode
 to Eshell
Date: Thu, 06 Jun 2024 09:19:52 +0300

>> The multi-line headers have such disadvantage that the outlines
>> are not compact anymore.  Also multi-line headers might have
>> more technial issues with displaying an ellipsis at the end.
>
> Given that you mentioned a few other cases where multi-line headers might
> be nice (assuming the number of lines is small), maybe it would make sense
> to see what an implementation of that looks like. I'll see about writing
> a patch for this.

It would be nice to solve this long-standing problem.

> (Another interesting thing I might try is to see if we could provide some
> custom single-line abbreviation for multi-line headers. That would let us
> have compact headers when they're collapsed, even if the "header" part is
> really multiple lines when expanded.)

Or maybe showing an outline should also show its pre-heading lines
(maybe this would require adding e.g. outline-pre-header-regexp
that matches the first line of such preface lines).

This would be especially useful in cases when there are dozens of
comment lines before the outline heading with the function name
in programming language modes.

Previous Next

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