Add action to toggle folding of child ranges
ClosedPublic

Authored by loh.tar on Apr 16 2019, 4:38 PM.

Details

Summary

With this patch will all, and only, the nested foldings of a range
(un)folded by right mouse click on the left hand icon border.

The range itself kept untouched, not toggled.

However, for convenience toggle the right button also a range which
doesn't contain other folding ranges.

  • Described action is also available in "View->Code Folding" menu as "Toggle Contained Nodes"
  • Old two actions "Fold/Unfold Current Node" are replaced by "Toggle Current Node"

The noted bug report request a slightly different behavior, but I think
it's close enough to considered that as "fixed".

BUG: 344414
FIXED-IN: 5.58

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
loh.tar created this revision.Apr 16 2019, 4:38 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptApr 16 2019, 4:38 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
loh.tar requested review of this revision.Apr 16 2019, 4:38 PM

This patch supersedes D20565

A possible improvement could be to add slots to make the folding accessible by some keyboard action, but I'm not in the mood for that now.
Bug 343060 - global folding code keyboard shortcuts

Oh, may that fit too?
Bug 352868 - Folding for all nodes (not just top-level) and/or siblings to current

I think the feature is nice.

For the code: I would like to have comments for the function decls. to tell what they really do.
e.g. toggleFoldingInRange needs a short summary what it does, the comments in the function themself are helpful.

Others, feedback?

The only real issue for the "usability" is imho the lack of discoverability.
Perhaps this should in addition get an action in the "Code Folding" menu?
The people can have a shortcut for it, too, if wanted.

I would prefer a context menu that has this as action. This is much better discoverable and also extensible with more folding actions.

mwolff added a subscriber: mwolff.Apr 17 2019, 10:04 AM

I would prefer a context menu that has this as action. This is much better discoverable and also extensible with more folding actions.

I agree. Right-click should show a context menu. Middle-click could toggle, if you need it.

loh.tar updated this revision to Diff 56444.Apr 17 2019, 3:35 PM
  • Add docu to header, not full happy with
  • Add desired adjustment checks
  • Make toggleFoldingInRange more action friendly
    • Add first try to unfold range itself into toggleFoldingInRange
    • Be a smart ass, minimize if nesting in mouseReleaseEvent
  • Fix to eager toggleFoldingInRange, don't fold else part when written on one line } else {

Regading toggleFoldingInRange

  • The return value is not needed anymore and will only in rare cases say "false"
  • Should now be useable as slot, so remove the return value?
  • Better name idea?

hm, there is already a folding action in the View menu.
I understood Dominik so, not to add a context menu to the icon border, but to add the action to the context menu of the "edit range".
Whatever, as said I'm not in the mood for that, and it should be done in a 2nd patch, when needed.
But a context menu on the icon border would I very much dislike. Middle click is not so handy and perform atm "paste to begin of line"

src/view/kateviewhelpers.cpp
2484

oops, "to offer such a folding"

cullmann requested changes to this revision.Apr 17 2019, 5:24 PM

The question is: with a context menu, isn't that then already too inconvenient?
At the moment we have no context menu for the bar at all.
I could live with the right click for the moment, we can still enhance that, if we add further "actions".
But I think we need at least an action in the "Code Foldings" sub-menu that does trigger this for the current active folding region.
If that is there, more can follow in extra changes, I think.

This revision now requires changes to proceed.Apr 17 2019, 5:24 PM
loh.tar updated this revision to Diff 56527.Apr 18 2019, 1:33 PM
loh.tar retitled this revision from Toggle folding of child ranges by right click to Add action to toggle folding of child ranges.
loh.tar edited the summary of this revision. (Show Details)
loh.tar set the repository for this revision to R39 KTextEditor.
  • Add menu action
  • Replace old two actions "Fold/Unfold Current Node" by "Toggle Current Node"
  • Move logic to ViewPrivate
  • KateBuffer: Add checks where they should be
loh.tar marked an inline comment as done.Apr 18 2019, 1:34 PM
cullmann accepted this revision.Apr 19 2019, 11:31 AM

Given we have no actions and some more proper documentation, I am in favor of this.

I would remove the two asserts here:

Q_ASSERT(startLine >= 0);

Q_ASSERT(startLine < lines());
if (startLine < 0 || startLine >= lines()) {
    return KTextEditor::Range::invalid();
}

I think range checking + invalid range as result is good enough.

This revision is now accepted and ready to land.Apr 19 2019, 11:31 AM
This revision was automatically updated to reflect the committed changes.

I would prefer a context menu that has this as action. This is much better discoverable and also extensible with more folding actions.

I agree. Right-click should show a context menu. Middle-click could toggle, if you need it.

Me wrote:

But a context menu on the icon border would I very much dislike. Middle click is not so handy and perform atm "paste to begin of line"

Well, as long the menu works everywhere but not the folding area, may that indeed not so bad. There could also be actions to toggle the visibility of parts of the border.
Or you make the current right-click the left-click action. That may even more handy and give current left-click the middle button.