Re: [pgAdmin][RM6131] Port query tool to React

Hi Akshay,

Thank you for doing such a detailed review. Please find my comments inline
below and attached patch.

On Thu, Mar 17, 2022 at 4:05 PM Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
wrote:

> Hi Aditya
>
> Following are the review comments:
>
> *GUI:*
>
> - The Maximize/Minimize button on the panel should be consistent with
> other panels.
>
> Fixed.

>
> - Scratch Pad is missing or is there any new setting to add a scratch
> pad?
>
> Added. The layout lib currently does not have a context menu on the
header to add a panel. For now, you can use the reset layout button to add
the scratch pad again if closed. Context menu can be added separately
later. Reset layout will not refresh the page.

>
> - Press "Cmd + G" on Query Tool it opens the old search bar is it
> still valid
> - [image: Screenshot 2022-03-16 at 7.02.34 PM.png]
>
> Fixed.

>
> -
> - In case of any error, we should move the cursor to the error
> location. We are highlighting the error row but it should be scroll to that
> location in the editor.
>
> Existing behaviour. Improvement done.

>
> - Error highlighting color should be aligned with the theme, check the
> existing color.
>
> Fixed.

>
> - Error highlighting color is not cleared when running the successful
> query. Run "SELECT * from pg_class123" and then run "SELECT * from
> pg_class".
>
> Fixed.

>
> - *ToolBar buttons*:
> - When the Save button is disabled then 'Save as' should be
> disabled as well.
>
> This is not correct. A user should be allowed to "Save as" a file even if
it is saved and save button is disabled.

>
> - Open any SQL file, change some text and click on the 'Save' button,
> No notifier message has been flashed that 'File saved successfully' and the
> button does not get disabled as well.
>
> Fixed.

>
> - Format SQL not working.
>
> Fixed.

>
> - Clear Query keyboard shortcut not working.
>
> Fixed.

>
> -
> - Clicking on the 'Clear Query' menu should pop up a confirmation
> dialog 'Are you sure you wish to discard the current changes?'
>
> Fixed.

>
> - Open any SQL file, change some text, and try to open another file,
> it should pop up a confirmation dialog 'Are you sure you wish to discard
> the current changes?'
>
> Fixed.

>
> - When clicking on the New Query Tool button it is not opening the new
> query tool window.
>
> Fixed.

>
> - Shortcut Key for Replace is not correct on *Windows* (Tooltip
> showing Alt + Ctrl + F) but actual is (Shift + Ctrl + F)
>
> Fixed.

>
> - Most of the Keyboard shortcuts are not working on Windows at all.
>
> Fixed.

>
> - *New Connection Dialog:*
> - The close button should be right-aligned.
>
> Fixed.

>
> - Unable to test further because when selecting any disconnected
> server it should pop up the password dialog to connect and then fetch the
> details like databases, users, roles.
>
> Fixed.

>
> - Help buttons are disabled.
>
> The existing help button opens the query tool help. Query tool help is
already added on the toolbar.

>
> - *Query History*:
> - For some queries like 'ROLLBACK' and 'COMMIT,' Rows affected
> shows in the negative (-1).
>
> Based on existing. I have made it blank.

>
> - *Remove All* should warn the user before removing everything "Are
> you sure you wish to remove all the history? This will remove all of your
> query histories from this and other sessions for this database."
>
> Fixed.

>
> - Duration is missing. It should be there with the Date and Rows
> affected.
>
> Fixed.

>
> - *Status Bar*:
> - We should display Milliseconds as well in Query Complete.
>
> It will now display in <hr>:<min>:<sec>.<msec> format. Fixed.

>
> - Total Rows: *N of N, *I always observe the same, can you please
> change it to *N *only, Or am I missing some scenario where it is
> changed?
>
> We're fetching rows on demand. "X of N" says total X rows fetched till now
but total N are available.

>
> - *Data output*:
> - Default message "No data output......" should be shown when there
> are no rows/data, check the old behavior.
>
> Fixed.

>
> - Extra space in the JSON Editor, even if I resize it, its height is
> not adjusted. Check the second screenshot
> - [image: Screenshot 2022-03-17 at 11.29.21 AM.png] [image:
> Screenshot 2022-03-17 at 11.29.59 AM.png]
>
> Fixed.

>
> -
> - *Explain*:
> - Old content should be cleared from the panel if we run the new
> query by clicking the play button. It should show an informative message,
> check the old behavior.
>
> Fixed.

>
> - *Macros*:
> - The close button should be right-aligned.
>
> Fixed.

>
> - Help buttons are disabled.
>
> The existing help button opens the query tool help. Query tool help is
already added on the toolbar.

>
> - *View/Edit Data*:
> - Clipboard issues.
>
> Fixed.

>
> - Unable to edit table data even if the primary key is defined.
>
> Fixed.

>
> - Select All rows is missing at the left corner of the 'Data Output'
> Panel.
>
> Fixed.

>
> - Change some data and try to save if an error comes then Spinner is
> not cleared.
>
> Fixed.

>
> - Filtered Rows not working. Click on the 'Filtered Rows ...' context
> menu.
>
> Fixed.

>
> - Sort/Filter Dialog is missing.
>
> The sort/filter dialog is present and opens when you click the filter
button. I have removed the "Sort/Filter" menu item which does the same
thing.

>
> - Select any cell of the table content, click on any filter menu
> 'Filter by selection' or 'Exclude by selection' multiple times. It updates
> the query every time and adds the condition which is not there previously.
>
> Fixed.

>
> - Limit (100 rows, 500 rows ...) not working. View data of any table
> having 1000+ rows and then apply the limit.
>
> I am not able to reproduce this. Works fine.

>
> - *Themes*:
> - Check and Fix all the issues related to the theme.
> - [image: Screenshot 2022-03-17 at 1.43.01 PM.png].
> [image: Screenshot 2022-03-17 at 1.44.10 PM.png].
> -
> - [image: Screenshot 2022-03-17 at 1.44.53 PM.png]
> [image: Screenshot 2022-03-17 at 1.45.54 PM.png]
>
> Fixed.

>
> -
>
>
> *Code:*
>
> - Fix pep8 issues.
>
> Fixed.

>
> - Can we fix the *Deprecation Warnings?*
>
> I think those are coming from bootstrap. Need to check that separately.
Not related to the query tool.

> *Note: *Code review still remains, meanwhile you can start fixing the
> above issues.
>

Forgot to mention in initial mail - I have removed dependency on Snap.svg
and have written Explain SVG codes from scratch.

>
> On Wed, Mar 16, 2022 at 5:54 PM Aditya Toshniwal <
> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>
>> Hi,
>>
>> Attached is an updated one with few more improvements and fixes.
>>
>> On Wed, Mar 16, 2022 at 1:40 PM Aditya Toshniwal <
>> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>>
>>> Please find the attached patch :)
>>>
>>> On Wed, Mar 16, 2022 at 12:16 PM Akshay Joshi <
>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Aditya
>>>>
>>>> I think you forgot to attach the patch.
>>>>
>>>> On Tue, Mar 15, 2022 at 4:00 PM Aditya Toshniwal <
>>>> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi Hackers,
>>>>>
>>>>> Attached is the initial patch that migrates the SQL Editor tool to
>>>>> React based. Change highlights:
>>>>> 1. Complete rewrite to React code.
>>>>> 2. UI improvements based on suggestions and requests.
>>>>> 3. Work towards stability and performance improvement.
>>>>> 4. Keep row numbers in view when scrolling horizontally. Fixes #3989
>>>>> 5. Fixed status bar at the bottom with useful details. Fixes #3253
>>>>> 6. Relocate GIS Viewer Button to the Left Side of Results Table. Fixed
>>>>> #6830
>>>>> 7. Allow to remove single history records. Refs #4113
>>>>> 8. Macros usability improvements. Ref #6969
>>>>> 9. Connection bar visibility issue. Fixes #7188
>>>>> 10. Query tool layout issues. Fixes #6725
>>>>>
>>>>> Please note, there are still few minor niggles at some places but the
>>>>> patch qualified to be reviewed. We will need a good amount of time to test
>>>>> this properly. So, I am sending the feature patch. JS test cases and
>>>>> documentation patches will follow soon.
>>>>>
>>>>> Please review.
>>>>>
>>>>> [image: image.png]
>>>>> --
>>>>> Thanks,
>>>>> Aditya Toshniwal
>>>>> pgAdmin Hacker | Software Architect | *edbpostgres.com*
>>>>> <http://edbpostgres.com>
>>>>> "Don't Complain about Heat, Plant a TREE"
>>>>>
>>>>
>>>>
>>>> --
>>>> *Thanks & Regards*
>>>> *Akshay Joshi*
>>>> *pgAdmin Hacker | Principal Software Architect*
>>>> *EDB Postgres <http://edbpostgres.com>*
>>>>
>>>> *Mobile: +91 976-788-8246*
>>>>
>>>
>>>
>>> --
>>> Thanks,
>>> Aditya Toshniwal
>>> pgAdmin Hacker | Software Architect | *edbpostgres.com*
>>> <http://edbpostgres.com>
>>> "Don't Complain about Heat, Plant a TREE"
>>>
>>
>>
>> --
>> Thanks,
>> Aditya Toshniwal
>> pgAdmin Hacker | Software Architect | *edbpostgres.com*
>> <http://edbpostgres.com>
>> "Don't Complain about Heat, Plant a TREE"
>>
>
>
> --
> *Thanks & Regards*
> *Akshay Joshi*
> *pgAdmin Hacker | Principal Software Architect*
> *EDB Postgres <http://edbpostgres.com>*
>
> *Mobile: +91 976-788-8246*
>

--
Thanks,
Aditya Toshniwal
pgAdmin Hacker | Software Architect | *edbpostgres.com*
<http://edbpostgres.com>
"Don't Complain about Heat, Plant a TREE"