Hello,

Attached is a refactor that extracts the keyAction function in the sqlEditor
and placed it into a keyboard_shortcuts file. The extracted code is unit
tested. I did not look at the feature tests.

We are planning on diving into it a bit more but maybe not immediately. Up
next we will be pulling out the implementations of the functions that were
being triggered in the keyAction function.

-- Sarah





On Thu, Jul 20, 2017 at 12:55 PM, Robert Eckhardt <reckha...@pivotal.io>
wrote:

> Murtuza,
>
> Is there a particular reason you choose the keyboard shortcuts that you
> choose. When we were looking at this earlier to see what was being used
> elsewhere we discovered:
>
> jetbrains      cmd+/
> pycharm        cmd+/
> Sublime        Ctrl+/     Toggle line comment
>        Ctrl+Shift+/ Toggle block comment
> Eclipse        CTRL + /
> Notepad++      CTRL+Q           Toggle line comment
>        CTRL+SHIFT+Q Toggle block comment
> TextWrangler   Ctrl+/
>
> -- Rob
>
>
> On Thu, Jul 20, 2017 at 1:05 PM, Murtuza Zabuawala <murtuza.zabuawala@
> enterprisedb.com> wrote:
>
>>
>> On Thu, Jul 20, 2017 at 10:29 PM, Dave Page <dp...@pgadmin.org> wrote:
>>
>>> Hi
>>>
>>> On Thu, Jul 20, 2017 at 3:33 PM, Murtuza Zabuawala <
>>> murtuza.zabuaw...@enterprisedb.com> wrote:
>>>
>>>> Hi Dave,
>>>>
>>>> Please find patch attached, There were two issues,
>>>> 1) We removed the default button to clear the editor window, it
>>>> broke _clear_query_tool() functionality.
>>>> 2) The buttons arrangements, we added new Edit button in between Delete
>>>> and Filter button causing the "Explain" -> "Explain Options" sub menu to go
>>>> out of browser visibility in feature test, so it was failing.
>>>>
>>>> I have put Edit button near Clear button for now, until we come up with
>>>> new design for our editor for displaying these options.
>>>>
>>>
>>> Hmm, I moved it there intentionally as it's a more traditional position
>>> and thus more discoverable.
>>>
>>> Can we just launch the browser with a wider size, say, 1280? It's on
>>> line 43 of app_starter.py...
>>>
>>>
>> Yes, that will work too.
>>
>>>
>>>>
>>>> --
>>>> Regards,
>>>> Murtuza Zabuawala
>>>> EnterpriseDB: http://www.enterprisedb.com
>>>> The Enterprise PostgreSQL Company
>>>>
>>>> On Thu, Jul 20, 2017 at 6:04 PM, Murtuza Zabuawala <
>>>> murtuza.zabuaw...@enterprisedb.com> wrote:
>>>>
>>>>> Hi Dave,
>>>>>
>>>>> I am working on this, will send you patch soon.
>>>>>
>>>>> On Thu, Jul 20, 2017 at 5:53 PM, Dave Page <dp...@pgadmin.org> wrote:
>>>>>
>>>>>> Did you get a chance to look at this yet Murtuza?
>>>>>>
>>>>>> On Wed, Jul 19, 2017 at 3:37 PM, Murtuza Zabuawala <
>>>>>> murtuza.zabuaw...@enterprisedb.com> wrote:
>>>>>>
>>>>>>> Sure, Will take a look.
>>>>>>>
>>>>>>> --
>>>>>>> Regards,
>>>>>>> Murtuza Zabuawala
>>>>>>> EnterpriseDB: http://www.enterprisedb.com
>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>
>>>>>>> On Wed, Jul 19, 2017 at 8:00 PM, Dave Page <dp...@pgadmin.org>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Except I managed to break a couple of tests :-(. Can you take a
>>>>>>>> look please? I've had some other work come up that I need to deal with.
>>>>>>>>
>>>>>>>> ============================================================
>>>>>>>> ==========
>>>>>>>> ERROR: runTest (pgadmin.feature_tests.query_t
>>>>>>>> ool_journey_test.QueryToolJourneyTest)
>>>>>>>> Tests the path through the query tool
>>>>>>>> ------------------------------------------------------------
>>>>>>>> ----------
>>>>>>>> Traceback (most recent call last):
>>>>>>>>   File 
>>>>>>>> "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/query_tool_journey_test.py",
>>>>>>>> line 45, in runTest
>>>>>>>>     self._test_history_tab()
>>>>>>>>   File 
>>>>>>>> "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/query_tool_journey_test.py",
>>>>>>>> line 71, in _test_history_tab
>>>>>>>>     self.__clear_query_tool()
>>>>>>>>   File 
>>>>>>>> "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/query_tool_journey_test.py",
>>>>>>>> line 91, in __clear_query_tool
>>>>>>>>     self.page.click_element(self.page.find_by_xpath("//*[@id='bt
>>>>>>>> n-edit']"))
>>>>>>>>   File 
>>>>>>>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/pgadmin_page.py",
>>>>>>>> line 148, in find_by_xpath
>>>>>>>>     return self.wait_for_element(lambda driver:
>>>>>>>> driver.find_element_by_xpath(xpath))
>>>>>>>>   File 
>>>>>>>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/pgadmin_page.py",
>>>>>>>> line 232, in wait_for_element
>>>>>>>>     return self._wait_for("element to exist", element_if_it_exists)
>>>>>>>>   File 
>>>>>>>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/pgadmin_page.py",
>>>>>>>> line 282, in _wait_for
>>>>>>>>     "Timed out waiting for " + waiting_for_message)
>>>>>>>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>>>>>>>> ges/selenium/webdriver/support/wait.py", line 80, in until
>>>>>>>>     raise TimeoutException(message, screen, stacktrace)
>>>>>>>> TimeoutException: Message: Timed out waiting for element to exist
>>>>>>>>
>>>>>>>>
>>>>>>>> ============================================================
>>>>>>>> ==========
>>>>>>>> ERROR: runTest (pgadmin.feature_tests.query_t
>>>>>>>> ool_tests.QueryToolFeatureTest)
>>>>>>>> Query tool feature test
>>>>>>>> ------------------------------------------------------------
>>>>>>>> ----------
>>>>>>>> Traceback (most recent call last):
>>>>>>>>   File 
>>>>>>>> "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/query_tool_tests.py",
>>>>>>>> line 52, in runTest
>>>>>>>>     self._clear_query_tool()
>>>>>>>>   File 
>>>>>>>> "/Users/dpage/git/pgadmin4/web/pgadmin/feature_tests/query_tool_tests.py",
>>>>>>>> line 173, in _clear_query_tool
>>>>>>>>     self.page.find_by_id("btn-edit").click()
>>>>>>>>   File 
>>>>>>>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/pgadmin_page.py",
>>>>>>>> line 151, in find_by_id
>>>>>>>>     return self.wait_for_element(lambda driver:
>>>>>>>> driver.find_element_by_id(element_id))
>>>>>>>>   File 
>>>>>>>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/pgadmin_page.py",
>>>>>>>> line 232, in wait_for_element
>>>>>>>>     return self._wait_for("element to exist", element_if_it_exists)
>>>>>>>>   File 
>>>>>>>> "/Users/dpage/git/pgadmin4/web/regression/feature_utils/pgadmin_page.py",
>>>>>>>> line 282, in _wait_for
>>>>>>>>     "Timed out waiting for " + waiting_for_message)
>>>>>>>>   File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packa
>>>>>>>> ges/selenium/webdriver/support/wait.py", line 80, in until
>>>>>>>>     raise TimeoutException(message, screen, stacktrace)
>>>>>>>> TimeoutException: Message: Timed out waiting for element to exist
>>>>>>>>
>>>>>>>>
>>>>>>>> ------------------------------------------------------------
>>>>>>>> ----------
>>>>>>>> Ran 9 tests in 262.111s
>>>>>>>>
>>>>>>>> On Wed, Jul 19, 2017 at 11:55 AM, Murtuza Zabuawala <
>>>>>>>> murtuza.zabuaw...@enterprisedb.com> wrote:
>>>>>>>>
>>>>>>>>> Thank you Dave.
>>>>>>>>>
>>>>>>>>> On Wed, Jul 19, 2017 at 4:17 PM, Dave Page <dp...@pgadmin.org>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Thanks, applied.
>>>>>>>>>>
>>>>>>>>>> I also took the opportunity to tidy up the menus a little and add
>>>>>>>>>> access keys for accessibility.
>>>>>>>>>>
>>>>>>>>>> One change I made was to make the Edit and Clear menus not have a
>>>>>>>>>> default option - e.g. instead of a button with a drop-down next to 
>>>>>>>>>> it,
>>>>>>>>>> they're now a single dropdown button with icon. I think this works 
>>>>>>>>>> better
>>>>>>>>>> as there are no obvious candidates for the "default" option for those
>>>>>>>>>> menus. I'm not overly enthusiastic about the look of those buttons 
>>>>>>>>>> though,
>>>>>>>>>> so if anyone has a better idea how they should be styled, please yelp
>>>>>>>>>> (CCing Chethana for his input as well)...
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Wed, Jul 19, 2017 at 9:56 AM, Murtuza Zabuawala <
>>>>>>>>>> murtuza.zabuaw...@enterprisedb.com> wrote:
>>>>>>>>>>
>>>>>>>>>>> Just a FYI,
>>>>>>>>>>> You need to run yarn bundle for this to be working as Surinder
>>>>>>>>>>> has moved all the CodeMirror code into bundle package.
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Jul 19, 2017 at 2:20 PM, Murtuza Zabuawala <
>>>>>>>>>>> murtuza.zabuaw...@enterprisedb.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> PFA updated patch,
>>>>>>>>>>>> 1) Added Keyboard shortcuts to comment line, uncomment line and
>>>>>>>>>>>> comment/uncomment block of code also added drop-down for the same.
>>>>>>>>>>>> 2) Also added options for indent & unindent code in the same
>>>>>>>>>>>> drop-down.
>>>>>>>>>>>> 3) Updated shortcut documents accordingly.
>>>>>>>>>>>>
>>>>>>>>>>>> Please review.
>>>>>>>>>>>>
>>>>>>>>>>>> On Mon, Jul 17, 2017 at 3:05 PM, Dave Page <dp...@pgadmin.org>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Mon, Jul 17, 2017 at 10:31 AM, Murtuza Zabuawala <
>>>>>>>>>>>>> murtuza.zabuaw...@enterprisedb.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Dave,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Mon, Jul 17, 2017 at 2:33 PM, Dave Page <dp...@pgadmin.org
>>>>>>>>>>>>>> > wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Wed, Jul 12, 2017 at 1:16 PM, Murtuza Zabuawala <
>>>>>>>>>>>>>>> murtuza.zabuaw...@enterprisedb.com> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> PFA patch which will add functionality to allow user to
>>>>>>>>>>>>>>>> comment/uncomment code in query editor.
>>>>>>>>>>>>>>>> RM#2456
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This is cool, but I'm not sure it's right as-is:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> * I prefer SQL style commenting, e.g.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> -- This is a comment
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Should we make that a config option if CodeMirror can do it?
>>>>>>>>>>>>>>> Or a different hotkey?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I'll check the extension code and update you accordingly, and
>>>>>>>>>>>>>> It will be good idea to keep the both the options because with 
>>>>>>>>>>>>>> large code
>>>>>>>>>>>>>> block current style works the best.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Right.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> * You've added it as an option to the Clear XXX dropdown,
>>>>>>>>>>>>>>> which really isn't the right place in my opinion. Should we add 
>>>>>>>>>>>>>>> a new drop
>>>>>>>>>>>>>>> down for this, and include some/all of the other Editing 
>>>>>>>>>>>>>>> options on there?
>>>>>>>>>>>>>>> E.g. tab/shift-tab.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I thought that is misc options dropdown for our editor, but
>>>>>>>>>>>>>> I don't see any point adding new drop down for one single 
>>>>>>>>>>>>>> option, Can we
>>>>>>>>>>>>>> add new button instead?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> I think you missed this bit:  "and include some/all of the
>>>>>>>>>>>>> other Editing options on there? E.g. tab/shift-tab.". 
>>>>>>>>>>>>> Essentially, we'd be
>>>>>>>>>>>>> adding an Edit menu...
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> * I think the docs should say Ctrl+Shift+/ rather than
>>>>>>>>>>>>>>> Shift+Ctrl+/, and be ordered in the table to reflect that. It 
>>>>>>>>>>>>>>> seems more
>>>>>>>>>>>>>>> natural to me.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Initially I wrote ctrl + shift + /only but when I saw all
>>>>>>>>>>>>>> other shortcuts starts with Shift , then I changed it to shift + 
>>>>>>>>>>>>>> ctrl + / :)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> No they don't - Ctrl+Alt+Left for example. I believe it's
>>>>>>>>>>>>> normal to put Ctrl first, then Shift as it's a modifier.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thoughts?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>> Dave Page
>>>>>>>>>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>>>>>>>>>> Twitter: @pgsnake
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>>>>>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> --
>>>>>>>>>>>>> Dave Page
>>>>>>>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>>>>>>>> Twitter: @pgsnake
>>>>>>>>>>>>>
>>>>>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>>>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> Dave Page
>>>>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>>>>> Twitter: @pgsnake
>>>>>>>>>>
>>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Dave Page
>>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>>> Twitter: @pgsnake
>>>>>>>>
>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Dave Page
>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>> Twitter: @pgsnake
>>>>>>
>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>> The Enterprise PostgreSQL Company
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>> --
>>> Dave Page
>>> Blog: http://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EnterpriseDB UK: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>
>>
>
diff --git a/web/pgadmin/static/js/sqleditor/keyboard_shortcuts.js 
b/web/pgadmin/static/js/sqleditor/keyboard_shortcuts.js
new file mode 100644
index 00000000..c1174139
--- /dev/null
+++ b/web/pgadmin/static/js/sqleditor/keyboard_shortcuts.js
@@ -0,0 +1,48 @@
+const F5_KEY = 116,
+  F7_KEY = 118,
+  F8_KEY = 119,
+  COMMA_KEY = 188,
+  PERIOD_KEY = 190,
+  FWD_SLASH_KEY = 191;
+
+function keyboardShortcuts(sqlEditorController, event) {
+  if (sqlEditorController.isQueryRunning()) {
+    return;
+  }
+
+  let keyCode = event.which || event.keyCode;
+
+  if (keyCode === F5_KEY) {
+    event.preventDefault();
+    sqlEditorController.executeQuery();
+  } else if (event.shiftKey && keyCode === F7_KEY) {
+    _stopEventPropagation();
+    sqlEditorController.explainAnalyze(event);
+  } else if (keyCode === F7_KEY) {
+    _stopEventPropagation();
+    sqlEditorController.explain(event);
+  } else if (keyCode === F8_KEY) {
+    event.preventDefault();
+    sqlEditorController.download();
+  } else if (event.shiftKey && event.ctrlKey && keyCode === COMMA_KEY) {
+    _stopEventPropagation();
+    sqlEditorController.commentLineCode();
+  } else if (event.shiftKey && event.ctrlKey && keyCode === PERIOD_KEY) {
+    _stopEventPropagation();
+    sqlEditorController.uncommentLineCode();
+  } else if (event.shiftKey && event.ctrlKey && keyCode === FWD_SLASH_KEY) {
+    _stopEventPropagation();
+    sqlEditorController.commentBlockCode();
+  }
+
+  function _stopEventPropagation() {
+    event.cancelBubble = true;
+    event.preventDefault();
+    event.stopPropagation();
+    event.stopImmediatePropagation();
+  }
+}
+
+module.exports = {
+  processEvent: keyboardShortcuts,
+};
diff --git a/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js 
b/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js
index 1ebe2b22..13bd03d1 100644
--- a/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js
+++ b/web/pgadmin/tools/sqleditor/templates/sqleditor/js/sqleditor.js
@@ -14,6 +14,7 @@ define('tools.querytool', [
     'sources/../jsx/history/query_history',
     'react', 'react-dom',
     'sources/alerts/alertify_wrapper',
+    'sources/sqleditor/keyboard_shortcuts',
     'sources/../bundle/slickgrid',
     'misc.file_manager',
     'backgrid.sizeable.columns',
@@ -23,7 +24,8 @@ define('tools.querytool', [
 ], function(
   gettext, url_for, $, _, S, alertify, pgAdmin, Backbone, codemirror,
   pgExplain, GridSelector, ActiveCellCapture, clipboard, copyData, 
RangeSelectionHelper, handleQueryOutputKeyboardEvent,
-    XCellSelectionModel, setStagedRows,  SqlEditorUtils, HistoryBundle, 
queryHistory, React, ReactDOM, AlertifyWrapper
+  XCellSelectionModel, setStagedRows,  SqlEditorUtils, HistoryBundle, 
queryHistory, React, ReactDOM, AlertifyWrapper,
+  keyboardShortcuts
 ) {
     /* Return back, this has been called more than once */
     if (pgAdmin.SqlEditor)
@@ -36,14 +38,6 @@ define('tools.querytool', [
         pgBrowser = pgAdmin.Browser,
         Slick = window.Slick;
 
-    // Define key codes for shortcut keys
-    var F5_KEY = 116,
-        F7_KEY = 118,
-        F8_KEY = 119,
-        COMMA_KEY = 188,
-        PERIOD_KEY = 190,
-        FWD_SLASH_KEY = 191;
-
     var is_query_running = false;
 
     // Defining Backbone view for the sql grid.
@@ -1270,14 +1264,7 @@ define('tools.querytool', [
 
       // Callback function for the flash button click.
       on_flash: function() {
-        var self = this;
-
-        // Trigger the flash signal to the SqlEditorController class
-        self.handler.trigger(
-            'pgadmin-sqleditor:button:flash',
-            self,
-            self.handler
-        );
+        this.handler.executeQuery();
       },
 
       // Callback function for the cancel query button click.
@@ -1292,49 +1279,19 @@ define('tools.querytool', [
         );
       },
 
-      // Callback function for the download button click.
-      on_download: function() {
-        var self = this;
-
-        // Trigger the download signal to the SqlEditorController class
-        self.handler.trigger(
-            'pgadmin-sqleditor:button:download',
-            self,
-            self.handler
-        );
-      },
-
       // Callback function for the line comment code
       on_comment_line_code: function() {
-        var self = this;
-        // Trigger the comment signal to the SqlEditorController class
-        self.handler.trigger(
-            'pgadmin-sqleditor:comment_line_code',
-            self,
-            self.handler
-        );
+        this.handler.commentLineCode();
       },
 
       // Callback function for the line uncomment code
       on_uncomment_line_code: function() {
-        var self = this;
-        // Trigger the comment signal to the SqlEditorController class
-        self.handler.trigger(
-            'pgadmin-sqleditor:uncomment_line_code',
-            self,
-            self.handler
-        );
+        this.handler.uncommentLineCode();
       },
 
       // Callback function for the block comment/uncomment code
       on_toggle_comment_block_code: function() {
-        var self = this;
-        // Trigger the comment signal to the SqlEditorController class
-        self.handler.trigger(
-            'pgadmin-sqleditor:toggle_comment_block_code',
-            self,
-            self.handler
-        );
+        this.handler.commentBlockCode();
       },
 
       on_indent_code: function() {
@@ -1433,33 +1390,19 @@ define('tools.querytool', [
       },
 
       // Callback function for explain button click.
-      on_explain: function(ev) {
-        var self = this;
+      on_explain: function(event) {
+        this._stopEventPropogation(event);
+        this._closeDropDown(event);
 
-        this._stopEventPropogation(ev);
-        this._closeDropDown(ev);
-
-        // Trigger the explain signal to the SqlEditorController class
-        self.handler.trigger(
-            'pgadmin-sqleditor:button:explain',
-            self,
-            self.handler
-        );
+        this.handler.explain(event);
       },
 
       // Callback function for explain analyze button click.
-      on_explain_analyze: function(ev) {
-        var self = this;
-
-        this._stopEventPropogation(ev);
-        this._closeDropDown(ev);
+      on_explain_analyze: function(event) {
+        this._stopEventPropogation(event);
+        this._closeDropDown(event);
 
-        // Trigger the explain analyze signal to the SqlEditorController class
-        self.handler.trigger(
-            'pgadmin-sqleditor:button:explain-analyze',
-            self,
-            self.handler
-        );
+        this.handler.explainAnalyze(event);
       },
 
       // Callback function for explain option "verbose" button click
@@ -1537,50 +1480,13 @@ define('tools.querytool', [
         );
       },
 
-      /*
-       * Callbacks for keyboard events bind to the
-       * query tool buttons.
-       * Following are the keyboard shortcuts:
-       *  F5 - Execute query
-       *  F7 - Explain query
-       *  F8 - Download result as CSV
-       *  Shift+F7 - Explain analyze query
-       */
-      keyAction: function(ev) {
-        // return if query is running
-        if (is_query_running) return;
-
-        var keyCode = ev.which || ev.keyCode;
-        if (ev.shiftKey && keyCode == F7_KEY) {
-          // Explain analyze query.
-          this.on_explain_analyze(ev);
-          ev.preventDefault();
-        } else if (keyCode == F5_KEY) {
-          // Execute query.
-          this.on_flash(ev);
-          ev.preventDefault();
-        } else if (keyCode == F7_KEY) {
-          // Explain query.
-          this.on_explain(ev);
-          ev.preventDefault();
-        } else if (keyCode == F8_KEY) {
-          // Download query result as CSV.
-          this.on_download(ev);
-          ev.preventDefault();
-        } else if (ev.shiftKey && ev.ctrlKey  && keyCode == COMMA_KEY ) {
-          // Toggle comments
-          this.on_comment_line_code(ev);
-          ev.preventDefault();
-        } else if (ev.shiftKey && ev.ctrlKey  && keyCode == PERIOD_KEY ) {
-          // Toggle comments
-          this.on_uncomment_line_code(ev);
-          ev.preventDefault();
-        } else if (ev.shiftKey && ev.ctrlKey  && keyCode == FWD_SLASH_KEY ) {
-          // Toggle comments
-          this.on_toggle_comment_block_code(ev);
-          ev.preventDefault();
-        }
-      }
+      on_download: function() {
+        this.handler.download();
+      },
+
+      keyAction: function(event) {
+        keyboardShortcuts.processEvent(this.handler, event);
+      },
     });
 
     /* Defining controller class for data grid, which actually
@@ -1657,21 +1563,13 @@ define('tools.querytool', [
           self.on('pgadmin-sqleditor:button:copy_row', self._copy_row, self);
           self.on('pgadmin-sqleditor:button:paste_row', self._paste_row, self);
           self.on('pgadmin-sqleditor:button:limit', self._set_limit, self);
-          self.on('pgadmin-sqleditor:button:flash', self._refresh, self);
           self.on('pgadmin-sqleditor:button:cancel-query', self._cancel_query, 
self);
-          self.on('pgadmin-sqleditor:button:download', self._download, self);
           self.on('pgadmin-sqleditor:button:auto_rollback', 
self._auto_rollback, self);
           self.on('pgadmin-sqleditor:button:auto_commit', self._auto_commit, 
self);
-          self.on('pgadmin-sqleditor:button:explain', self._explain, self);
-          self.on('pgadmin-sqleditor:button:explain-analyze', 
self._explain_analyze, self);
           self.on('pgadmin-sqleditor:button:explain-verbose', 
self._explain_verbose, self);
           self.on('pgadmin-sqleditor:button:explain-costs', 
self._explain_costs, self);
           self.on('pgadmin-sqleditor:button:explain-buffers', 
self._explain_buffers, self);
           self.on('pgadmin-sqleditor:button:explain-timing', 
self._explain_timing, self);
-          // Commenting related
-          self.on('pgadmin-sqleditor:comment_line_code', 
self._comment_line_code, self);
-          self.on('pgadmin-sqleditor:uncomment_line_code', 
self._uncomment_line_code, self);
-          self.on('pgadmin-sqleditor:toggle_comment_block_code', 
self._comment_block_code, self);
           // Indentation related
           self.on('pgadmin-sqleditor:indent_selected_code', 
self._indent_selected_code, self);
           self.on('pgadmin-sqleditor:unindent_selected_code', 
self._unindent_selected_code, self);
@@ -2745,7 +2643,7 @@ define('tools.querytool', [
 
 
         // This function will run the SQL query and refresh the data in the 
backgrid.
-        _refresh: function() {
+        executeQuery: function() {
           var self = this;
 
           // Start execution of the query.
@@ -2874,7 +2772,7 @@ define('tools.querytool', [
                 function() {
                   if (res.data.status) {
                     // Refresh the sql grid
-                    self._refresh();
+                    self.executeQuery();
                   }
                   else {
                     alertify.alert('Filter By Selection Error', 
res.data.result);
@@ -2944,7 +2842,7 @@ define('tools.querytool', [
                 function() {
                   if (res.data.status) {
                     // Refresh the sql grid
-                    self._refresh();
+                    self.executeQuery();
                   }
                   else {
                     alertify.alert('Filter Exclude Selection Error', 
res.data.result);
@@ -2995,7 +2893,7 @@ define('tools.querytool', [
                 function() {
                   if (res.data.status) {
                     // Refresh the sql grid
-                    self._refresh();
+                    self.executeQuery();
                   }
                   else {
                     alertify.alert('Remove Filter Error', res.data.result);
@@ -3050,7 +2948,7 @@ define('tools.querytool', [
                     $('#filter').addClass('hidden');
                     $('#editor-panel').removeClass('sql-editor-busy-fetching');
                     // Refresh the sql grid
-                    self._refresh();
+                    self.executeQuery();
                   }
                   else {
                     alertify.alert('Apply Filter Error',res.data.result);
@@ -3174,7 +3072,7 @@ define('tools.querytool', [
                 function() {
                   if (res.data.status) {
                     // Refresh the sql grid
-                    self._refresh();
+                    self.executeQuery();
                   }
                   else
                     alertify.alert('Change limit Error', res.data.result);
@@ -3414,7 +3312,7 @@ define('tools.querytool', [
         },
 
         // This function will download the grid data as CSV file.
-        _download: function() {
+        download: function() {
           var self = this,
           selected_code = self.gridView.query_tool_obj.getSelection(),
           sql = "";
@@ -3551,7 +3449,7 @@ define('tools.querytool', [
         },
 
         // This function will
-        _explain: function() {
+        explain: function() {
           var self = this;
           var verbose = $('.explain-verbose').hasClass('visibility-hidden') ? 
'OFF' : 'ON';
           var costs = $('.explain-costs').hasClass('visibility-hidden') ? 
'OFF' : 'ON';
@@ -3563,7 +3461,7 @@ define('tools.querytool', [
         },
 
         // This function will
-        _explain_analyze: function() {
+        explainAnalyze: function() {
           var self = this;
           var verbose = $('.explain-verbose').hasClass('visibility-hidden') ? 
'OFF' : 'ON';
           var costs = $('.explain-costs').hasClass('visibility-hidden') ? 
'OFF' : 'ON';
@@ -3726,21 +3624,21 @@ define('tools.querytool', [
         /*
          * This function will comment code (Wrapper function)
          */
-        _comment_line_code: function() {
+        commentLineCode: function() {
           this._toggle_comment_code('comment_line');
         },
 
         /*
          * This function will uncomment code (Wrapper function)
          */
-        _uncomment_line_code: function() {
+        uncommentLineCode: function() {
           this._toggle_comment_code('uncomment_line');
         },
 
         /*
          * This function will comment/uncomment code (Wrapper function)
          */
-        _comment_block_code: function() {
+        commentBlockCode: function() {
           this._toggle_comment_code('block');
         },
 
@@ -3788,6 +3686,10 @@ define('tools.querytool', [
           editor.execCommand("indentLess");
         },
 
+        isQueryRunning: function () {
+          return is_query_running;
+        },
+
         /*
          * This function get explain options and auto rollback/auto commit
          * values from preferences
diff --git a/web/regression/javascript/sqleditor/keyboard_shortcuts_spec.js 
b/web/regression/javascript/sqleditor/keyboard_shortcuts_spec.js
new file mode 100644
index 00000000..d64a5043
--- /dev/null
+++ b/web/regression/javascript/sqleditor/keyboard_shortcuts_spec.js
@@ -0,0 +1,274 @@
+//////////////////////////////////////////////////////////////////////////
+//
+// pgAdmin 4 - PostgreSQL Tools
+//
+// Copyright (C) 2013 - 2017, The pgAdmin Development Team
+// This software is released under the PostgreSQL Licence
+//
+//////////////////////////////////////////////////////////////////////////
+
+import keyboardShortcuts from 'sources/sqleditor/keyboard_shortcuts';
+
+describe('the keyboard shortcuts', () => {
+  const F1_KEY = 112,
+    F5_KEY = 116,
+    F7_KEY = 118,
+    F8_KEY = 119,
+    COMMA_KEY = 188,
+    PERIOD_KEY = 190,
+    FWD_SLASH_KEY = 191;
+
+  let sqlEditorControllerSpy;
+  let event;
+  beforeEach(() => {
+    event = {
+      shift: false,
+      which: undefined,
+      preventDefault: jasmine.createSpy('preventDefault'),
+      cancelBubble: false,
+      stopPropagation: jasmine.createSpy('stopPropagation'),
+      stopImmediatePropagation: jasmine.createSpy('stopImmediatePropagation'),
+    };
+
+    sqlEditorControllerSpy = jasmine.createSpyObj('SqlEditorController', [
+      'isQueryRunning',
+      'download',
+      'explain',
+      'explainAnalyze',
+      'executeQuery',
+      'commentLineCode',
+      'uncommentLineCode',
+      'commentBlockCode',
+    ]);
+  });
+
+  describe('when the key is not handled by the function', function () {
+
+    beforeEach(() => {
+      event.which = F1_KEY;
+      keyboardShortcuts.processEvent(sqlEditorControllerSpy, event);
+    });
+
+    it('should allow event to propagate', () => {
+      expect(event.preventDefault).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('F5', () => {
+    describe('when there is no query already running', () => {
+      beforeEach(() => {
+        event.keyCode = F5_KEY;
+        keyboardShortcuts.processEvent(sqlEditorControllerSpy, event);
+      });
+
+      it('should execute the query', () => {
+        expect(sqlEditorControllerSpy.executeQuery).toHaveBeenCalled();
+      });
+
+      it('should stop event propagation', () => {
+        expect(event.preventDefault).toHaveBeenCalled();
+      });
+    });
+
+    describe('when the query is already running', () => {
+      it('does nothing', () => {
+        event.keyCode = F5_KEY;
+        sqlEditorControllerSpy.isQueryRunning.and.returnValue(true);
+
+        keyboardShortcuts.processEvent(sqlEditorControllerSpy, event);
+
+        expect(sqlEditorControllerSpy.executeQuery).not.toHaveBeenCalled();
+      });
+    });
+  });
+
+  describe('F7', () => {
+    describe('when there is not a query already running', () => {
+      beforeEach(() => {
+        event.which = F7_KEY;
+        keyboardShortcuts.processEvent(sqlEditorControllerSpy, event);
+      });
+
+      it('should explain the query plan', () => {
+        expect(sqlEditorControllerSpy.explain).toHaveBeenCalledWith(event);
+      });
+
+      expectEventPropagationToStop();
+    });
+
+    describe('when the query is already running', () => {
+      it('does nothing', () => {
+        event.keyCode = F7_KEY;
+        sqlEditorControllerSpy.isQueryRunning.and.returnValue(true);
+
+        keyboardShortcuts.processEvent(sqlEditorControllerSpy, event);
+
+        expect(sqlEditorControllerSpy.explain).not.toHaveBeenCalled();
+      });
+    });
+  });
+
+  describe('Shift+F7', () => {
+    describe('when therre is not a query already running', () => {
+      beforeEach(() => {
+        event.shiftKey = true;
+        event.which = F7_KEY;
+        keyboardShortcuts.processEvent(sqlEditorControllerSpy, event);
+      });
+
+      it('should analyze explain the query plan', () => {
+        
expect(sqlEditorControllerSpy.explainAnalyze).toHaveBeenCalledWith(event);
+      });
+
+      expectEventPropagationToStop();
+    });
+
+    describe('when the query is already running', () => {
+      it('does nothing', () => {
+        event.shiftKey = true;
+        event.which = F7_KEY;
+        sqlEditorControllerSpy.isQueryRunning.and.returnValue(true);
+
+        keyboardShortcuts.processEvent(sqlEditorControllerSpy, event);
+
+        expect(sqlEditorControllerSpy.explainAnalyze).not.toHaveBeenCalled();
+      });
+    });
+  });
+
+  describe('F8', () => {
+    describe('when there is not a query already running', () => {
+      beforeEach(() => {
+        event.which = F8_KEY;
+        keyboardShortcuts.processEvent(sqlEditorControllerSpy, event);
+      });
+
+      it('should download the query results as a CSV', () => {
+        expect(sqlEditorControllerSpy.download).toHaveBeenCalled();
+      });
+
+      it('should stop event propagation', () => {
+        expect(event.preventDefault).toHaveBeenCalled();
+      });
+    });
+
+    describe('when the query is already running', () => {
+      it('does nothing', () => {
+        event.keyCode = F8_KEY;
+        sqlEditorControllerSpy.isQueryRunning.and.returnValue(true);
+
+        keyboardShortcuts.processEvent(sqlEditorControllerSpy, event);
+
+        expect(sqlEditorControllerSpy.download).not.toHaveBeenCalled();
+      });
+    });
+  });
+
+  describe('Shift+CTRL+Comma', () => {
+    describe('when there is not a query already running', () => {
+      beforeEach(() => {
+        event.shiftKey = true;
+        event.ctrlKey = true;
+        event.which = COMMA_KEY;
+        keyboardShortcuts.processEvent(sqlEditorControllerSpy, event);
+      });
+
+      it('should comment the line', () => {
+        expect(sqlEditorControllerSpy.commentLineCode).toHaveBeenCalled();
+      });
+
+      expectEventPropagationToStop();
+    });
+
+    describe('when the query is already running', () => {
+      it('does nothing', () => {
+        event.shiftKey = true;
+        event.ctrlKey = true;
+        event.which = COMMA_KEY;
+        sqlEditorControllerSpy.isQueryRunning.and.returnValue(true);
+
+        keyboardShortcuts.processEvent(sqlEditorControllerSpy, event);
+
+        expect(sqlEditorControllerSpy.commentLineCode).not.toHaveBeenCalled();
+      });
+    });
+  });
+
+  describe('Shift+CTRL+Period', () => {
+    describe('when there is not a query already running', () => {
+      beforeEach(() => {
+        event.shiftKey = true;
+        event.ctrlKey = true;
+        event.which = PERIOD_KEY;
+        keyboardShortcuts.processEvent(sqlEditorControllerSpy, event);
+      });
+
+      it('should uncomment the line', () => {
+        expect(sqlEditorControllerSpy.uncommentLineCode).toHaveBeenCalled();
+      });
+
+      expectEventPropagationToStop();
+    });
+
+    describe('when the query is already running', () => {
+      it('does nothing', () => {
+        event.shiftKey = true;
+        event.ctrlKey = true;
+        event.which = COMMA_KEY;
+        sqlEditorControllerSpy.isQueryRunning.and.returnValue(true);
+
+        keyboardShortcuts.processEvent(sqlEditorControllerSpy, event);
+
+        
expect(sqlEditorControllerSpy.uncommentLineCode).not.toHaveBeenCalled();
+      });
+    });
+  });
+
+  describe('Shift+CTRL+/', () => {
+    describe('when there is not a query already running', () => {
+      beforeEach(() => {
+        event.shiftKey = true;
+        event.ctrlKey = true;
+        event.which = FWD_SLASH_KEY;
+        keyboardShortcuts.processEvent(sqlEditorControllerSpy, event);
+      });
+
+      it('should comment a block of code', () => {
+        expect(sqlEditorControllerSpy.commentBlockCode).toHaveBeenCalled();
+      });
+
+      expectEventPropagationToStop();
+    });
+
+    describe('when the query is already running', () => {
+      it('does nothing', () => {
+        event.shiftKey = true;
+        event.ctrlKey = true;
+        event.which = FWD_SLASH_KEY;
+        sqlEditorControllerSpy.isQueryRunning.and.returnValue(true);
+
+        keyboardShortcuts.processEvent(sqlEditorControllerSpy, event);
+
+        expect(sqlEditorControllerSpy.commentBlockCode).not.toHaveBeenCalled();
+      });
+    });
+  });
+
+  function expectEventPropagationToStop() {
+    describe('stops all event propogation', () => {
+
+      it('should cancel the bubble', () => {
+        expect(event.cancelBubble).toBe(true);
+      });
+
+      it('should prevent the default behavior', () => {
+        expect(event.preventDefault).toHaveBeenCalled();
+      });
+
+      it('should stop event propagation', () => {
+        expect(event.stopPropagation).toHaveBeenCalled();
+        expect(event.stopImmediatePropagation).toHaveBeenCalled();
+      });
+    });
+  }
+});

Reply via email to