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

Hi Murtuza,

All the review comments given by Akshay are fixed. Below are some minor
observations.

- Add two new rows with valid data. Select one row and delete that row.
Now "Save" button should be enabled because we still have one valid new row
to save the data.
- When we click on new blank row and select "By Selection" then it
throws 'javascript' exception.

Uncaught TypeError: Cannot read property 'id' of undefined

Thanks,
Neel patel

On Mon, Aug 29, 2016 at 10:43 AM, Murtuza Zabuawala <
murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:

>
> --
> Regards,
> Murtuza Zabuawala
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> ---------- Forwarded message ----------
> From: Murtuza Zabuawala <murtuza(dot)zabuawala(at)enterprisedb(dot)com>
> Date: Fri, Aug 26, 2016 at 8:27 PM
> Subject: Re: [pgadmin-hackers] PATCH: SlickGrid integration in query tool
> (pgAdmin4)
> To: Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
> Cc: Dave Page <dpage(at)pgadmin(dot)org>, pgadmin-hackers <
> pgadmin-hackers(at)postgresql(dot)org>
>
>
> Hi,
>
> PFA updated patch for SlickGrid.
>
> *Note:* This patch contains our own custom Slickgrid changes in SlickGrid
> no need to apply patch sent in last email.
>
>
> --
> Regards,
> Murtuza Zabuawala
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> On Wed, Aug 24, 2016 at 6:27 PM, Akshay Joshi <
> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>
>> 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.
>>
>> Done
>
>>
>> - Increase the font size from 8pt to 9pt and also make the table
>> header's in bold.
>>
>> Done
>
>>
>> - 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.
>>
>> Copying from selected Cell is done. But for now you have edit target cell
> & paste the data in editor.
> I will create TODO ticket for this pasting data without opening editor in
> to cell as it requires some time.
>
>>
>> - 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.
>>
>> Done
>
>>
>> - Slick grid is not refreshing properly when deleted multiple rows.
>>
>> Done
>
>>
>> - 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.
>>
>> Done
>
>>
>> - Paste rows button gets disabled once it is clicked, as per me it
>> should be enabled if some rows are copied to clipboard.
>>
>> Done
>
>>
>> - 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.
>>
>> Done
>
>>
>> - Extra space has been added for the drop down menu of query tool
>> toolbar. Please refer "Before_Slickgrid.png" and "After_Slickgrid.png"
>>
>> Done
>
>>
>> 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(dot)zabuawala(at)enterprisedb(dot)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*
>>
>
>
>