Thanks, applied. On Thu, Jul 20, 2017 at 9:31 PM, Sarah McAlear <smcal...@pivotal.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 <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.page.find_by_xpath("//*[@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.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