Re: PATCH: SlickGrid integration in query tool (pgAdmin4)

Hi Murtuza

I have tested the functionality before reviewing the actual code, below are
my review comments

*Functionality related comments*:

- Filter "By Selection" and "Exclude Selection" not working.
- Increase the font size from 8pt to 9pt and also make the table
header's in bold.
- User won't be able to enter/copy the data directly from the cell. With
current implementation user has to double click on cell then one data
editor popped up where user can enter/copy data and will have to press
"save" or "cancel" button. Then will have to double click again on the cell
where he/she want's to paste and will have to press "save" or "cancel"
button.
- Slick grid not rendered properly when changing tabs or resize "Data
output" docker panel. Please refer "Error-1.png"
- *Steps to reproduce*: View table data with more than 100 rows,
scroll down a bit, now either shift tab and back again to "Data output"
panel OR resize the "Data output" docker panel.
- Slick grid is not refreshing properly when deleted multiple rows.
- Error Message is not cleared from "Messages" tab even after successful
transaction.
- *Steps to reproduce*: Generate error by saving some wrong data and
check "Messages" tab, now correct the value and save the data again. Data
gets saved, but "Messages" tab still show error message.
- Paste rows button gets disabled once it is clicked, as per me it
should be enabled if some rows are copied to clipboard.
- Facing weird issue for some tables, data is not being saved even if I
have given the correct values. Mostly it happens with NOT NULL columns.
- *Steps to reproduce*: View Data of the table where there are
columns with NOT NULL constraint. I have tested it for "public.jobhist"
table of PPAS-9.5. Add new row and insert values for primary
key columns,
but don't insert any value for NOT NULL column. Now click on
Save button it
will throw an error "null value in column <xyz> violates not-null
constraint". Now insert the proper value for that column and try to save
it, still it will give the same error.
- Extra space has been added for the drop down menu of query tool
toolbar. Please refer "Before_Slickgrid.png" and "After_Slickgrid.png"

On Tue, Aug 23, 2016 at 4:00 PM, Murtuza Zabuawala <
murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:

> Hi,
>
> PFA minor add-on patch on previous Slickgrid v2 patch to remove console
> log messages from JS..
>
> --
> Regards,
> Murtuza Zabuawala
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> On Tue, Aug 23, 2016 at 3:47 PM, Murtuza Zabuawala <murtuza.zabuawala@
> enterprisedb.com> wrote:
>
>> Hi,
>>
>> PFA updated patch & comments inline.
>>
>> --
>> Regards,
>> Murtuza Zabuawala
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>> On Thu, Aug 18, 2016 at 9:16 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> Hi,
>>>
>>> On Wed, Aug 17, 2016 at 11:19 AM, Murtuza Zabuawala <
>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi,
>>>>
>>>> PFA initial patch for SlickGrid integration in query tool.
>>>>
>>>
>>> This is looking awesome! I think there are some tweaks to make, but
>>> performance-wise, it's blowing away the old code. My tests show it's even
>>> faster than you're seeing; I'm seeing ~3x performance rendering 21K rows
>>> for example, vs. the first 2K of the same dataset on page 1 with the old
>>> code - plus, the rendered HTML isn't so huge that it slows everything else
>>> down to being unusable!
>>>
>>> Can you look at the following issues please?
>>>
>>> 1) The colour for a selected row should be #eeeeee
>>>
>> Done
>>
>>> 2) If there's a horizontal scroll bar (in Chrome on OSX, possibly
>>> others), then it partially eclipses the new blank row in View Data mode.
>>> The vertical scroll needs to have the height of the horizontal scrollbar
>>> added.
>>>
>> This issue needed small code hack in SlickGrid code itself. (Ashesh help
>> me with issue as all the row sizes were dynamic (in terms of pixels)
>> & calculated as we display rows on grid canvas, so we can not use CSS/JS to
>> tweak it)
>> I have attached separate patch for Slickgrid changes, we have to apply
>> this patch whenever we update SlickGrid version in future.
>>
>>>
>>>
>> 3) If the window is resized, the grid doesn't redraw to the new size.
>>>
>> Done
>>
>>> 4) I cannot leave a Primary Key field blank so that it picks up a
>>> default value (I get "Primary key columns cannot be null.")
>>>
>> Done
>>
>>> 5) When editing text fields with multiple lines, the CR/LFs are lost. Do
>>> we need a custom editor for this (textarea?).
>>>
>> Added custom detached editor [ which supports all keyboard navigation :-)
>> ].
>>
>>>
>>> 6) The tooltips should probably include <pre> tags around the content
>>> otherwise the formatting is messed up and becomes unreadable.
>>>
>> This is not permitted, Only text to be displayed is allowed, it will not
>> parse html tags in and show them as is.
>> Ref: http://www.w3schools.com/tags/att_global_title.asp
>>
>> Thanks!
>>>
>>>
>>>>
>>>> My finding on grid rendering performance with each grid,
>>>>
>>>> Tested on PG9.5 (sql attached).
>>>>
>>>> Please review.
>>>>
>>>> --
>>>> Regards,
>>>> Murtuza Zabuawala
>>>> EnterpriseDB: http://www.enterprisedb.com
>>>> The Enterprise PostgreSQL Company
>>>>
>>>>
>>>> --
>>>> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
>>>> To make changes to your subscription:
>>>> http://www.postgresql.org/mailpref/pgadmin-hackers
>>>>
>>>>
>>>
>>>
>>> --
>>> Dave Page
>>> Blog: http://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EnterpriseDB UK: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>
>>
>
>
> --
> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers
>
>

--
*Akshay Joshi*
*Principal Software Engineer *

*Phone: +91 20-3058-9517Mobile: +91 976-788-8246*