-
Notifications
You must be signed in to change notification settings - Fork 28.7k
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
QuickOpen Input: Fixes Validation which produces #42763 #45077
QuickOpen Input: Fixes Validation which produces #42763 #45077
Conversation
@@ -329,7 +329,7 @@ export class QuickOpenController extends Component implements IQuickOpenService | |||
} | |||
|
|||
// Respect input value | |||
if (options.value) { | |||
if (options.value && !options.valueSelection) { | |||
this.pickOpenWidget.setValue(options.value, options.valueSelection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't we loosing the value here as soon as a selection is provided? Isn't the better change to reset valueSelection
after an validation pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue I found (just to clarify my thinking, not implying that it's correct) is that we set the value if it's present in options.value, which causes the cursor to move to the end of the string.
By checking that we haven't already started the validation process (init), we can set the value / placeholder / template value, and then when the user begins to enter their own (and thus we start validating against the rule set), and ignore setting the value itself which would cause the cursor jump. I'm unsure how the selection works in all it's entirety, so if you have a better solution / idea I'm open to fixing this PR and learning from the updated fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which causes the cursor to move to the end of the string.
I believe that happens because we explicitly set the value selection
Closing this because I have pushed a fix for the underlying issue... Sorry for jumping in but the quick-pick infrastructure is a minefield full of debt and surprises. I am not expecting anyone to find his/her way around in there. cc @bpasero |
@jrieken yup, one idea forward would be to take out the input-related UI pieces from quick open into their own widget and decouple this from quick open altogether. I talked with @chrmarti about it who is in charge to explore this in the context of more input-related things like wizards and multi-select input. |
This fixes the issue which was described by #42763
Examples:
Code Used to Simulate Error
Azure Functions Replication of Error