Skip to content

Add interface for adding value selection in QuickPick for extension API #233275

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.

Sign up for GitHub

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
Merged
TylerLeonhardt merged 7 commits into from
Nov 14, 2024
Merged

Add interface for adding value selection in QuickPick for extension API #233275

TylerLeonhardt merged 7 commits into from
Nov 14, 2024

Conversation

@CrafterKolyan
Copy link
Contributor

@CrafterKolyan CrafterKolyan commented Nov 7, 2024 •
edited
Loading

Fixes #233274

Adding interface to setValueSelection in QuickInput for extensions.
I am also fine with creating property valueSelection like this:

vscode/src/vscode-dts/vscode.d.ts

Lines 12828 to 12837 in 818169a

/**
* Selection range in the input value. Defined as tuple of two number where the
* first is the inclusive start index and the second the exclusive end index. When `undefined` the whole
* pre-filled value will be selected, when empty (start equals end) only the cursor will be set,
* otherwise the defined range will be selected.
*
* This property does not get updated when the user types or makes a selection,
* but it can be updated by the extension.
*/
valueSelection: readonly [number, number] | undefined;

though it looks to me that having a functon setValueSelection explicitly documents the fact that you can't read valueSelection property for the current user selection.

The best implementation in my opinion should be valueSelection property that is also updated with user selection, though for my use case just setValueSelection works. Ready to reimplement it to any of these two cases.

Will probably change it to valueSelection any way as I think that means literally zero code from my side apart from exposing the already existing API.

@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Nov 8, 2024

We should have this be a property valueSelection to be aligned with the InputBox equivalent.

Keep in mind, since this requires API proposal, we will discuss this internally. However, this feels small and provides value so I think this will be a good addition.

@CrafterKolyan CrafterKolyan force-pushed the add-value-selection-in-quick-pick-extension-api branch from fb751b3 to 4ad5865 Compare November 8, 2024 05:50
@CrafterKolyan CrafterKolyan reopened this Nov 8, 2024
@CrafterKolyan
Copy link
Contributor Author

CrafterKolyan commented Nov 8, 2024 •
edited
Loading

Sure, no problem discussing this. I changed the interface.

To be honest I feel that this should be the property of QuickInput (which is more general and this is where value comes from) rather than of InputBox or QuickPick.

Also sorry for accidentally closing the PR, I accidentally force-pushed this branch to the current main and it automatically closed the PR.

@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Nov 8, 2024 •
edited
Loading

Yes, I think on QuickInput would be ideal. Your PR could just add valueSelection to QuickInput in a proposed API, and then when it's finalized we can remove it from InputBox

@CrafterKolyan CrafterKolyan changed the title Add interface for adding value selection in QuickPick for extension API Add interface for adding value selection in QuickInput for extension API Nov 8, 2024
@CrafterKolyan
Copy link
Contributor Author

CrafterKolyan commented Nov 8, 2024

Sure, I've changed this to have valueSelection on QuickInput

@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Nov 13, 2024 •
edited
Loading

Our API review meeting brought up a good point that value is both on QuickPick and InputBox... let's follow that for now. Sorry to go back and forth but can you add this to QuickPick and in a follow up we can fix both value and valueSelection to be on QuickInput?

@CrafterKolyan
Copy link
Contributor Author

CrafterKolyan commented Nov 13, 2024

Sorry, I think I misunderstand something. value already lives in QuickInput. InputBox and QuickPick simply inherit it from QuickInput. And as I said previously value lives in QuickInput.

What change do you propose? Right now valueSelection is defined near value. Both property and getters/setters

@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Nov 13, 2024 •
edited
Loading

value is not on QuickInput:

vscode/src/vscode-dts/vscode.d.ts

Line 12649 in beaa10f

export interface QuickInput {

It is, however, in QuickPick:

vscode/src/vscode-dts/vscode.d.ts

Line 12734 in beaa10f

value: string;

and InputBox:

vscode/src/vscode-dts/vscode.d.ts

Line 12826 in beaa10f

value: string;

We want valueSelection to be next to value... which in this case would be on InputBox and QuickPick

@CrafterKolyan
Copy link
Contributor Author

CrafterKolyan commented Nov 14, 2024

Ah sorry, got it. You were talking about the exposed API, rather than the implementation. Agree that in that case we should have valueSelection added only to QuickPick. Will do

@Fobnud-8wokpo-qivsyz

This comment has been minimized.

Sign in to view
@CrafterKolyan CrafterKolyan changed the title Add interface for adding value selection in QuickInput for extension API Add interface for adding value selection in QuickPick for extension API Nov 14, 2024
@CrafterKolyan
Copy link
Contributor Author

CrafterKolyan commented Nov 14, 2024

Sure, I've done that. Let me know if you think that implementation of valueSelection should also live in QuickPick and InputBox separately or if I can leave all of it in QuickInput

@vs-code-engineering vs-code-engineering bot added this to the November 2024 milestone Nov 14, 2024
@TylerLeonhardt TylerLeonhardt enabled auto-merge (squash) November 14, 2024 20:51
@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Nov 14, 2024

Thanks for working on this @CrafterKolyan! So the timeline for this is:

  • This is merged (today)
  • The proposed API is released (early dec)
  • We get some response that the API works as intended against real extensions (no concrete timeline, based on extension author feedback)
  • finalize it by merging it in to vscode.d.ts after all the above are done

@TylerLeonhardt TylerLeonhardt merged commit 3c86c97 into microsoft:main Nov 14, 2024
@CrafterKolyan CrafterKolyan deleted the add-value-selection-in-quick-pick-extension-api branch November 15, 2024 00:14
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Dec 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@roblourens roblourens roblourens approved these changes

@TylerLeonhardt TylerLeonhardt TylerLeonhardt approved these changes

Assignees

@TylerLeonhardt TylerLeonhardt

Labels

None yet

Projects

None yet

Milestone

November 2024

Development

Successfully merging this pull request may close these issues.

API to select character range of QuickPick input text value for VSCode extensions

4 participants

@CrafterKolyan @TylerLeonhardt @Fobnud-8wokpo-qivsyz @roblourens