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

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.zabuawala@
> 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(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
>