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

Hi Hackers,

Attached is updated patch which now also includes:
Can't copy and paste row correctly if first column contains no data #7294

On Tue, Apr 5, 2022 at 5:45 PM Aditya Toshniwal <
aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:

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

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