Skip to content

Add 'Collapse All Diffs' action button to multi-diff editor#199064

Merged
hediet merged 6 commits intomicrosoft:mainfrom
gjsjohnmurray:do-198248
Nov 29, 2023
Merged

Add 'Collapse All Diffs' action button to multi-diff editor#199064
hediet merged 6 commits intomicrosoft:mainfrom
gjsjohnmurray:do-198248

Conversation

@gjsjohnmurray
Copy link
Copy Markdown
Contributor

@gjsjohnmurray gjsjohnmurray commented Nov 24, 2023 •
edited
Loading

This PR closes #198248

junk

@gjsjohnmurray
Copy link
Copy Markdown
Contributor Author

gjsjohnmurray commented Nov 24, 2023

/assign @hediet in the hope this will be considered for merging, perhaps even in time for upcoming endgame of the November 2023 milestone.

@vscodenpa vscodenpa mentioned this pull request Nov 24, 2023
Closed
hediet
hediet reviewed Nov 27, 2023
View reviewed changes
Comment thread src/vs/editor/browser/widget/multiDiffEditorWidget/multiDiffEditorViewModel.ts Outdated

public collapseAll(): void {
for (const d of this.items.get()) {
d.collapsed.set(true, undefined);
Copy link
Copy Markdown
Member

@hediet hediet Nov 27, 2023

Choose a reason for hiding this comment

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

This should run in a transaction (rather than passing in undefined), as otherwise every collapsed.set will cause a re-layout.

Copy link
Copy Markdown
Contributor Author

@gjsjohnmurray gjsjohnmurray Nov 27, 2023

Choose a reason for hiding this comment

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

Thanks!

hediet
hediet reviewed Nov 27, 2023
View reviewed changes
Comment thread src/vs/editor/browser/widget/multiDiffEditorWidget/multiDiffEditorViewModel.ts Outdated
@gjsjohnmurray @hediet
Use transaction for collapseAll
19116d5 
Co-authored-by: Henning Dieterichs <notify.henning.dieterichs@live.de>
hediet
hediet previously approved these changes Nov 27, 2023
View reviewed changes
@hediet
Copy link
Copy Markdown
Member

hediet commented Nov 27, 2023

Fails with:

src/vs/editor/browser/widget/multiDiffEditorWidget/multiDiffEditorViewModel.ts(31,1): Bad whitespace indentation
[hygiene                 ] src/vs/editor/browser/widget/multiDiffEditorWidget/multiDiffEditorViewModel.ts(32,1): Bad whitespace indentation
[hygiene                 ] src/vs/editor/browser/widget/multiDiffEditorWidget/multiDiffEditorViewModel.ts(33,1): Bad whitespace indentation
[hygiene                 ] src/vs/editor/browser/widget/multiDiffEditorWidget/multiDiffEditorViewModel.ts(34,1): Bad whitespace indentation
[hygiene                 ] File not formatted. Run the 'Format Document' command to fix it: src/vs/editor/browser/widget/multiDiffEditorWidget/multiDiffEditorViewModel.ts

Probably because you took my suggestion where I had to use space to indent (urgh github).

hediet
hediet previously approved these changes Nov 27, 2023
View reviewed changes
@hediet
Copy link
Copy Markdown
Member

hediet commented Nov 27, 2023

Just noticed:

precondition: ContextKeyExpr.equals('activeEditor', MultiDiffEditor.ID),

This might be problematic when you have a multi diff editor to the side. When the activeEditor is a normal text editor, the menu entry would disappear from the multi diff editor and only reappear once it gets focus. (from my understanding of activeEditor, it would need to be tested)

chrmarti
chrmarti previously approved these changes Nov 27, 2023
View reviewed changes
@gjsjohnmurray
Copy link
Copy Markdown
Contributor Author

gjsjohnmurray commented Nov 27, 2023

This might be problematic when you have a multi diff editor to the side.

It's already the case that when an editor's group is not the active group none of its toolbar buttons display. So I don't think anything needs to be changed. Please merge this PR.

@gjsjohnmurray gjsjohnmurray dismissed stale reviews from chrmarti and hediet via 5b90766 November 28, 2023 10:25
@gjsjohnmurray gjsjohnmurray mentioned this pull request Nov 28, 2023
Closed
@hediet hediet enabled auto-merge November 28, 2023 11:04
@hediet hediet disabled auto-merge November 28, 2023 11:04
@hediet
Copy link
Copy Markdown
Member

hediet commented Nov 28, 2023

Today we have our testing day and don't allow code changes, but we can merge it tomorrow.

@hediet hediet enabled auto-merge November 29, 2023 12:03
@hediet hediet merged commit 3d990fc into microsoft:main Nov 29, 2023
@gjsjohnmurray gjsjohnmurray deleted the do-198248 branch November 29, 2023 13:41
@gjsjohnmurray gjsjohnmurray mentioned this pull request Dec 4, 2023
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hediet hediet hediet approved these changes

@sandy081 sandy081 sandy081 approved these changes

@chrmarti chrmarti Awaiting requested review from chrmarti

Assignees

@hediet hediet

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Multi diff editor - Support collapsing of all documents

4 participants

@gjsjohnmurray @hediet @chrmarti @sandy081