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

Thanks, patch applied.

On Mon, Aug 29, 2016 at 3:30 PM, Murtuza Zabuawala <
murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:

> Hi,
>
> PFA updated patch.
>
> --
> Regards,
> Murtuza Zabuawala
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> On Mon, Aug 29, 2016 at 1:18 PM, Neel Patel <neel(dot)patel(at)enterprisedb(dot)com>
> wrote:
>
>> 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.
>>
>> Done
>
>>
>> - When we click on new blank row and select "By Selection" then it
>> throws 'javascript' exception.
>>
>> Uncaught TypeError: Cannot read property 'id' of undefined
>>
> Done
>
>>
>> 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*
>>>>
>>>
>>>
>>>
>>
>

--
*Akshay Joshi*
*Principal Software Engineer *

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