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 interface for adding value selection in QuickPick for extension API #233275

Conversation

CrafterKolyan
Copy link
Contributor

@CrafterKolyan CrafterKolyan commented Nov 7, 2024

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

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

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

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

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

@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Nov 13, 2024

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

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

value is not on QuickInput:

export interface QuickInput {

It is, however, in QuickPick:

value: string;

and InputBox:

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

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.

@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

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

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
7 checks passed
@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.
Labels
None yet
Projects
None yet
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