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

[WIP] Adding a 'onKeyDown' handler to the quick pick/inputbox #36690

Closed
wants to merge 5 commits into from

Conversation

Chillee
Copy link

@Chillee Chillee commented Oct 21, 2017

Adds a function like so for the quick pick box for use for extensions.
handleKeyDown: (e: KeyboardEvent, value: string) => string

I decided to listen to handleKeyDown as in many cases (such as for the vim extension), we're trying to listen for keys like <right> or <tab> that are not captured by onType.

The idea right now is that it returns a string that is then assigned to the current input. I'd like to be able to return quick pick selection options as well, but I thought I'd open a PR first to make sure I'm not doing anything stupid.

Trying to solve #426, #23169, etc.

Specifically for VSCodeVim, I'm trying to fix these issues: VSCodeVim/Vim#745 and VSCodeVim/Vim#779

A couple things that probably need to be done.

  1. Test to make sure it works for quick pick as well. I suspect it shouldn't be too difficult... should be done in a similar method to how it's done currently.
  2. Modify the vscode.proposed.ts to expose the new interface.
  3. Write a sample extension to show how it's used.

@rebornix

@msftclas
Copy link

msftclas commented Oct 21, 2017

CLA assistant check
All CLA requirements met.

@@ -172,7 +173,7 @@ export class QuickOpenController extends Component implements IQuickOpenService
if (this.pickOpenWidget && this.pickOpenWidget.isVisible()) {
this.pickOpenWidget.hide(HideReason.CANCELED);
}

// options.handleKeyDown = (e, value) => {console.log(e, value); return value.trim();};
Copy link
Author

Choose a reason for hiding this comment

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

This is unnecessary, was accidentally left in here as a result for testing.

@mjbvz mjbvz requested a review from bpasero October 21, 2017 23:06
@bpasero bpasero added this to the On Deck milestone Oct 23, 2017
@Chillee Chillee changed the title [WIP] Adding a 'onKeyDown' handler to the quick pick box [WIP] Adding a 'onKeyDown' handler to the quick pick/inputbox Oct 23, 2017
@bpasero bpasero modified the milestones: On Deck, Backlog Nov 6, 2017
@bpasero
Copy link
Member

bpasero commented Dec 7, 2017

Closing due to inactivity.

@bpasero bpasero closed this Dec 7, 2017
@Chillee
Copy link
Author

Chillee commented Dec 7, 2017

@bpasero Sorry, is it the practice to close a PR for inactivity on the part of the maintainers? Nobody has provided any feedback on anything about this PR yet. I know there's things that need to be changed before it's merged, but I haven't heard anything on that.

@bpasero
Copy link
Member

bpasero commented Dec 7, 2017

@Chillee wasn't aware you are asking for a review, after all this PR was still marked as WIP by you. Are you waiting for a review or are you still working on this?

@Chillee
Copy link
Author

Chillee commented Dec 7, 2017

@bpasero I was waiting on a review to make sure I was touching the right places/taking the right approach. Currently, it works for the use case I opened up this PR for, but there hasn't been much other than that.

To turn this into something I would feel comfortable merging, I still need to update the vscode.d.ts, write some sample tests, and extend this inputbox API to the quickpick list as well. However, most of those things would involve doing things similar to what I did here, so I wanted a quick nod of approval first.

TL;DR I still want to work on it, but I'm waiting for a quick sanity check. Apologies if WIP means something else usually. I've typically also used it to solicit feedback.

@bpasero bpasero reopened this Dec 7, 2017
@bpasero
Copy link
Member

bpasero commented Dec 7, 2017

Sounds good. @rebornix fyi

@bpasero
Copy link
Member

bpasero commented Dec 7, 2017

@Chillee I do not think that we should push forward a solution where DOM types (KeyboardEvent) appear in the API. We typically do not expose any DOM relevant things to the extensions because extensions are not running in an environment where DOM access is possible.

I think for VIM to enable it for the editor we allow to overwrite what happens when the user types and then the extension is taking full control of the output (on the level of commands). We would probably need something like that for other parts where user input is possible. I think that would include any input box we have throughout the UI. An extension could then hook into the typing in these UI controls and decide what should happen. Does that make sense?

I think a PR for this is a bit unrealistic. In the future please make sure to help out for issues that have the "help wanted" label. Those are usually the ones where we think a PR makes sense 👍

@bpasero
Copy link
Member

bpasero commented Jan 11, 2018

Closing as per my comment.

@bpasero bpasero closed this Jan 11, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Mar 29, 2020
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.

4 participants