I hope you're overthinking. Having not implemented it yet I'm not sure. Block commenting will look like the below
/* CREATE TABLE foo ( id serial, data text ); -- Index required on data for finding Wumpus' quickly CREATE INDEX foo_idx ON foo (data); */ -- Rob On Fri, Jul 21, 2017 at 11:13 AM, Dave Page <dp...@pgadmin.org> wrote: > > > On Fri, Jul 21, 2017 at 4:08 PM, Robert Eckhardt <reckha...@pivotal.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.zabuaw...@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 <reckha...@pivotal.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.zabuaw...@enterprisedb.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 <reckha...@pivotal.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.zabuaw...@enterprisedb.com> wrote: >>>>> >>>>>> >>>>>> On Thu, Jul 20, 2017 at 10:29 PM, Dave Page <dp...@pgadmin.org> >>>>>> wrote: >>>>>> >>>>>>> Hi >>>>>>> >>>>>>> On Thu, Jul 20, 2017 at 3:33 PM, Murtuza Zabuawala < >>>>>>> murtuza.zabuaw...@enterprisedb.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.zabuaw...@enterprisedb.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 <dp...@pgadmin.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.zabuaw...@enterprisedb.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 <dp...@pgadmin.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.p >>>>>>>>>>>> age.find_by_xpath("//*[@id='btn-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.zabuaw...@enterprisedb.com> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Thank you Dave. >>>>>>>>>>>>> >>>>>>>>>>>>> On Wed, Jul 19, 2017 at 4:17 PM, Dave Page <dp...@pgadmin.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.zabuaw...@enterprisedb.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.zabuaw...@enterprisedb.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 < >>>>>>>>>>>>>>>> dp...@pgadmin.org> wrote: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Hi >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> On Mon, Jul 17, 2017 at 10:31 AM, Murtuza Zabuawala < >>>>>>>>>>>>>>>>> murtuza.zabuaw...@enterprisedb.com> wrote: >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Hi Dave, >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> On Mon, Jul 17, 2017 at 2:33 PM, Dave Page < >>>>>>>>>>>>>>>>>> dp...@pgadmin.org> wrote: >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Hi >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> On Wed, Jul 12, 2017 at 1:16 PM, Murtuza Zabuawala < >>>>>>>>>>>>>>>>>>> murtuza.zabuaw...@enterprisedb.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 >
