Subject: Issue with SlickGrid

From: Joao Pedro De Almeida Pereira <[email-masked]>
Date: Wed, 26 Apr 2017 16:53:08 -0400

Hello Hackers,

While doing some changes to the Query Results we found out that there was a
issue with Slick grid.

The issue that we found was with the CellSelectModel, behaved differently
when pressing Ctrl and Command(Mac). We created a PR
<https://github.com/6pac/SlickGrid/pull/100> with the change to changes the
behavior of the plugin.

When this PR is applied to the SlickGrid library we need to apply it to the
current version of SlickGrid that we have vendorized.
According to the libraries.txt file we are in version 2.2.4 of the library
but a diff between our code and the libraries version 2.2.4 shows
differences in the code.

Did we do any change to SlickGrid library that is vendorized? Or is just
the information in libraries.txt that is incorrect?
Does anyone know any problem if we bump the version of SlickGrid to the
newer version after the PR is applied?

Thanks
Joao

---

From: Murtuza Zabuawala <[email-masked]>
Date: Thu, 27 Apr 2017 12:09:36 +0530

Hello Joao,

Yes, We made some changes in SlickGrid library when we integrated it into
Query tool.

*Issue:* Last row from the query result set was not displaying correctly in
query tool when we have scrollbar in grid.

The row hight/width pixel size calculations is done inside SlickGrid
javascript code, Though we tried solve it through CSS but we had no luck,
so we had no other choice but to do it in library it self.

The changes were,
1) "getDataLengthIncludingAddNew()" function (slick.grid.js) to add two new
rows instead of one when user add values into row (one row is dummy & not
visible to user so that it displays last row correctly)
2) Other change was done into "appendRowHtml()" function to calculating the
correct number of rows in SlickGrid result as we have added our own custom
row as mentioned earlier.
3) Abbreviated long CSS classes as mentioed in README file.

Apologies we missed to update this change in README.

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

On Thu, Apr 27, 2017 at 2:23 AM, Joao Pedro De Almeida Pereira <
[email-masked]> wrote:

> Hello Hackers,
>
> While doing some changes to the Query Results we found out that there was
> a issue with Slick grid.
>
> <https://github.com/6pac/SlickGrid/pull/100> with the change to changes
> the behavior of the plugin.
>
> When this PR is applied to the SlickGrid library we need to apply it to
> the current version of SlickGrid that we have vendorized.

---

From: Murtuza Zabuawala <[email-masked]>
Date: Thu, 27 Apr 2017 12:16:07 +0530

+++
Reference:
https://www.postgresql.org/message-id/CAKKotZRjqbKAZev81Zk78nikDVXqLKEDV5r%2BsW8Me31Gpzrm_A%40mail.gmail.com

On Thu, Apr 27, 2017 at 12:09 PM, Murtuza Zabuawala <
[email-masked]> wrote:

> *Issue:* Last row from the query result set was not displaying correctly
> in query tool when we have scrollbar in grid.
>
> 1) "getDataLengthIncludingAddNew()" function (slick.grid.js) to add two
> new rows instead of one when user add values into row (one row is dummy &
> not visible to user so that it displays last row correctly)
> 2) Other change was done into "appendRowHtml()" function to calculating
> the correct number of rows in SlickGrid result as we have added our own
> custom row as mentioned earlier.
>> According to the libraries.txt file we are in version 2.2.4 of the
>> library but a diff between our code and the libraries version 2.2.4 shows
>> differences in the code.
>>

---

From: Joao Pedro De Almeida Pereira <[email-masked]>
Date: Thu, 27 Apr 2017 11:12:08 +0000

Hello Murtuza,
Thanks for the explanation. Based on what you said it looks like a bug in
the library, have you guys considered sending a PR to it?

Thanks

On Thu, Apr 27, 2017, 2:46 AM Murtuza Zabuawala <
[email-masked]> wrote:

>>> While doing some changes to the Query Results we found out that there
>>> was a issue with Slick grid.
>>>
>>> The issue that we found was with the CellSelectModel, behaved
>>> differently when pressing Ctrl and Command(Mac). We created a PR

---

From: Murtuza Zabuawala <[email-masked]>
Date: Thu, 27 Apr 2017 17:43:14 +0530

No, we didn't.

On Thu, Apr 27, 2017 at 4:42 PM, Joao Pedro De Almeida Pereira <
[email-masked]> wrote:

> On Thu, Apr 27, 2017, 2:46 AM Murtuza Zabuawala <murtuza.zabuawala@
> enterprisedb.com> wrote:
>
>> +++
>> Reference: https://www.postgresql.org/message-id/
>> CAKKotZRjqbKAZev81Zk78nikDVXqLKEDV5r%2BsW8Me31Gpzrm_A%40mail.gmail.com
>>
>> --
>> Regards,
>> Murtuza Zabuawala
>> On Thu, Apr 27, 2017 at 12:09 PM, Murtuza Zabuawala <murtuza.zabuawala@
>> enterprisedb.com> wrote:
>>
>>> Hello Joao,
>>>
>>> Yes, We made some changes in SlickGrid library when we integrated it
>>> into Query tool.
>>>
>>> *Issue:* Last row from the query result set was not displaying
>>> correctly in query tool when we have scrollbar in grid.
>>>
>>>> <https://github.com/6pac/SlickGrid/pull/100> with the change to
>>>> changes the behavior of the plugin.
>>>>
>>>> Did we do any change to SlickGrid library that is vendorized? Or is
>>>> just the information in libraries.txt that is incorrect?

---

From: Joao Pedro De Almeida Pereira <[email-masked]>
Date: Thu, 27 Apr 2017 11:49:07 -0400

Hi Hackers,

We found that the latest version of SlickGrid fixes the scrollbar issue. We
have upgraded it to the latest version in our vendor directory and updated
the tests accordingly in the attached patch.

We didn't apply any of the custom changes that were previously added.
Please validate that the memory issues that were referenced in the README
file are solved with the latest version of SlickGrid. If we can avoid
changing the code of the libraries that we use, it will be far easier to
continue to upgrade in the future. We will need to upgrade the version of
SlickGrid again soon, once they approve our pull request
<https://github.com/6pac/SlickGrid/pull/100>.

Thanks,
Joao & Matt

On Thu, Apr 27, 2017 at 8:13 AM, Murtuza Zabuawala <
[email-masked]> wrote:

>>> Reference: https://www.postgresql.org/message-id/CAKKotZRjqb
>>> KAZev81Zk78nikDVXqLKEDV5r%2BsW8Me31Gpzrm_A%40mail.gmail.com
>>>
>>> --
>>> Regards,
>>> Murtuza Zabuawala
>>>> 1) "getDataLengthIncludingAddNew()" function (slick.grid.js) to add
>>>> two new rows instead of one when user add values into row (one row is dummy
>>>> & not visible to user so that it displays last row correctly)
>>>>> When this PR is applied to the SlickGrid library we need to apply it
>>>>> to the current version of SlickGrid that we have vendorized.
>>>>> Does anyone know any problem if we bump the version of SlickGrid to
>>>>> the newer version after the PR is applied?
>>>>>
>>>>> Thanks
>>>>> Joao
>>>>>
>>>>
>>>>
>>>
>

-- 
Sent via pgadmin-hackers mailing list ([email-masked])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

---

From: Dave Page <[email-masked]>
Date: Thu, 27 Apr 2017 16:53:25 +0100

Thanks Joao. Murtuza, can you review this please?

On Thu, Apr 27, 2017 at 4:49 PM, Joao Pedro De Almeida Pereira <
[email-masked]> wrote:

> Hi Hackers,
>
> We found that the latest version of SlickGrid fixes the scrollbar issue.
> We have upgraded it to the latest version in our vendor directory and
> updated the tests accordingly in the attached patch.
>
> On Thu, Apr 27, 2017 at 8:13 AM, Murtuza Zabuawala <murtuza.zabuawala@
> enterprisedb.com> wrote:
>
>> No, we didn't.
>>
>> --
>> Regards,
>> Murtuza Zabuawala
>>> Thanks for the explanation. Based on what you said it looks like a bug
>>> in the library, have you guys considered sending a PR to it?
>>>
>>> Thanks
>>>
>>>>> 2) Other change was done into "appendRowHtml()" function to
>>>>> calculating the correct number of rows in SlickGrid result as we have added
>>>>> our own custom row as mentioned earlier.

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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

---

From: Murtuza Zabuawala <[email-masked]>
Date: Wed, 3 May 2017 09:56:02 +0530

Hi Jao,

I found few minor issues,

1)
Individual cell selection is not visible,
This will be useful to user when user wants to use "By Selection" &
"Exclude Selection" filter option.
(Even if active cell is not visible to user, above filters are working
though when we click on any cell)

2)
Column header does not look good due bottom padding.
Refer screenshot.

3)
Select All color selection issue,

try running,
select * from pg_class

Now scroll horizontally till the last column and again scroll back to first
column,
you will see that one of the column gets the blue color as if it is
selected by user.
Refer screenshot.

Rest of the patch looks good to me.

On Thu, Apr 27, 2017 at 9:23 PM, Dave Page <[email-masked]> wrote:

>>>>>>> While doing some changes to the Query Results we found out that
>>>>>>> there was a issue with Slick grid.
>>>>>>>

---

From: Sarah McAlear <[email-masked]>
Date: Fri, 5 May 2017 17:44:18 -0400

Hi Murtuza!

1)
> though when we click on any cell)

Good catch. We'll fix that.

2)

> Refer screenshot.

We think that we actually fixed that with a patch we are currently working
on and will be submitting next week.

3)

> try running,
> select * from pg_class
> Now scroll horizontally till the last column and again scroll back to
> first column,

Good catch with this one too! We actually think this is a bug that may have
been introduced before this. We were able to replicate the same issue
without the patch being applied. We're going to take this bug on in our
backlog and include the fix in the upcoming patch we just mentioned.
This only seems to be a bug when the table has more columns than fits on
the viewport. We think we have an idea of how to fix this.

Thanks!
Matt & Sarah

On Wed, May 3, 2017 at 12:26 AM, Murtuza Zabuawala <
[email-masked]> wrote:

> Now scroll horizontally till the last column and again scroll back to
> first column,
>>>>>>>> When this PR is applied to the SlickGrid library we need to apply
>>>>>>>> it to the current version of SlickGrid that we have vendorized.