Fixed #5670: implemented "collapse all" in the Outline widget

[fangnx]

• Added a "collapse all" toolbar item to the Outline widget.

[kittaakos]

Yes, I know it is work in progress...

I wish we could generalize it a bit. Do we need to add all this boilerplate code for each tree view we want to collapse?

Perhaps we have to add a subclass for AbstractViewContribution<TW> and allow to be collapsible with an option. Indeed, TW should be a subclass of TreeWidget. Supporting this would have a great value.

[vince-fugnitto]

Glad to see that the suggestion to create a custom OutlineTreeModel works better! :)

I noticed an odd behavior when the outline-widget-tree is collapsed.

Steps to Reproduce:

1. collapse the outline-widget-tree using the collapseAll toolbar button
2. select a symbol at the end of the editor
3. re-attempt to collapseAll, the editor jumps to the top

Solutions

For the following use case, I believe there are two approaches we can do to fix it:

1. only execute collapseAll if there is at least one node that is expanded
2. only enable the collapseAll button if there is a node that is expanded

[fangnx]

Glad to see that the suggestion to create a custom OutlineTreeModel works better! :)

I noticed an odd behavior when the outline-widget-tree is collapsed.

Steps to Reproduce:

1. collapse the outline-widget-tree using the collapseAll toolbar button
2. select a symbol at the end of the editor
3. re-attempt to collapseAll, the editor jumps to the top

Solutions

For the following use case, I believe there are two approaches we can do to fix it:

1. only execute collapseAll if there is at least one node that is expanded
2. only enable the collapseAll button if there is a node that is expanded

After some investigation, it seems when a symbol is selected in the editor, the selection in outline tree is not properly updated.

[fangnx]

@vince-fugnitto The issue with selected symbols in monaco editors has been solved. Now the collapsing action does not perform any selection, so there is no need for selecting node after it is done, which means the editor would not "move" with the collapse-all action.

[vince-fugnitto]

@fangnx I think someone else should also try reviewing, I helped through the development process and just want to know anyone else's feedback on the approach taken.

[kittaakos]

@fangnx, are you planning to generalize the Collapse All behavior for all TreeWidgets in Theia or you consider this PR as done?

[fangnx]

@fangnx, are you planning to generalize the Collapse All behavior for all TreeWidgets in Theia or you consider this PR as done?

I consider the PR as done. I think generalizing the TreeContribution should be a different PR as it requires quite a lot of refactoring.

[paul-marechal]

LGTM

Refactoring the boilerplate code in the other extensions would be nice, but it should be a separate PR as @fangnx said. For now this change does what it is meant to do :)

We may fill an issue regarding the refactoring, in case we don't work on it right away.

@kittaakos are you OK with this approach?

[vince-fugnitto]

@fangnx please rebase against master and resolve the conflicts of the changelog.
It still looks like its part of 0.9.0 and not 0.10.0.

[akosyakov]

@vince-fugnitto @marechal-p please finish the review, code-wise it looks good to me

[vince-fugnitto]

I think as an improvement we can make the icons expand and collapse perform expand and collapse without setting focus on the entire tree node.

I think the following use case can be handled better:

1. open the file callhierarchy-context.ts and select the last outline symbol (dispose) in the outline view
2. perform collapse-all in the outline view
3. attempt to re-expand the top level node using the expand icon (CallHierarchyContext)

Expected: the editor stays fixed, dispose stays highlighted (similarly to VSCode)
Actual: the editor jumps to the top, CallHierarchyContext and dispose are both highlighted

[fangnx]

I think as an improvement we can make the icons expand and collapse perform expand and collapse without setting focus on the entire tree node.

I think the following use case can be handled better:

1. open the file callhierarchy-context.ts and select the last outline symbol (dispose) in the outline view
2. perform collapse-all in the outline view
3. attempt to re-expand the top level node using the expand icon (CallHierarchyContext)

Expected: the editor stays fixed, dispose stays highlighted (similarly to VSCode)
Actual: the editor jumps to the top, CallHierarchyContext and dispose are both highlighted

Yes, I think that can be a good improvement (useful & aligned with VS Code behaviour). Should it be a different PR? ;)

[vince-fugnitto]

Yes, I think that can be a good improvement (useful & aligned with VS Code behaviour). Should it be a different PR? ;)

I think it can be done separately, but is a bug that two nodes are highlighted simultaneously?

[paul-marechal]

Would onExpansion make more sense?

[akosyakov]

It is naming convention for events or notification usually to start on. I prefer to give names to clarify intention. We could name handleExpansion but it does not explain what method does. I would usually then introduce 2 template methods:

• generic tempalte method handleExpansion, so one can override it
• and selectOnExpansion which is called by default from handleExpansion

selectOnExpansion then can be renamed to something else to clarify intention even better, i.e. selectIfAncestorOfSelected

[fangnx]

Just updated ;)

[akosyakov]

@marechal-p please help with landing if you are fine with changes

[paul-marechal]

LGTM, merging.