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

add fill selection text for quick search #191956

Merged
merged 14 commits into from
Sep 14, 2023
Merged

Conversation

weartist
Copy link
Contributor

@weartist weartist commented Sep 1, 2023

For: #191513

@weartist
Copy link
Contributor Author

weartist commented Sep 1, 2023

This is a simple version, there are still many problems to solve, i.e. if we select a word in the editor, but we switch the cursor to the left search window, and let the cursor in the input box, the shortcut search will still fill the editor selected word, I think it should not be filled at this time, so I will add judgment for editor is focus

Copy link
Contributor

@andreamah andreamah left a comment

Choose a reason for hiding this comment

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

Thanks again for working on this! Just some small comments.

Copy link
Contributor

@andreamah andreamah left a comment

Choose a reason for hiding this comment

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

Thanks! just a few more nitpicks

@weartist
Copy link
Contributor Author

weartist commented Sep 8, 2023

Thanks! just a few more nitpicks

These are all help to correct my code progress ~

Copy link
Contributor

@andreamah andreamah left a comment

Choose a reason for hiding this comment

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

Apologies for not commenting on this earlier - I just noticed this while testing your PR.

@andreamah
Copy link
Contributor

@TylerLeonhardt This PR looks fine to me, but I did notice that this doesn't work if preserveInput is true. This is because preserveInput takes precedence over any default string (even if defaultFilterValue is defined).

if (defaultFilterValue === DefaultQuickAccessFilterValue.LAST) {
newValue = this.lastAcceptedPickerValues.get(descriptor);
} else if (typeof defaultFilterValue === 'string') {
newValue = `${descriptor.prefix}${defaultFilterValue}`;
}

Should we edit this somehow, or is this intentional?

@TylerLeonhardt
Copy link
Member

@andreamah that seems backwards to me. Can you get an issue going and I'll take a look deeper?

@TylerLeonhardt
Copy link
Member

Excited to see this land! Thanks, @weartist!

@VSCodeTriageBot VSCodeTriageBot added this to the September 2023 milestone Sep 12, 2023
@andreamah andreamah merged commit 23605b1 into microsoft:main Sep 14, 2023
9 checks passed
lins0621 pushed a commit to lins0621/vscode that referenced this pull request Sep 14, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 29, 2023
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.

None yet

5 participants