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

[Jim Porter <jporterbugs@gmail.com>]

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.