ctrlKey toggles the hint list #8504

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants

Fix for bug #8142. Control key toggles the hint list.

Member

peterflynn commented Jul 23, 2014

@ramsundhar20 Hmm, moving the highlight when you press Ctrl alone seems a little weird. It's very uncommon for modifier keys to cause any action on their own. Do you have an example of another editor that does this?

Also, make sure this doesn't break shortcut key handling. On Windows & Mac shortcuts are normally handled natively, but you can patch Globals.js to force brackets.nativeMenus to false in order to test JS-driven shortcut key handling. Are you still able to invoke commands like Ctrl+S or Ctrl+W while the code hints popup is open? And can you move the cursor in whole-word increments using Ctrl+Left/Right?

Thank you for your inputs. I will check the key combinations you suggested. I had a issue when implementing the actual requirement. I wasn't getting the space keycode when Ctrl was pressed. I was getting a keycode of 17 instead of 32. I thought it would be comfortable to toggle hints with just a single key, when the hint is open.

Contributor

RaymondLim commented Jul 23, 2014

@ramsundhar20 You cannot check keyCode === KeyEvent.DOM_VK_CONTROL to capture the Ctrl/Cmd+Space keydown combination. What you're getting is a single keydown on Ctrl key. The correct way to check for the combination is to check event.ctrlKey and keyCode === KeyEvent.DOM_VK_SPACE.

@RaymondLim please find my earlier comments in the bug.I have proposed the same solution. I am not getting the keycode 32(space) when Ctrl key is pressed. Please suggest a alternative method

Contributor

RaymondLim commented Jul 23, 2014

@ramsundhar20 You don't get space key because you're not updating the list of allowed keys in CodeHintList.prototype.isHandlingKeyCode by adding KeyEvent.DOM_VK_SPACE. Once you do that, then you should be able to check the key combination correct way.

@RaymondLim I tried that also. I will verify it once again.

Contributor

RaymondLim commented Jul 23, 2014

@ramsundhar20 You're right! After all, Ctrl+Space is already used as a shortcut to invoke the code hints and you won't be getting the combination as a keydown event. So we can't use the same key combination as described in #8142.

Any idea, how this can be implemented? How about hint list toggling on just space key instead of ctrl key?

It would void issues as @peterflynn said, if they persist.

Contributor

RaymondLim commented Jul 23, 2014

If we want to implement this, then we need to modify the code in _startNewSession() in CodeHintManager.js file by not calling _endSession() and instead call our new code to move down the selection in CodeHintList.js.

I will look into those sections and come back with a better fix.

I just checked the suggestions said by @peterflynn . It seems to work fine and doesn't break any functionality. I feel ctrl toggle is a cool feature to have. Its very handy, may be you can try this feature in your local code base.

Contributor

redmunds commented Aug 4, 2014

Assigning to @RaymondLim who is triaging this PR.

Contributor

RaymondLim commented Aug 4, 2014

@ramsundhar20 I don't think @peterflynn was approving the ctrl key as the right (or acceptable) shortcut. Since it does not fit to the request in #8142, we will not take this solution. You're welcome to work on a new solution as I mentioned in the above comment.

Member

peterflynn commented Aug 4, 2014

Yes, I think we should close this -- having Ctrl alone take any action is very unusual, and absent any precedent set by others editors doing that, I don't think we should do it either.

We can still take a fix for #8142 if it uses Ctrl+Space or some other standard shortcut, but it's probably best to do it atop a new, clean pull request.

@ramsundhar20 Sound ok?

Contributor

JeffryBooher commented Aug 4, 2014

I'm adding the triage complete label since we don't want to take this pr.

Member

peterflynn commented Aug 4, 2014

@JeffryBooher Normally, adding that label means we do want to take the PR. So I'll close it instead. (@ramsundhar20, feel free to open a new PR with a better shortcut if you want).

@peterflynn peterflynn closed this Aug 4, 2014

Sounds good. Am working on the fix. I will post a separate pull request after the fix is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment