Re: [pgAdmin4][PATCH] Improvements to Query Results Grid User Experience

Hello Dave,

We created the previous patches using the command recommended in the
pgAdmin website, but apparently the diff doesn't work correctly when you
have binary files in the commit, like images.

We regenerated the patches using the command with the addition of --binary

git diff e0d5cf6b ac65f43b --binary > 6-adapt-slickgrid-to-pgadmin.patch

After that, we applied them in master and this was the output:

± jp+si |master → origin {9} ✓| → git apply 1-upgrade-slickgrid.patch
1-upgrade-slickgrid.patch:120: trailing whitespace.

1-upgrade-slickgrid.patch:129: trailing whitespace.

1-upgrade-slickgrid.patch:138: trailing whitespace.

1-upgrade-slickgrid.patch:147: trailing whitespace.

1-upgrade-slickgrid.patch:156: trailing whitespace.

warning: squelched 3856 whitespace errors
warning: 3861 lines add whitespace errors.

maujer in ~/workspace/pgadmin4
± jp+si |master → origin {9} U:17 ?:7 ✗| → git apply
2-select-cells-improvements.patch

maujer in ~/workspace/pgadmin4
± jp+si |master → origin {9} U:35 ?:12 ✗| → git apply 3-drag-and-select.patch

maujer in ~/workspace/pgadmin4
± jp+si |master → origin {9} U:41 ?:13 ✗| → git apply 4-remove-checkboxes.patch

maujer in ~/workspace/pgadmin4
± jp+si |master → origin {9} U:41 ?:14 ✗| → git apply
5-improvements-to-range-selection.patch
5-improvements-to-range-selection.patch:647: trailing whitespace.
function scrollColumnIntoView(columnIndex) {
5-improvements-to-range-selection.patch:648: trailing whitespace.
var colspan = getColspan(row, columnIndex);
5-improvements-to-range-selection.patch:649: trailing whitespace.

5-improvements-to-range-selection.patch:650: trailing whitespace.
var left = columnPosLeft[columnIndex],
5-improvements-to-range-selection.patch:651: trailing whitespace.
right = columnPosRight[columnIndex + (colspan > 1 ? colspan - 1 : 0)],
warning: squelched 15 whitespace errors
warning: 20 lines add whitespace errors.

maujer in ~/workspace/pgadmin4
± jp+si |master → origin {9} U:41 ?:16 ✗| → git apply
6-adapt-slickgrid-to-pgadmin.patch

​

Maybe we should update the website with this information to help future
committers.

Thanks
Joao & Shruti

On Fri, May 26, 2017 at 10:35 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Hi,
>
> These patches really don't look like they were created in the normal way:
>
> (pgadmin4)snake:web dpage$ git apply ~/Downloads/1-upgrade-slickgrid.patch
> /Users/dpage/Downloads/1-upgrade-slickgrid.patch:126: trailing whitespace.
>
> /Users/dpage/Downloads/1-upgrade-slickgrid.patch:129: trailing whitespace.
> where the browser copies/pastes the serialized data.
> /Users/dpage/Downloads/1-upgrade-slickgrid.patch:130: trailing whitespace.
>
> /Users/dpage/Downloads/1-upgrade-slickgrid.patch:154: trailing whitespace.
>
> /Users/dpage/Downloads/1-upgrade-slickgrid.patch:165: trailing whitespace.
>
> error: cannot apply binary patch to
> 'web/pgadmin/static/vendor/slickgrid/images/CheckboxN.png' without
> full index line
> error: web/pgadmin/static/vendor/slickgrid/images/CheckboxN.png: patch
> does not apply
> error: cannot apply binary patch to
> 'web/pgadmin/static/vendor/slickgrid/images/CheckboxY.png' without
> full index line
> error: web/pgadmin/static/vendor/slickgrid/images/CheckboxY.png: patch
> does not apply
> error: patch failed:
> web/pgadmin/static/vendor/slickgrid/plugins/slick.cellselectionmodel.js:94
> error: web/pgadmin/static/vendor/slickgrid/plugins/slick.
> cellselectionmodel.js:
> patch does not apply
> error: patch failed: web/pgadmin/static/vendor/
> slickgrid/slick.grid.css:102
> error: web/pgadmin/static/vendor/slickgrid/slick.grid.css: patch does not
> apply
> error: patch failed: web/pgadmin/static/vendor/slickgrid/slick.grid.js:1
> error: web/pgadmin/static/vendor/slickgrid/slick.grid.js: patch does not
> apply
> (pgadmin4)snake:web dpage$ cd ../
> (pgadmin4)snake:pgadmin4 dpage$ git apply ~/Downloads/1-upgrade-
> slickgrid.patch
> /Users/dpage/Downloads/1-upgrade-slickgrid.patch:126: trailing whitespace.
>
> /Users/dpage/Downloads/1-upgrade-slickgrid.patch:129: trailing whitespace.
> where the browser copies/pastes the serialized data.
> /Users/dpage/Downloads/1-upgrade-slickgrid.patch:130: trailing whitespace.
>
> /Users/dpage/Downloads/1-upgrade-slickgrid.patch:154: trailing whitespace.
>
> /Users/dpage/Downloads/1-upgrade-slickgrid.patch:165: trailing whitespace.
>
> error: cannot apply binary patch to
> 'web/pgadmin/static/vendor/slickgrid/images/CheckboxN.png' without
> full index line
> error: web/pgadmin/static/vendor/slickgrid/images/CheckboxN.png: patch
> does not apply
> error: cannot apply binary patch to
> 'web/pgadmin/static/vendor/slickgrid/images/CheckboxY.png' without
> full index line
> error: web/pgadmin/static/vendor/slickgrid/images/CheckboxY.png: patch
> does not apply
> error: patch failed:
> web/pgadmin/static/vendor/slickgrid/plugins/slick.cellselectionmodel.js:94
> error: web/pgadmin/static/vendor/slickgrid/plugins/slick.
> cellselectionmodel.js:
> patch does not apply
> error: patch failed: web/pgadmin/static/vendor/
> slickgrid/slick.grid.css:102
> error: web/pgadmin/static/vendor/slickgrid/slick.grid.css: patch does not
> apply
> error: patch failed: web/pgadmin/static/vendor/slickgrid/slick.grid.js:1
> error: web/pgadmin/static/vendor/slickgrid/slick.grid.js: patch does not
> apply
>
> I even tried with patch:
>
> (pgadmin4)snake:pgadmin4 dpage$ patch -p1 <
> ~/Downloads/1-upgrade-slickgrid.patch
> patching file libraries.txt
> patching file web/pgadmin/static/vendor/slickgrid/README
> patching file web/pgadmin/static/vendor/slickgrid/README.md
> patching file web/pgadmin/static/vendor/slickgrid/controls/slick.
> columnpicker.js
> patching file web/pgadmin/static/vendor/slickgrid/plugins/slick.
> cellcopymanager.js
> patching file web/pgadmin/static/vendor/slickgrid/plugins/slick.
> cellexternalcopymanager.js
> patching file web/pgadmin/static/vendor/slickgrid/plugins/slick.
> cellrangeselector.js
> patching file web/pgadmin/static/vendor/slickgrid/plugins/slick.
> cellselectionmodel.js
> Hunk #3 succeeded at 95 with fuzz 2.
> patching file web/pgadmin/static/vendor/slickgrid/plugins/slick.
> headerbuttons.js
> patching file web/pgadmin/static/vendor/slickgrid/plugins/slick.
> headermenu.js
> patching file web/pgadmin/static/vendor/slickgrid/plugins/slick.
> rowselectionmodel.js
> patching file web/pgadmin/static/vendor/slickgrid/slick-default-theme.css
> patching file web/pgadmin/static/vendor/slickgrid/slick.core.js
> patching file web/pgadmin/static/vendor/slickgrid/slick.dataview.js
> patching file web/pgadmin/static/vendor/slickgrid/slick.editors.js
> patching file web/pgadmin/static/vendor/slickgrid/slick.formatters.js
> patching file web/pgadmin/static/vendor/slickgrid/slick.grid.css
> Hunk #6 FAILED at 117.
> 1 out of 9 hunks FAILED -- saving rejects to file
> web/pgadmin/static/vendor/slickgrid/slick.grid.css.rej
> patching file web/pgadmin/static/vendor/slickgrid/slick.grid.js
> Hunk #1 FAILED at 1.
> Hunk #2 FAILED at 18.
> Hunk #3 FAILED at 75.
> Hunk #4 FAILED at 89.
> Hunk #5 FAILED at 129.
> Hunk #6 FAILED at 143.
> Hunk #7 FAILED at 174.
> Hunk #8 FAILED at 208.
> Hunk #9 FAILED at 289.
> Hunk #10 FAILED at 330.
> Hunk #11 FAILED at 343.
> Hunk #12 FAILED at 385.
> Hunk #13 FAILED at 457.
> Hunk #14 FAILED at 491.
> Hunk #15 FAILED at 510.
> Hunk #16 FAILED at 548.
> Hunk #17 FAILED at 557.
> Hunk #18 FAILED at 600.
> Hunk #19 FAILED at 652.
> Hunk #20 FAILED at 686.
> Hunk #21 FAILED at 706.
> Hunk #22 FAILED at 749.
> Hunk #23 FAILED at 858.
> Hunk #24 FAILED at 870.
> Hunk #25 FAILED at 886.
> Hunk #26 FAILED at 937.
> Hunk #27 FAILED at 957.
> Hunk #28 FAILED at 987.
> Hunk #29 FAILED at 1007.
> Hunk #30 FAILED at 1039.
> Hunk #31 FAILED at 1057.
> Hunk #32 FAILED at 1080.
> Hunk #33 FAILED at 1098.
> Hunk #34 FAILED at 1117.
> Hunk #35 FAILED at 1155.
> Hunk #36 FAILED at 1267.
> Hunk #37 FAILED at 1303.
> Hunk #38 FAILED at 1317.
> Hunk #39 FAILED at 1400.
> Hunk #40 FAILED at 1415.
> Hunk #41 FAILED at 1446.
> Hunk #42 FAILED at 1485.
> Hunk #43 FAILED at 1583.
> Hunk #44 FAILED at 1600.
> Hunk #45 FAILED at 1637.
> Hunk #46 FAILED at 1650.
> Hunk #47 FAILED at 1763.
> Hunk #48 FAILED at 1780.
> Hunk #49 FAILED at 1804.
> Hunk #50 FAILED at 1818.
> Hunk #51 FAILED at 1832.
> Hunk #52 FAILED at 1848.
> Hunk #53 FAILED at 1877.
> Hunk #54 FAILED at 1893.
> Hunk #55 FAILED at 2256.
> Hunk #56 FAILED at 2274.
> Hunk #57 FAILED at 2415.
> Hunk #58 FAILED at 2474.
> Hunk #59 FAILED at 2538.
> Hunk #60 FAILED at 2606.
> Hunk #61 FAILED at 2621.
> Hunk #62 FAILED at 2724.
> Hunk #63 FAILED at 2740.
> Hunk #64 FAILED at 2798.
> Hunk #65 FAILED at 2815.
> Hunk #66 FAILED at 2839.
> Hunk #67 FAILED at 2913.
> Hunk #68 FAILED at 2928.
> Hunk #69 FAILED at 2954.
> Hunk #70 FAILED at 2974.
> Hunk #71 FAILED at 3018.
> Hunk #72 FAILED at 3307.
> Hunk #73 FAILED at 3443.
> Hunk #74 FAILED at 3454.
> Hunk #75 FAILED at 3621.
> Hunk #76 FAILED at 3662.
> Hunk #77 FAILED at 3674.
> Hunk #78 FAILED at 3684.
> Hunk #79 FAILED at 3724.
> Hunk #80 FAILED at 3770.
> 80 out of 80 hunks FAILED -- saving rejects to file
> web/pgadmin/static/vendor/slickgrid/slick.grid.js.rej
> patching file web/pgadmin/static/vendor/slickgrid/slick.
> groupitemmetadataprovider.js
> patching file web/pgadmin/static/vendor/slickgrid/slick.remotemodel-
> yahoo.js
> patching file web/pgadmin/static/vendor/slickgrid/slick.remotemodel.js
>
> On Thu, May 25, 2017 at 2:35 PM, Shruti B Iyer <siyer(at)pivotal(dot)io> wrote:
> > Hi
> >
> > Attached are the patches generated using diff and the correction of the
> bugs
> > you mentioned in the previous email.
> >
> > We also split the initial first patch
> > 0001-Improves-user-s-ability-to-select-cells-in-query-res.patch into two
> > different patches because it included the upgrade of slickgrid and other
> > changes making it hard to read. Now, the code review should be much
> easier
> > and the upgrade of a vendor library becomes more clear in the commit
> > history.
> >
> > The 6th patch 6-adapt-slickgrid-to-pgadmin.patch contains the edits to
> the
> > slickgrid library that makes drag-and-select bounding box align with the
> > gridlines. We created a PR to slickgrid that we are still discussing the
> > implementation. Meanwhile. we created a temporary fix to solve the
> problem.
> > We suggest that this 6th patch be committed separately.
> >
> > Thanks
> > Joao & Shruti
> >
> > On Wed, May 24, 2017 at 8:18 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> >>
> >> Hi
> >>
> >> We don't want this to be committed, even to a local tree (as there's a
> >> risk it may inadvertently get pushed). These patches are for review
> >> only at this stage. Once they are ready, they may or may not be
> >> committed individually, and even then it's very unlikely that we'll
> >> want to use the supplied commit message (at minimum we'll want to add
> >> a "Fixes #xxxx" so Redmine updates the ticket and links the commit to
> >> it).
> >>
> >> Please follow the project's normal process and submit patches that can
> >> be applied with git apply.
> >>
> >> Thanks.
> >>
> >>
> >> On Tue, May 23, 2017 at 5:46 PM, Shruti B Iyer <siyer(at)pivotal(dot)io>
> wrote:
> >> > Hi Dave,
> >> >
> >> > git am is also helpful to apply a patch that was created using git
> >> > format-patch, which is how we created these four patches. git am will
> >> > apply
> >> > the diff in the patch and also make a commit using the commit message
> >> > stored
> >> > in the patch. Could you try it again using git am? If it still doesn't
> >> > work
> >> > for you, we can try creating the diff files.
> >> >
> >> > Thanks,
> >> > Shruti & Matt
> >> >
> >> > On Tue, May 23, 2017 at 5:28 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> >> >>
> >> >> Hi
> >> >>
> >> >> git am is for applying patches from mailbox files:
> >> >>
> >> >> Splits mail messages in a mailbox into commit log message, authorship
> >> >> information and patches, and applies them to the current branch.
> >> >>
> >> >> That doesn't seem like it'll help me as a gmail user. Can you fix the
> >> >> patches to apply with git apply please?
> >> >>
> >> >> On Tuesday, May 23, 2017, Shruti B Iyer <siyer(at)pivotal(dot)io> wrote:
> >> >>>
> >> >>> Hi Dave,
> >> >>>
> >> >>> We see the same errors when doing git apply for each patch. However,
> >> >>> if
> >> >>> you do git am for each patch, it should proceed.
> >> >>>
> >> >>> Thanks,
> >> >>> Shruti & Matt
> >> >>>
> >> >>> On Tue, May 23, 2017 at 4:55 PM, Dave Page <dpage(at)pgadmin(dot)org>
> wrote:
> >> >>>>
> >> >>>> Hi!
> >> >>>>
> >> >>>> Looks great! I found a few issues which I think should be addressed
> >> >>>> before we continue too far. Note that I haven't reviewed the code
> at
> >> >>>> this stage:
> >> >>>>
> >> >>>> - When dragging a selection, the bounding box doesn't line up with
> >> >>>> the
> >> >>>> bottom of the grid rows. Note that I couldn't screen shot this
> >> >>>> unfortunately. It's not broken as such - just looks wrong.
> >> >>>>
> >> >>>> - If I copy one or more rows, I'm unable to paste them in as new
> rows
> >> >>>> when editing table data.
> >> >>>>
> >> >>>> - The 0004 patch doesn't apply:
> >> >>>>
> >> >>>> (pgadmin4)snake:web dpage$ git apply
> >> >>>> ~/Downloads/0004-Introduces-XCellSelectionModel.patch
> >> >>>> /Users/dpage/Downloads/0004-Introduces-
> XCellSelectionModel.patch:640:
> >> >>>> trailing whitespace.
> >> >>>> function scrollColumnIntoView(columnIndex) {
> >> >>>> /Users/dpage/Downloads/0004-Introduces-
> XCellSelectionModel.patch:641:
> >> >>>> trailing whitespace.
> >> >>>> var colspan = getColspan(row, columnIndex);
> >> >>>> /Users/dpage/Downloads/0004-Introduces-
> XCellSelectionModel.patch:642:
> >> >>>> trailing whitespace.
> >> >>>>
> >> >>>> /Users/dpage/Downloads/0004-Introduces-
> XCellSelectionModel.patch:643:
> >> >>>> trailing whitespace.
> >> >>>> var left = columnPosLeft[columnIndex],
> >> >>>> /Users/dpage/Downloads/0004-Introduces-
> XCellSelectionModel.patch:644:
> >> >>>> trailing whitespace.
> >> >>>> right = columnPosRight[columnIndex + (colspan > 1 ?
> colspan -
> >> >>>> 1
> >> >>>> : 0)],
> >> >>>> error: patch failed:
> >> >>>> web/pgadmin/static/vendor/slickgrid/slick.grid.js:2794
> >> >>>> error: web/pgadmin/static/vendor/slickgrid/slick.grid.js: patch
> does
> >> >>>> not
> >> >>>> apply
> >> >>>>
> >> >>>> Thanks, Dave.
> >> >>>>
> >> >>>>
> >> >>>> On Tue, May 23, 2017 at 12:11 PM, Matthew Kleiman
> >> >>>> <mkleiman(at)pivotal(dot)io>
> >> >>>> wrote:
> >> >>>> > Hi Hackers!
> >> >>>> >
> >> >>>> > Attached are the updates to the query results grid, broken up
> into
> >> >>>> > four
> >> >>>> > patches.
> >> >>>> >
> >> >>>> >
> >> >>>> > Description of the Completed Functionality After Applying All
> Four
> >> >>>> > Patches
> >> >>>> > Currently the designed behavior is somewhere between excel like
> >> >>>> > behavior and
> >> >>>> > not. As such we can describe the behavior as follows:
> >> >>>> >
> >> >>>> > Select columns by clicking on the header
> >> >>>> > Select rows by clicking on the row header (column 0)
> >> >>>> > You can drag and select with the mouse
> >> >>>> > You can select all with ctrl+a or by clicking the upper left cell
> >> >>>> > You can copy with ctrl+c or with the copy icon
> >> >>>> > you can increase or decrease the size of the selected area with
> >> >>>> > shift+arrow
> >> >>>> > shift+arrow understands directionality, e.g. drag select from
> left
> >> >>>> > to
> >> >>>> > right
> >> >>>> > differs from drag select from right to left
> >> >>>> > Clicking anywhere outside of the selected area deselects the area
> >> >>>> > and
> >> >>>> > reselects the new cell(s) clicked on
> >> >>>> >
> >> >>>> > Current potentially awkward but intentional functionality
> >> >>>> >
> >> >>>> > When you select multiple columns/rows by clicking on the header,
> >> >>>> > then
> >> >>>> > press
> >> >>>> > shift+arrow all but the last selected columns/rows are deselected
> >> >>>> >
> >> >>>> > Includes fixes for:
> >> >>>> > RM Bug #2348 - On resize of first/any column in "Query Tool/View
> >> >>>> > Data"
> >> >>>> > will
> >> >>>> > select/deselect all the rows/columns.
> >> >>>> > RM Bug #2344 - ctrl+v and ctrl+c need to work
> >> >>>> >
> >> >>>> >
> >> >>>> > Detailed Description of Each Patch
> >> >>>> >
> >> >>>> > 0001-Improves-user-s-ability-to-select-cells-in-query-res.patch
> -
> >> >>>> >
> >> >>>> > - user can select columns
> >> >>>> >
> >> >>>> > - user can modify column or row selection with shift+arrow
> >> >>>> >
> >> >>>> > - user can select entire grid with ctrl+A or cmd+A
> >> >>>> >
> >> >>>> > - user can copy from grid using keyboard shortcuts
> >> >>>> >
> >> >>>> > 0002-Drag-and-select-from-data-grid.patch -
> >> >>>> >
> >> >>>> > 0003-Removes-checkboxes-from-the-grid.patch -
> >> >>>> >
> >> >>>> > - Changes header color to grey
> >> >>>> >
> >> >>>> > 0004-Introduces-XCellSelectionModel.patch -
> >> >>>> >
> >> >>>> > - header styles depend on the selection
> >> >>>> >
> >> >>>> > - show the correct row/column when scrolling up or left
> >> >>>> >
> >> >>>> > - fixes drag and drop when drop is done outside the grid
> >> >>>> >
> >> >>>> >
> >> >>>> > Thanks,
> >> >>>> >
> >> >>>> > Matt & Shruti
> >> >>>> >
> >> >>>> >
> >> >>>> >
> >> >>>> >
> >> >>>> >
> >> >>>> > --
> >> >>>> > 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
> >> >>>
> >> >>>
> >> >>
> >> >>
> >> >> --
> >> >> Dave Page
> >> >> Blog: http://pgsnake.blogspot.com
> >> >> Twitter: @pgsnake
> >> >>
> >> >> EnterpriseDB UK: http://www.enterprisedb.com
> >> >> The Enterprise PostgreSQL Company
> >> >>
> >> >
> >>
> >>
> >>
> >> --
> >> Dave Page
> >> Blog: http://pgsnake.blogspot.com
> >> Twitter: @pgsnake
> >>
> >> EnterpriseDB UK: http://www.enterprisedb.com
> >> The Enterprise PostgreSQL Company
> >
> >
>
>
>
> --
> 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
>