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

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