Re: [pgAdmin4][Patch]: Allow user to Comment/Uncomment code in query editor

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 <reckhardt(at)pivotal(dot)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 <dpage(at)pgadmin(dot)org> wrote:
>>
>>> Hi
>>>
>>> On Thu, Jul 20, 2017 at 3:33 PM, Murtuza Zabuawala <
>>> murtuza(dot)zabuawala(at)enterprisedb(dot)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(dot)zabuawala(at)enterprisedb(dot)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 <dpage(at)pgadmin(dot)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(dot)zabuawala(at)enterprisedb(dot)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 <dpage(at)pgadmin(dot)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("//*[(at)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(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>>>>
>>>>>>>>> Thank you Dave.
>>>>>>>>>
>>>>>>>>> On Wed, Jul 19, 2017 at 4:17 PM, Dave Page <dpage(at)pgadmin(dot)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(dot)zabuawala(at)enterprisedb(dot)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(dot)zabuawala(at)enterprisedb(dot)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 <dpage(at)pgadmin(dot)org>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Mon, Jul 17, 2017 at 10:31 AM, Murtuza Zabuawala <
>>>>>>>>>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Dave,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Mon, Jul 17, 2017 at 2:33 PM, Dave Page <dpage(at)pgadmin(dot)org
>>>>>>>>>>>>>> > wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Wed, Jul 12, 2017 at 1:16 PM, Murtuza Zabuawala <
>>>>>>>>>>>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)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
>>>
>>
>>
>