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

Thanks, applied.

On Thu, Jul 20, 2017 at 9:31 PM, Sarah McAlear <smcalear(at)pivotal(dot)io> wrote:

> 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(dot)zabuawala(at)enterprisedb(dot)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/pga
>>>>>>>>> dmin4/lib/python2.7/site-packages/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/pga
>>>>>>>>> dmin4/lib/python2.7/site-packages/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
>>>>
>>>
>>>
>>
>

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company