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

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/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
>>
>
>