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

Preserve sort order when filtering Git branch / tag quickpicks (fix #199471) #199473

Merged
merged 11 commits into from
Jan 15, 2024

Conversation

gjsjohnmurray
Copy link
Contributor

@gjsjohnmurray gjsjohnmurray commented Nov 29, 2023

This PR fixes #199471 by using the quickPickSortByLabel proposed API.

@gjsjohnmurray
Copy link
Contributor Author

Can this get merged soon, or do the rules of your grooming iteration mean it'll have to wait another month?

@gjsjohnmurray
Copy link
Contributor Author

@lszomoru please consider merging this

@lszomoru
Copy link
Member

lszomoru commented Jan 8, 2024

Working with @TylerLeonhardt to make this option available when using showQuickPick().
As soon as those changes are in, this PR should become much simpler.

@lszomoru lszomoru added this to the December / January 2024 milestone Jan 8, 2024
extensions/git/src/commands.ts Outdated Show resolved Hide resolved
extensions/git/src/commands.ts Outdated Show resolved Hide resolved
@TylerLeonhardt
Copy link
Member

We discussed this and this won't be added to showQuickPick so this PR is the way to go

Copy link
Member

@lszomoru lszomoru left a comment

Choose a reason for hiding this comment

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

@gjsjohnmurray, could you please address the comments? Thank you!

@lszomoru lszomoru enabled auto-merge (squash) January 15, 2024 11:12
quickPick.placeholder = placeHolder;
quickPick.sortByLabel = false;
quickPick.items = await items;
listeners.push(quickPick.onDidHide(() => {
Copy link
Member

Choose a reason for hiding this comment

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

If the quick input is hidden (user presses ESC) this method will never return. I believe that the onDidHide event listener needs to go inside the choice method. onDidHide should only run resolve(undefined). Then on line 2671, we only need to dispose the quickPick (this will hide + dispose) and the listeners.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have pushed a change that I hope does what you mean.

Copy link
Member

@lszomoru lszomoru left a comment

Choose a reason for hiding this comment

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

@gjsjohnmurray, found another issue now that I looked at the code again.

auto-merge was automatically disabled January 15, 2024 11:32

Head branch was pushed to by a user without write access

@lszomoru lszomoru enabled auto-merge (squash) January 15, 2024 11:51
@lszomoru lszomoru merged commit 2b06224 into microsoft:main Jan 15, 2024
6 checks passed
@gjsjohnmurray gjsjohnmurray deleted the fix-199471 branch January 15, 2024 14:01
@microsoft microsoft locked and limited conversation to collaborators Jun 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"git.branchSortOrder": "committerdate" is lost when quickpick is being filtered
4 participants