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

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.patel@enterprisedb.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.zabuawala@enterprisedb.com> wrote:

--

Regards,

Murtuza Zabuawala
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---------- Forwarded message ----------
From: Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.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.joshi@enterprisedb.com>
Cc: Dave Page <dpage@pgadmin.org>, pgadmin-hackers <pgadmin-hackers@postgresql.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.joshi@enterprisedb.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.zabuawala@enterprisedb.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@pgadmin.org> wrote:

Hi,

On Wed, Aug 17, 2016 at 11:19 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.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@postgresql.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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

--

Akshay Joshi

Principal Software Engineer 

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