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

I wouldn't say wrong, it just wasn't what I was expecting.

I guess I'd like to hear what others are expecting. If I had my way we
would use

Ctrl+/ single line comment and uncomment (prepend with --)
Ctrl+Shift+/ block comment and uncomment (bracket with /* and */)

where Ctrl == command on Mac.

-- Rob

On Fri, Jul 21, 2017 at 7:09 AM, Murtuza Zabuawala <
murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:

> Hi Robert,
>
> I mean rather than using cmd key for Mac and CTRL key for Windows/Linux, I
> used "CTRL" key for all the platforms.
> And regarding choosing comma & period keys, they all are near each to each
> other so user can remember them easily.
>
> Let me know If my thinking was wrong for shortcut keys, I'll change them
> accordingly and send new patch.
>
> On Fri, Jul 21, 2017 at 4:25 PM, Robert Eckhardt <reckhardt(at)pivotal(dot)io>
> wrote:
>
>> I'm not sure what you mean by across platforms. Do you mean that those
>> are the keyboard shortcuts in pgAdmin 3?
>>
>> Rob
>>
>> On Jul 21, 2017 4:42 AM, "Murtuza Zabuawala" <
>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>
>> Hi Robert,
>>
>> Just to make shortcut keys uniform across all the platforms.
>>
>> On Fri, Jul 21, 2017 at 1:25 AM, 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
>>>>>
>>>>
>>>>
>>>
>>
>>
>