bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|

bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Stefan Kangas
forcemerge 41130 41198
thanks

Yuan Fu <[hidden email]> writes:

> Add two commands that cycles a heading (like that in Org mode) in outline-mode:
>
> - outline-cycle: cycles between “hide all”, “sub headings” and “show all” state. They are called “FOLDED”, “CHILDREN”, “SUBTREE” in Org mode.
> - outline-cycle-buffer: cycles between “only top level headings”, “all headings”, “show all” states

Thanks for working on this.  I've tested your patch, and it seems to
work as advertised.

> Could this be useful?

I think it could.  I have previously sent the wishlist Bug#41130, which
I have now merged with this bug.  I have seen no objections to that
proposal, so I hope that it is uncontroversial.

In Bug#41130, I also suggest to add the same keybinding as in org-mode:
TAB and S-TAB.  Could you add such keybindings to outline-mode-map in
your patch?

I think we also need ChangeLog in the commit message, an entry in NEWS,
and updates to the manual.

> +(defun outline-cycle ()
> +  "Cycle between “hide all”, “headings only” and “show all”.
> +
> +“Hide all” means hide all subheadings and their bodies.
> +“Headings only” means show sub headings but not their bodies.
> +“Show all” means show all subheadings and their bodies."

I can't remember seeing double quotes used like that before in doc
strings.  Correct me if I'm wrong, but wouldn't we normally use
`single-quotes' for something like this?

Best regards,
Stefan Kangas



Reply | Threaded
Open this post in threaded view
|

bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Yuan Fu-2


> On May 18, 2020, at 10:45 PM, Stefan Kangas <[hidden email]> wrote:
>
> forcemerge 41130 41198
> thanks
>
> Yuan Fu <[hidden email]> writes:
>
>> Add two commands that cycles a heading (like that in Org mode) in outline-mode:
>>
>> - outline-cycle: cycles between “hide all”, “sub headings” and “show all” state. They are called “FOLDED”, “CHILDREN”, “SUBTREE” in Org mode.
>> - outline-cycle-buffer: cycles between “only top level headings”, “all headings”, “show all” states
>
> Thanks for working on this.  I've tested your patch, and it seems to
> work as advertised.
Thanks.

>
>> Could this be useful?
>
> I think it could.  I have previously sent the wishlist Bug#41130, which
> I have now merged with this bug.  I have seen no objections to that
> proposal, so I hope that it is uncontroversial.
>
> In Bug#41130, I also suggest to add the same keybinding as in org-mode:
> TAB and S-TAB.  Could you add such keybindings to outline-mode-map in
> your patch?
It seems that the outline-mode bindings all live under C-c. Maybe C-c TAB and C-c S-TAB? Should I ask on emacs-devel for for suggestions?

>
> I think we also need ChangeLog in the commit message, an entry in NEWS,
> and updates to the manual.

I updated NEWS and the manual. For ChangeLog, I thought that’s automatically generated from commit messages?

>
>> +(defun outline-cycle ()
>> +  "Cycle between “hide all”, “headings only” and “show all”.
>> +
>> +“Hide all” means hide all subheadings and their bodies.
>> +“Headings only” means show sub headings but not their bodies.
>> +“Show all” means show all subheadings and their bodies."
>
> I can't remember seeing double quotes used like that before in doc
> strings.  Correct me if I'm wrong, but wouldn't we normally use
> `single-quotes' for something like this?
>
Done.

Yuan


outline.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Yuan Fu-2


> On May 19, 2020, at 6:36 PM, Stefan Kangas <[hidden email]> wrote:
>
> Yuan Fu <[hidden email]> writes:
>
>>> In Bug#41130, I also suggest to add the same keybinding as in org-mode:
>>> TAB and S-TAB.  Could you add such keybindings to outline-mode-map in
>>> your patch?
>>
>> It seems that the outline-mode bindings all live under C-c. Maybe C-c
>> TAB and C-c S-TAB? Should I ask on emacs-devel for for suggestions?
>
> I think it is better to use the same keys as org-mode, since one
> important motivation for this feature is precisely to align the
> outline-mode keybindings with org-mode.  This aspect would be
> significantly diminished by using other keys, IMHO.
>
> My impression was that also there was consensus around (or at least no
> objections to) that idea the last time we discussed it.  I would
> therefore propose to just add TAB and S-TAB to the patch, but allow a
> week or two before pushing to give others a chance to comment here.
>
> But, if you really want to, of course you could also ask on emacs-devel.
> Worst case, we get to have another round of bikeshedding. ;-)
I added TAB and S-TAB bindings. Though S-TAB appears to be <backtab> on my machine (Mac) for both terminal and GUI so I bound that instead. Is that so for other machines? To reduce the possible allergy, I made the TAB binding conditional—only invokes outline-cycle when point is on a heading.

A minor thing: if the motivation is to align with Org mode, should I change the text message to align with Org mode? It prints “show all”, “hide all”, etc (because they make sense), but Org mode prints stuff like “CHILDREN”, “SUBTREE”, etc.

>
>>> I think we also need ChangeLog in the commit message, an entry in NEWS,
>>> and updates to the manual.
>>
>> I updated NEWS and the manual. For ChangeLog, I thought that’s
>> automatically generated from commit messages?
>
> That is my understanding too.
>
> But I only saw a diff attached, not a patch with a commit message.  That
> is what I tried to say, but I could've been more clear.
>
> (CONTRIBUTE suggests using `git format-patch -1' to email a patch, which
> includes the commit message.)
Ah sorry, I switched to a new way to generate patches and didn’t notice that the commit message is gone. It is included this time.

>
>> +@findex outline-cycle
>> +@findex outline-cycle-buffer
>> +  Outline also provides two convenience commands to cycle the
>> +visibility of each heading and the whole buffer.  @code{outline-cycle}
>> +cycles the current heading between "hide all", "subheadings", and
>> +"show all" state.  @code{outline-cycle-buffer} cycles the whole buffer
>> +between "only top-level headings", "all headings and subheadings", and
>> +"show all" states.
> [...]
>> +*** New commands to cycle heading visibility.
>> +'outline-cycle' cycles the current heading between "hide all",
>> +"subheadings", and "show all" state. 'outline-cycle-buffer' cycles the
>> +whole buffer between "only top-level headings", "all headings and
>> +subheadings", and "show all" states.
>
> Looks good to me (but should refer to the above keybindings if they're
> added).
Yuan




outline.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Howard Melman
Yuan Fu <[hidden email]> writes:

>> On May 19, 2020, at 6:36 PM, Stefan Kangas <[hidden email]> wrote:
>>
>> Yuan Fu <[hidden email]> writes:
>>
>>>> In Bug#41130, I also suggest to add the same keybinding as in org-mode:
>>>> TAB and S-TAB.  Could you add such keybindings to outline-mode-map in
>>>> your patch?
>>>
>>> It seems that the outline-mode bindings all live under C-c. Maybe C-c
>>> TAB and C-c S-TAB? Should I ask on emacs-devel for for suggestions?
>>
>> I think it is better to use the same keys as org-mode, since one
>> important motivation for this feature is precisely to align the
>> outline-mode keybindings with org-mode.  This aspect would be
>> significantly diminished by using other keys, IMHO.
>>
>> My impression was that also there was consensus around (or at least no
>> objections to) that idea the last time we discussed it.  I would
>> therefore propose to just add TAB and S-TAB to the patch, but allow a
>> week or two before pushing to give others a chance to comment here.

My original motivation was just to use org's cycling concept
to avoid having to remember all of outlines various commands
and bindings. So regardless of where these commands end up,
I think just having them is a big win. Of course if they
exist, aligning them with org bindings would be very nice.

The idea was to use this for code folding in programming
modes. S-TAB doesn't typically have a binding so it's safe
to use and aligns with org-mode.  

TAB is more problematic as it's typically bound to indenting
commands. I don't think context awareness would solve the
issue as you could want to indent or cycle when point is on
a "heading".  But if the other bindings (I think from
bug#41198) for M-right and M-left were included they could
substitute for indenting.

So in a programming mode with outline-minor-mode enabled:
- S-TAB can be used anywhere for global cycling
- TAB on a heading is used for cycling
- TAB not on a heading is used to indent
- M-right and M-left anywhere can be used to indent/unindent

I think this aligns with org-mode ('m not an org mode
user). If someone is bothered by having to use M-arrows for
indentation in this case, the answer is to not use
outline-minor-mode in a programming mode or rebind outline-cycle.

> A minor thing: if the motivation is to align with Org mode, should I
> change the text message to align with Org mode? It prints “show all”,
> “hide all”, etc (because they make sense), but Org mode prints stuff
> like “CHILDREN”, “SUBTREE”, etc.

IMHO consistency with outline terms is more important
here. It would be helpful if outline and org agreed on
terms. If they don't, then perhaps this could use both with
org terms in parenthesis.

--

Howard




Reply | Threaded
Open this post in threaded view
|

bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Stefan Kangas-2
Howard Melman <[hidden email]> writes:

> So in a programming mode with outline-minor-mode enabled:
> - S-TAB can be used anywhere for global cycling
> - TAB on a heading is used for cycling
> - TAB not on a heading is used to indent
> - M-right and M-left anywhere can be used to indent/unindent

Thanks.

I think what you say makes sense for outline-minor-mode.  We should take
care to distinguish that case from outline-mode, however.

>> A minor thing: if the motivation is to align with Org mode, should I
>> change the text message to align with Org mode? It prints “show all”,
>> “hide all”, etc (because they make sense), but Org mode prints stuff
>> like “CHILDREN”, “SUBTREE”, etc.
>
> IMHO consistency with outline terms is more important
> here. It would be helpful if outline and org agreed on
> terms. If they don't, then perhaps this could use both with
> org terms in parenthesis.

I think outline-mode should also be internally consistent with itself.
So I would suggest to stick to whatever terminology outline-mode uses
elsewhere also for these commands.

Maybe this suggests that the terminology in org-mode and outline-mode
could be aligned also on this point?  But that seems to be a different
issue than the one at hand.

Best regards,
Stefan Kangas



Reply | Threaded
Open this post in threaded view
|

bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Lars Ingebrigtsen
In reply to this post by Yuan Fu-2
Yuan Fu <[hidden email]> writes:

> Ah sorry, I switched to a new way to generate patches and didn’t
> notice that the commit message is gone. It is included this time.

Skimming this thread, it seemed that everybody was in agreement that
this was a good change, but the patch was never applied, so I did that
now.

Some discussion then followed about what key(s) would be the best ones
here, and I didn't see any consensus, so I left the patch as is.  Feel
free to change the keys as you want, but I'm closing this bug report.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#41198: bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Yuan Fu-2


> On Oct 12, 2020, at 11:16 PM, Lars Ingebrigtsen <[hidden email]> wrote:
>
> Yuan Fu <[hidden email]> writes:
>
>> Ah sorry, I switched to a new way to generate patches and didn’t
>> notice that the commit message is gone. It is included this time.
>
> Skimming this thread, it seemed that everybody was in agreement that
> this was a good change, but the patch was never applied, so I did that
> now.
>
> Some discussion then followed about what key(s) would be the best ones
> here, and I didn't see any consensus, so I left the patch as is.  Feel
> free to change the keys as you want, but I'm closing this bug report.
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>   bloggy blog: http://lars.ingebrigtsen.no

Thanks Lars :-)


Reply | Threaded
Open this post in threaded view
|

bug#41198: bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Juri Linkov-2
In reply to this post by Lars Ingebrigtsen
> Skimming this thread, it seemed that everybody was in agreement that
> this was a good change, but the patch was never applied, so I did that
> now.

Now finally org keys are available in etc/NEWS, nice!
But typing S-TAB at the beginning of etc/NEWS signals the error:

  Debugger entered--Lisp error: (error "Before first heading")
    signal(error ("Before first heading"))

This is not how org-mode works - S-TAB doesn't fail before first heading
in org-mode.



Reply | Threaded
Open this post in threaded view
|

bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Lars Ingebrigtsen
Juri Linkov <[hidden email]> writes:

>> Skimming this thread, it seemed that everybody was in agreement that
>> this was a good change, but the patch was never applied, so I did that
>> now.
>
> Now finally org keys are available in etc/NEWS, nice!
> But typing S-TAB at the beginning of etc/NEWS signals the error:
>
>   Debugger entered--Lisp error: (error "Before first heading")
>     signal(error ("Before first heading"))
>
> This is not how org-mode works - S-TAB doesn't fail before first heading
> in org-mode.

How does it work in org-mode?

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Robert Pluim
>>>>> On Thu, 15 Oct 2020 09:02:25 +0200, Lars Ingebrigtsen <[hidden email]> said:

    Lars> Juri Linkov <[hidden email]> writes:
    >>> Skimming this thread, it seemed that everybody was in agreement that
    >>> this was a good change, but the patch was never applied, so I did that
    >>> now.
    >>
    >> Now finally org keys are available in etc/NEWS, nice!
    >> But typing S-TAB at the beginning of etc/NEWS signals the error:
    >>
    >> Debugger entered--Lisp error: (error "Before first heading")
    >> signal(error ("Before first heading"))
    >>
    >> This is not how org-mode works - S-TAB doesn't fail before first heading
    >> in org-mode.

    Lars> How does it work in org-mode?

The same as anywhere else: it cycles the visibility of the headings downbuffer.

Robert
--



Reply | Threaded
Open this post in threaded view
|

bug#41198: bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Yuan Fu-2
In reply to this post by Juri Linkov-2


> On Oct 14, 2020, at 3:24 PM, Juri Linkov <[hidden email]> wrote:
>
>> Skimming this thread, it seemed that everybody was in agreement that
>> this was a good change, but the patch was never applied, so I did that
>> now.
>
> Now finally org keys are available in etc/NEWS, nice!
> But typing S-TAB at the beginning of etc/NEWS signals the error:
>
>  Debugger entered--Lisp error: (error "Before first heading")
>    signal(error ("Before first heading"))
>
> This is not how org-mode works - S-TAB doesn't fail before first heading
> in org-mode.

Do you suggest to change it to a user-error? Or just be silent?

Yuan



Reply | Threaded
Open this post in threaded view
|

bug#41198: bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Yuan Fu-2


> On Oct 15, 2020, at 7:33 PM, Yuan Fu <[hidden email]> wrote:
>
>
>
>> On Oct 14, 2020, at 3:24 PM, Juri Linkov <[hidden email]> wrote:
>>
>>> Skimming this thread, it seemed that everybody was in agreement that
>>> this was a good change, but the patch was never applied, so I did that
>>> now.
>>
>> Now finally org keys are available in etc/NEWS, nice!
>> But typing S-TAB at the beginning of etc/NEWS signals the error:
>>
>> Debugger entered--Lisp error: (error "Before first heading")
>>   signal(error ("Before first heading"))
>>
>> This is not how org-mode works - S-TAB doesn't fail before first heading
>> in org-mode.
>
> Do you suggest to change it to a user-error? Or just be silent?
>
This patch should suppresses errors in both commands. Let me use it for a while and see if it works ok.

Yuan



outline-fix.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#41198: bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Lars Ingebrigtsen
In reply to this post by Yuan Fu-2
Yuan Fu <[hidden email]> writes:

> Do you suggest to change it to a user-error? Or just be silent?

Robert suggested:

>     Lars> How does it work in org-mode?
>
> The same as anywhere else: it cycles the visibility of the headings
> downbuffer.

I guess that makes sense?  I don't use outlining, though, so I don't
really know whether that would be surprising behaviour in outline mode,
though.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#41198: bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Robert Pluim
>>>>> On Fri, 16 Oct 2020 06:59:37 +0200, Lars Ingebrigtsen <[hidden email]> said:

    Lars> Yuan Fu <[hidden email]> writes:
    >> Do you suggest to change it to a user-error? Or just be silent?

    Lars> Robert suggested:

    Lars> How does it work in org-mode?
    >>
    >> The same as anywhere else: it cycles the visibility of the headings
    >> downbuffer.

    Lars> I guess that makes sense?  I don't use outlining, though, so I don't
    Lars> really know whether that would be surprising behaviour in outline mode,
    Lars> though.

I donʼt think Iʼd find it surprising. Of course we could fix this by
just adding "* " to the first line of NEWS :-)

Robert
--



Reply | Threaded
Open this post in threaded view
|

bug#41198: bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Yuan Fu-2
In reply to this post by Yuan Fu-2


> On Oct 16, 2020, at 4:20 AM, Juri Linkov <[hidden email]> wrote:
>
>>> Now finally org keys are available in etc/NEWS, nice!
>>> But typing S-TAB at the beginning of etc/NEWS signals the error:
>>>
>>> Debugger entered--Lisp error: (error "Before first heading")
>>>   signal(error ("Before first heading"))
>>>
>>> This is not how org-mode works - S-TAB doesn't fail before first heading
>>> in org-mode.
>>
>> Do you suggest to change it to a user-error? Or just be silent?
>
> To imitate org-mode, maybe outline-mode could temporarily (i.e. by
> using save-excursion) navigate to the first heading before running
> the rest of outline-cycle-buffer when it's before first heading?

This patch should make outline behaves like org: S-TAB always cycle the whole buffer, regardless where is the point. TAB cycles a heading, and does nothing if point is before the first heading.

Yuan


outline-fix.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Lars Ingebrigtsen
Yuan Fu <[hidden email]> writes:

> This patch should make outline behaves like org: S-TAB always cycle
> the whole buffer, regardless where is the point. TAB cycles a heading,
> and does nothing if point is before the first heading.

I think the behaviour makes sense, but the implementation isn't ideal:

[...]

> +  (condition-case nil
> +      (pcase (outline--cycle-state)
> +        ('hide-all
> +         (if (outline-has-subheading-p)
> +             (progn (outline-show-children)
> +                    (message "Only headings"))
> +           (outline-show-subtree)
> +           (message "Show all")))
> +        ('headings-only
> +         (outline-show-subtree)
> +         (message "Show all"))
> +        ('show-all
> +         (outline-hide-subtree)
> +         (message "Hide all")))
> +    ;; If error: "Before first heading" occurs, ignore it.
> +    (error nil)))

This is basically an `ignore-errors' around a whole bunch of code, used
as a program flow mechanism, and that's always awkward, because it hides
real errors in the code.

Altering the functions to not error out in these situations would be
better.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Juri Linkov-2
>> +  (condition-case nil
>> +      (pcase (outline--cycle-state)
>> +        ('hide-all
>> +         (if (outline-has-subheading-p)
>> +             (progn (outline-show-children)
>> +                    (message "Only headings"))
>> +           (outline-show-subtree)
>> +           (message "Show all")))
>> +        ('headings-only
>> +         (outline-show-subtree)
>> +         (message "Show all"))
>> +        ('show-all
>> +         (outline-hide-subtree)
>> +         (message "Hide all")))
>> +    ;; If error: "Before first heading" occurs, ignore it.
>> +    (error nil)))
>
> This is basically an `ignore-errors' around a whole bunch of code, used
> as a program flow mechanism, and that's always awkward, because it hides
> real errors in the code.
>
> Altering the functions to not error out in these situations would be
> better.

Like 'outline-back-to-heading' has an optional argument 'invisible-ok',
maybe a new argument named 'error-ok' or 'outside-ok' could
be added to not error out when point is outside of the outline tree.



Reply | Threaded
Open this post in threaded view
|

bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Yuan Fu-2


> On Oct 17, 2020, at 4:30 PM, Juri Linkov <[hidden email]> wrote:
>
>>> +  (condition-case nil
>>> +      (pcase (outline--cycle-state)
>>> +        ('hide-all
>>> +         (if (outline-has-subheading-p)
>>> +             (progn (outline-show-children)
>>> +                    (message "Only headings"))
>>> +           (outline-show-subtree)
>>> +           (message "Show all")))
>>> +        ('headings-only
>>> +         (outline-show-subtree)
>>> +         (message "Show all"))
>>> +        ('show-all
>>> +         (outline-hide-subtree)
>>> +         (message "Hide all")))
>>> +    ;; If error: "Before first heading" occurs, ignore it.
>>> +    (error nil)))
>>
>> This is basically an `ignore-errors' around a whole bunch of code, used
>> as a program flow mechanism, and that's always awkward, because it hides
>> real errors in the code.
>>
>> Altering the functions to not error out in these situations would be
>> better.
>
> Like 'outline-back-to-heading' has an optional argument 'invisible-ok',
> maybe a new argument named 'error-ok' or 'outside-ok' could
> be added to not error out when point is outside of the outline tree.

I can modify outline code to signal a signal (say ‘outline-before-first-heading) rather than an error, and handle that signal specifically. How’s that?

Yuan


Reply | Threaded
Open this post in threaded view
|

bug#41198: bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Lars Ingebrigtsen
Yuan Fu <[hidden email]> writes:

> I can modify outline code to signal a signal (say
> ‘outline-before-first-heading) rather than an error, and handle that
> signal specifically. How’s that?

Yes, that would also work.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#41198: bug#41130: bug#41198: 27.0.60; [PATCH] heading cycling command for outline

Yuan Fu-2


> On Oct 18, 2020, at 4:36 AM, Lars Ingebrigtsen <[hidden email]> wrote:
>
> Yuan Fu <[hidden email]> writes:
>
>> I can modify outline code to signal a signal (say
>> ‘outline-before-first-heading) rather than an error, and handle that
>> signal specifically. How’s that?
>
> Yes, that would also work.
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>   bloggy blog: http://lars.ingebrigtsen.no
Here is the new patch.

Yuan


outline-fix.patch (4K) Download Attachment
12