Fix for issue #8142: "Ctrl + Space" should move to the next entry when the code hint menu is open by bmax · Pull Request #12251 · adobe/brackets

bmax · 2016-02-28T05:53:03Z

Hello,

Here is my suggested fix for #8142. There are a couple of lines of code that I changed and on each line I will comment and explain. I am new to Open Source and am sure this is not the best way! Any feedback is welcomed.

bmax · 2016-02-28T05:53:34Z

+     * @param {KeyBoardEvent} keyEvent
     */
-    CodeHintList.prototype.isHandlingKeyCode = function (keyCode) {
+    CodeHintList.prototype.isHandlingEvent = function (event) {


Changed this to isHandlingEvent so that we could check the actual event object to make sure ctrlKey is called.

This is potentially a breaking change. One way would be deprecating the isHandlingKeyCode method, throwing a deprecation warning, constructing a new event with the keycode and passing it to the isHandlingEvent-method.

Another (simpler) way would be keeping the isHandlingKeyCode and checking the type, such as

CodeHintList.prototype.isHandlingKeyCode = function (keyCodeOrEvent) { var keyCode = typeof keyCodeOrEvent === "object" ? keyCodeOrEvent.keyCode || keyCodeOrEvent; }

+1, type checking the argument seems to be better. else the public API will break.
Also, keep the existing function name and functionality intact.

abose · 2016-02-28T06:45:05Z

@bmax Thank you for contributing to Brackets. You will need to Accept the Brackets CLA before we can merge this pull request.
For discussions with the brackets community or any assistance/queries, please join the brackets slack channel by sending a mail to admin@brackets.io with the subject line slack registration request specifying the email addresses you would like to register.
Also, join brackets dev google groups link here..

bmax · 2016-02-28T07:05:51Z

@abose Thank you for the quick response. I emailed for slack registration and also accepted CLA.

petetnt · 2016-02-28T11:47:55Z

-    CodeHintList.prototype.isHandlingKeyCode = function (keyCode) {
+    CodeHintList.prototype.isHandlingEvent = function (event) {
+        var keyCode = event.keyCode;
        return (keyCode === KeyEvent.DOM_VK_UP || keyCode === KeyEvent.DOM_VK_DOWN ||


IMHO this check is getting pretty behemothy and could be refactored somehow.

petetnt · 2016-02-28T12:04:01Z

Made some initial comments. 👍

abose · 2016-02-28T16:59:49Z

+            // if the response is true, end the session and begin another
+            if (response === true) {
+                var previousEditor = sessionEditor;
+                //_endSession();


remove commented code.

sprintr · 2016-02-29T09:35:01Z

A couple of issues I am facing with this PR.

$(".foo").

Making an explicit request here makes the code hints flash in and flash out. You don't get a chance to select any of the hints.
It should show the first hint when explicitly requested and should move top-to-bottom rather than bottom-to-top.

bmax · 2016-02-29T14:49:40Z

@sprintr Thank you! 1. I did not think about the case of implicit calls and not sure why it flashes in and out only sometimes but other times it stays open fine. 2. easy fix, will be in next commit.

bmax · 2016-02-29T14:50:26Z

Found a small problem with my fix, If you have hint list keyboard shortcut set to ctrl+space then when you open the list, it automatically jumps to the end of the list. One way I can think of fixing this is making codeHintOpened a boolean and when it is >= 1 then allow the ctrl+space command to work. Any better ideas or suggestions?

petetnt · 2016-03-02T20:29:04Z

                keyCode === KeyEvent.DOM_VK_PAGE_UP || keyCode === KeyEvent.DOM_VK_PAGE_DOWN ||
                keyCode === KeyEvent.DOM_VK_RETURN ||
+                keyCode === KeyEvent.DOM_VK_CONTROL ||
+                (ctrlKey && keyCode === KeyEvent.DOM_VK_SPACE) ||


Nit: linting error here (should be indented to column 20, not 17)

abose · 2016-03-31T15:51:54Z

Pinging @bmax
Hope you got time to look into the issues raised by @petetnt

petetnt · 2016-04-04T07:12:15Z

We made some progress with @bmax last weekend but there's still some (small) issues withstanding regarding two failing tests. 🚒

petetnt · 2016-06-24T11:39:55Z

I fixed the errors in the tests, see the attached diff

diff --git a/src/editor/CodeHintList.js b/src/editor/CodeHintList.js
index 24b8e92..e0edc2c 100644
--- a/src/editor/CodeHintList.js
+++ b/src/editor/CodeHintList.js
@@ -320,6 +320,7 @@ define(function (require, exports, module) {
                 keyCode === KeyEvent.DOM_VK_PAGE_UP || keyCode === KeyEvent.DOM_VK_PAGE_DOWN ||
                 keyCode === KeyEvent.DOM_VK_RETURN ||
                 keyCode === KeyEvent.DOM_VK_CONTROL ||
+                keyCode === KeyEvent.DOM_VK_ESCAPE ||
                 (ctrlKey && keyCode === KeyEvent.DOM_VK_SPACE) ||
                 (keyCode === KeyEvent.DOM_VK_TAB && this.insertHintOnTab));
     };
@@ -394,11 +395,12 @@ define(function (require, exports, module) {
         if (event.type === "keydown" && this.isHandlingKeyCode(event)) {
             keyCode = event.keyCode;

-            if (event.shiftKey &&
-                    (event.keyCode === KeyEvent.DOM_VK_UP ||
-                     event.keyCode === KeyEvent.DOM_VK_DOWN ||
-                     event.keyCode === KeyEvent.DOM_VK_PAGE_UP ||
-                     event.keyCode === KeyEvent.DOM_VK_PAGE_DOWN)) {
+            if (event.keyCode === KeyEvent.DOM_VK_ESCAPE ||
+                    (event.shiftKey &&
+                        (event.keyCode === KeyEvent.DOM_VK_UP ||
+                        event.keyCode === KeyEvent.DOM_VK_DOWN ||
+                        event.keyCode === KeyEvent.DOM_VK_PAGE_UP ||
+                        event.keyCode === KeyEvent.DOM_VK_PAGE_DOWN))) {
                 this.handleClose();

                 // Let the event bubble.
@@ -406,7 +408,7 @@ define(function (require, exports, module) {
             } else if (keyCode === KeyEvent.DOM_VK_UP) {
                 _rotateSelection.call(this, -1);
             } else if (keyCode === KeyEvent.DOM_VK_DOWN ||
-                (event.ctrlKey && keyCode === KeyEvent.DOM_VK_SPACE)) {
+                    (event.ctrlKey && keyCode === KeyEvent.DOM_VK_SPACE)) {
                 _rotateSelection.call(this, 1);
             } else if (keyCode === KeyEvent.DOM_VK_PAGE_UP) {
                 _rotateSelection.call(this, -_itemsPerPage());
diff --git a/src/editor/CodeHintManager.js b/src/editor/CodeHintManager.js
index e31e4b6..c3a776b 100644
--- a/src/editor/CodeHintManager.js
+++ b/src/editor/CodeHintManager.js
@@ -563,7 +563,8 @@ define(function (require, exports, module) {
     function _handleKeyupEvent(jqEvent, editor, event) {
         keyDownEditor = editor;
         if (_inSession(editor)) {
-            if (event.keyCode === KeyEvent.DOM_VK_HOME || event.keyCode === KeyEvent.DOM_VK_END) {
+            if (event.keyCode === KeyEvent.DOM_VK_HOME ||
+                    event.keyCode === KeyEvent.DOM_VK_END) {
                 _endSession();
             } else if (event.keyCode === KeyEvent.DOM_VK_LEFT ||
                        event.keyCode === KeyEvent.DOM_VK_RIGHT ||
@@ -712,6 +713,16 @@ define(function (require, exports, module) {

     EditorManager.on("activeEditorChange", activeEditorChangeHandler);

+    // Dismiss code hints before executing any command other than showing code hints since the command
+    // may make the current hinting session irrevalent after execution.
+    // For example, when the user hits Ctrl+K to open Quick Doc, it is
+    // pointless to keep the hint list since the user wants to view the Quick Doc.
+    CommandManager.on("beforeExecuteCommand", function (event, commandId) {
+        if (commandId !== Commands.SHOW_CODE_HINTS) {
+            _endSession();
+        }
+    });
+    
     CommandManager.register(Strings.CMD_SHOW_CODE_HINTS, Commands.SHOW_CODE_HINTS, _startNewSession);

     exports._getCodeHintList        = _getCodeHintList;

First do git fetch origin master && git rebase origin/master, then apply that patch and run the tests -> all tests pass.

…with the javascript '.' character hints

bmax · 2016-06-29T03:27:14Z

@petetnt @abose This pull request should be ready to commit. I added two new tests to support the fix. I apologize about the terrible commit history, not sure if I should clean it up.

petetnt · 2016-06-29T06:40:09Z

Great job @bmax, LGTM.

For the commit history, I think the one who merges this PR should just do a squash & merge in the GitHub UI.

abose · 2016-07-02T06:17:57Z

👍 @bmax . can you squash some of the commits?
Tagging @swmitra code hints related change.

bmax · 2016-07-02T18:46:37Z

@abose Done! Let me know if that looks better.

petetnt · 2016-07-02T19:18:56Z

Again LGTM. It's pretty hard to squash this further manually, if needed I suggest just merging this with squash & merge on GitHub UI.

zaggino · 2016-08-25T22:54:27Z

@petetnt this is proably waiting for you

petetnt · 2016-08-26T07:23:04Z

Merging this. Thanks for this get PR and for your extended patience @bmax 👍

adityapaturi · 2016-09-21T11:45:36Z

@petetnt and @bmax : This change has introduced a new issue #12799. Please have a look

Can now move up with ctrl+space.

26fe388

bmax reviewed Feb 28, 2016
View reviewed changes

bmax mentioned this pull request Feb 28, 2016

"Ctrl + Space" should move to the next entry when the code hint menu popped up. #8142

Closed

petetnt reviewed Feb 28, 2016
View reviewed changes

abose reviewed Feb 28, 2016
View reviewed changes

bmax force-pushed the fix_issue_8142 branch from ca1288f to bb3cea9 Compare February 29, 2016 02:23

petetnt reviewed Mar 2, 2016
View reviewed changes

Merge branch 'master' of https://github.com/adobe/brackets

02fb12d

bmax added 2 commits June 25, 2016 20:26

Changes made suggested by PR

6273a1f

Found a cleaner way to do the alreadyOpen Bool. Still having trouble …

4d62ccc

…with the javascript '.' character hints

petetnt added 3 commits June 25, 2016 20:26

Remove unneeded var

e716cf5

Fix issues with CodeHints starting with dots

e427e2b

Fix wrong parameter

4b01805

bmax force-pushed the fix_issue_8142 branch from 4e3e2d4 to e9336d7 Compare July 2, 2016 15:26

bmax and others added 6 commits July 2, 2016 14:26

Changes made suggested by PR

d2d46c0

Fix issues with CodeHints starting with dots

3bdb1e3

Fix issues with CodeHints starting with dots

a290f41

Fix issues with CodeHints starting with dots

a35652b

Remove unneeded var

47ed6b5

All tests pass.

50619f2

bmax force-pushed the fix_issue_8142 branch from e9336d7 to 50619f2 Compare July 2, 2016 18:27

ficristo changed the title ~~Fix for issue #8142~~ Fix for issue #8142: "Ctrl + Space" should move to the next entry when the code hint menu is open Aug 14, 2016

petetnt merged commit 63962ce into adobe:master Aug 26, 2016

ficristo added this to the Release 1.8 milestone Aug 26, 2016

adityapaturi mentioned this pull request Sep 21, 2016

Pressing Escape key is removing multiple popups #12799

Closed

3 tasks done

This was referenced Aug 30, 2021

[CLOSED] Fix for issue #8142: "Ctrl + Space" should move to the next entry when the code hint menu is open brackets-archive/bracketsIssues#10527

Open

[CLOSED] Pressing Escape key is removing multiple popups brackets-archive/bracketsIssues#10900

Open

Conversation

bmax commented Feb 28, 2016

Uh oh!

bmax Feb 28, 2016

Choose a reason for hiding this comment

Uh oh!

petetnt Feb 28, 2016

Choose a reason for hiding this comment

Uh oh!

abose Feb 28, 2016

Choose a reason for hiding this comment

Uh oh!

abose commented Feb 28, 2016

Uh oh!

bmax commented Feb 28, 2016

Uh oh!

petetnt Feb 28, 2016

Choose a reason for hiding this comment

Uh oh!

petetnt commented Feb 28, 2016

Uh oh!

abose Feb 28, 2016

Choose a reason for hiding this comment

Uh oh!

sprintr commented Feb 29, 2016

Uh oh!

bmax commented Feb 29, 2016

Uh oh!

bmax commented Feb 29, 2016

Uh oh!

petetnt Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

abose commented Mar 31, 2016

Uh oh!

petetnt commented Apr 4, 2016

Uh oh!

petetnt commented Jun 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmax commented Jun 29, 2016

Uh oh!

petetnt commented Jun 29, 2016

Uh oh!

abose commented Jul 2, 2016

Uh oh!

bmax commented Jul 2, 2016

Uh oh!

petetnt commented Jul 2, 2016

Uh oh!

zaggino commented Aug 25, 2016

Uh oh!

petetnt commented Aug 26, 2016

Uh oh!

adityapaturi commented Sep 21, 2016

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

petetnt commented Jun 24, 2016 •

edited

Loading