Skip to content

Add outline.collapseItems and fix outlines collapsing unexpectedly (#53262)#158650

Merged
jrieken merged 13 commits into
microsoft:mainfrom
gjsjohnmurray:fix-53262
Oct 7, 2022
Merged

Add outline.collapseItems and fix outlines collapsing unexpectedly (#53262)#158650
jrieken merged 13 commits into
microsoft:mainfrom
gjsjohnmurray:fix-53262

Conversation

@gjsjohnmurray

@gjsjohnmurray gjsjohnmurray commented Aug 20, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR implements #53262, adding a setting outline.initialState with values collapsed and expanded (the default).

I used an enumeration rather than a boolean so it can be extended in the future if required.

The 'Collapse All' button now becomes 'Expand All' when tree is fully collapsed.

I also resolved a bug that was causing outlines to collapse unexpectedly after switching several times between files. To reproduce the bug:

  1. Open two files whose language supports outline view. I used launch.json and tasks.json from a .vscode folder.
  2. When first loaded their outlines will be fully expanded.
  3. Switch back and forth between the two editor tabs. After a couple of cycles you will see the outlines collapse, and even if you expand nodes in the outline they will collapse if you switch away and back.

@gjsjohnmurray gjsjohnmurray mentioned this pull request Aug 20, 2022
Closed
@gjsjohnmurray

gjsjohnmurray commented Sep 19, 2022

Copy link
Copy Markdown
Contributor Author

@jrieken please consider merging this in time for next week's endgame start.

@jrieken jrieken added this to the October 2022 milestone Sep 27, 2022
jrieken
jrieken reviewed Oct 5, 2022
View reviewed changes
Comment thread src/vs/workbench/contrib/outline/browser/outlinePane.ts Outdated
tree.dispose();
this._tree = undefined;
}));
this._disposables.add(this._tree.onDidChangeCollapseState(() => this._updateAllCollapsedContext()));

@jrieken jrieken Oct 5, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will leak because listeners are piling up in the object disposable store, not the session store...

@gjsjohnmurray gjsjohnmurray Oct 6, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this.

jrieken
jrieken reviewed Oct 5, 2022
View reviewed changes

@jrieken jrieken left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This is looking pretty good. I have pushed a small tweak to prevent listeners piling up and added an extra ctx update that's needed when switching between editors.

Before we merge: can we find a better name for the setting? IMO it should mention expand or collapse

@gjsjohnmurray

gjsjohnmurray commented Oct 6, 2022

Copy link
Copy Markdown
Contributor Author

can we find a better name for the setting? IMO it should mention expand or collapse

How about outline.initialExpandState (drawing from workbench.tree.expandMode as the model)?

Or conversely outline.initialCollapseState (model is search.collapseResults)?

Between those two I slightly prefer the first because it aligns with the default (existing) behaviour (initially expanded).

@jrieken

jrieken commented Oct 6, 2022

Copy link
Copy Markdown
Contributor

I prefer the naming of the former but the latter is a more similar case - because it isn't about an interaction model but default expand/collapse state. How about outline.collapseItems with values alwaysExpand and alwaysCollapse. The enum makes it descriptive and allows for a future auto/smart mode which expands the first N levels

@gjsjohnmurray

gjsjohnmurray commented Oct 6, 2022

Copy link
Copy Markdown
Contributor Author

How about outline.collapseItems with values alwaysExpand and alwaysCollapse.

I like this. Will you make the changes, or do you want me to?

@jrieken

jrieken commented Oct 6, 2022

Copy link
Copy Markdown
Contributor

I like this. Will you make the changes, or do you want me to?

As you like. I won't be able to do it today but could tackle it tomorrow

@gjsjohnmurray

gjsjohnmurray commented Oct 6, 2022

Copy link
Copy Markdown
Contributor Author

Please do it. I am out of time today, then basically OOO for nearly 2 weeks.

jrieken
jrieken approved these changes Oct 7, 2022
View reviewed changes

@jrieken jrieken left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gjsjohnmurray

@jrieken jrieken mentioned this pull request Oct 7, 2022
Closed
@gjsjohnmurray gjsjohnmurray changed the title Add outline.initialState and fix outlines collapsing unexpectedly (#53262) Add outline.collapseItems and fix outlines collapsing unexpectedly (#53262) Oct 7, 2022
@jrieken
jrieken merged commit ee19fe0 into microsoft:main Oct 7, 2022
@gjsjohnmurray
gjsjohnmurray deleted the fix-53262 branch October 7, 2022 08:52
@github-actions github-actions Bot locked and limited conversation to collaborators Nov 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@joaomoreno joaomoreno joaomoreno approved these changes
@jrieken jrieken jrieken approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@jrieken jrieken

Labels

None yet

Projects

None yet

Milestone

October 2022

Development

Successfully merging this pull request may close these issues.

3 participants

@gjsjohnmurray @jrieken @joaomoreno