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

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

>
>
> --
> Regards,
> Murtuza Zabuawala
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> On Thu, Jul 20, 2017 at 6:04 PM, Murtuza Zabuawala <murtuza.zabuawala@
> 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 <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