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

On Fri, Jul 21, 2017 at 4:08 PM, Robert Eckhardt <reckhardt(at)pivotal(dot)io>
wrote:

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

I think those might be easier to remember than the current keys.

I'm not sure it makes sense to have a single key for line commenting and
uncommenting though. Whilst it's certainly cleaner, what should the
behaviour be for a block such as:

CREATE TABLE foo (
id serial,
data text
);

-- Index required on data for finding Wumpus' quickly
CREATE INDEX foo_idx ON foo (data);

Is that likely to be a problem, or am I overthinking it?

>
> -- Rob
>
> On Fri, Jul 21, 2017 at 7:09 AM, Murtuza Zabuawala <murtuza.zabuawala@
> enterprisedb.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
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>

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

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