Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 6 commits into from Nov 29, 2023
Merged

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

merged 6 commits into from Nov 29, 2023
+46 −3

Conversation

gjsjohnmurray
Copy link
Contributor

@gjsjohnmurray gjsjohnmurray commented Nov 24, 2023 •
edited

This PR closes #198248

junk

@gjsjohnmurray
Copy link
Contributor Author

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

@VSCodeTriageBot VSCodeTriageBot mentioned this pull request Nov 24, 2023
Closed
hediet
hediet reviewed Nov 27, 2023
View reviewed changes
src/vs/editor/browser/widget/multiDiffEditorWidget/multiDiffEditorViewModel.ts Outdated
@@ -27,6 +27,12 @@ export class MultiDiffEditorViewModel extends Disposable {
}
}

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

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
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@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
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
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
Contributor Author

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 disabled auto-merge November 28, 2023 11:04
@hediet
Copy link
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 merged commit 3d990fc into microsoft:main Nov 29, 2023
6 checks passed
@gjsjohnmurray gjsjohnmurray deleted the do-198248 branch November 29, 2023 13:41
@gjsjohnmurray gjsjohnmurray mentioned this pull request Dec 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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