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

Hi,

PFA patch which will add functionality to allow user to comment/uncomment code in query editor.

RM#2456

--

Regards,

Murtuza Zabuawala
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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.zabuawala@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 <dpage@pgadmin.org> wrote:

Hi

On Mon, Jul 17, 2017 at 10:31 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

On Mon, Jul 17, 2017 at 2:33 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Wed, Jul 12, 2017 at 1:16 PM, Murtuza Zabuawala <murtuza.zabuawala@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

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_tool_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='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/pgadmin4/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_tool_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-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.zabuawala@enterprisedb.com> wrote:

Thank you Dave.

On Wed, Jul 19, 2017 at 4:17 PM, Dave Page <dpage@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.zabuawala@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.zabuawala@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 <dpage@pgadmin.org> wrote:

Hi

On Mon, Jul 17, 2017 at 10:31 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

On Mon, Jul 17, 2017 at 2:33 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Wed, Jul 12, 2017 at 1:16 PM, Murtuza Zabuawala <murtuza.zabuawala@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

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@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_tool_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='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/pgadmin4/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_tool_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-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.zabuawala@enterprisedb.com> wrote:

Thank you Dave.

On Wed, Jul 19, 2017 at 4:17 PM, Dave Page <dpage@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.zabuawala@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.zabuawala@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 <dpage@pgadmin.org> wrote:

Hi

On Mon, Jul 17, 2017 at 10:31 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

On Mon, Jul 17, 2017 at 2:33 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Wed, Jul 12, 2017 at 1:16 PM, Murtuza Zabuawala <murtuza.zabuawala@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

Hi Dave,

I am working on this, will send you patch soon.

On Thu, Jul 20, 2017 at 5:53 PM, Dave Page <dpage@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.zabuawala@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 <dpage@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_tool_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='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/pgadmin4/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_tool_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-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.zabuawala@enterprisedb.com> wrote:

Thank you Dave.

On Wed, Jul 19, 2017 at 4:17 PM, Dave Page <dpage@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.zabuawala@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.zabuawala@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 <dpage@pgadmin.org> wrote:

Hi

On Mon, Jul 17, 2017 at 10:31 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

On Mon, Jul 17, 2017 at 2:33 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Wed, Jul 12, 2017 at 1:16 PM, Murtuza Zabuawala <murtuza.zabuawala@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

Hi Murtuza,

Can the code related to Menu KeyEvents be moved out into a separate file say 'menu_actions.js' if feasible ?

As the more functionality will be added to sqleditor, codemirror and its file menu in future, the codebase will eventually increase.

It will ease writing jasmine test cases as well.

Thanks,

Surinder

On Thu, Jul 20, 2017 at 8:03 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.

--

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@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.zabuawala@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 <dpage@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_tool_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='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/pgadmin4/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_tool_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-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.zabuawala@enterprisedb.com> wrote:

Thank you Dave.

On Wed, Jul 19, 2017 at 4:17 PM, Dave Page <dpage@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.zabuawala@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.zabuawala@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 <dpage@pgadmin.org> wrote:

Hi

On Mon, Jul 17, 2017 at 10:31 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

On Mon, Jul 17, 2017 at 2:33 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Wed, Jul 12, 2017 at 1:16 PM, Murtuza Zabuawala <murtuza.zabuawala@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

On Thu, Jul 20, 2017 at 10:29 PM, Dave Page <dpage@pgadmin.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.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@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.zabuawala@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 <dpage@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_tool_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='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/pgadmin4/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_tool_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-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.zabuawala@enterprisedb.com> wrote:

Thank you Dave.

On Wed, Jul 19, 2017 at 4:17 PM, Dave Page <dpage@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.zabuawala@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.zabuawala@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 <dpage@pgadmin.org> wrote:

Hi

On Mon, Jul 17, 2017 at 10:31 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

On Mon, Jul 17, 2017 at 2:33 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Wed, Jul 12, 2017 at 1:16 PM, Murtuza Zabuawala <murtuza.zabuawala@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

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.zabuawala@enterprisedb.com> wrote:

On Thu, Jul 20, 2017 at 10:29 PM, Dave Page <dpage@pgadmin.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.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@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.zabuawala@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 <dpage@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_tool_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='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/pgadmin4/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_tool_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-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.zabuawala@enterprisedb.com> wrote:

Thank you Dave.

On Wed, Jul 19, 2017 at 4:17 PM, Dave Page <dpage@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.zabuawala@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.zabuawala@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 <dpage@pgadmin.org> wrote:

Hi

On Mon, Jul 17, 2017 at 10:31 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

On Mon, Jul 17, 2017 at 2:33 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Wed, Jul 12, 2017 at 1:16 PM, Murtuza Zabuawala <murtuza.zabuawala@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

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 <reckhardt@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.zabuawala@enterprisedb.com> wrote:

On Thu, Jul 20, 2017 at 10:29 PM, Dave Page <dpage@pgadmin.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.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@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.zabuawala@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 <dpage@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_tool_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='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/pgadmin4/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_tool_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-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.zabuawala@enterprisedb.com> wrote:

Thank you Dave.

On Wed, Jul 19, 2017 at 4:17 PM, Dave Page <dpage@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.zabuawala@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.zabuawala@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 <dpage@pgadmin.org> wrote:

Hi

On Mon, Jul 17, 2017 at 10:31 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

On Mon, Jul 17, 2017 at 2:33 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Wed, Jul 12, 2017 at 1:16 PM, Murtuza Zabuawala <murtuza.zabuawala@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