Re: [pgadmin-hackers] [pgAdmin4][PATCH] Improvements to Query ResultsGrid User Experience

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@pivotal.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@pgadmin.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@pivotal.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@pgadmin.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@pivotal.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@pgadmin.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@pivotal.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@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
>> >>>
>> >>>
>> >>
>> >>
>> >> --
>> >> 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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers