Re: [pgadmin-hackers][patch] Column selection on SQLEditor

Thanks - patch committed!

On Wed, Apr 12, 2017 at 10:00 PM, Matthew Kleiman <mkleiman@pivotal.io> wrote:
> Hi Dave,
>
> We have found the source of this bug and fixed it in the attached patch.
> If this revised patch addresses the copy issues you pointed out, it can be
> pulled into master.
>
> Please note that we'll be working on a few more stories around column
> selection styling, e.g. removal of checkboxes. See email thread with subject
> "[pgadmin-hackers] [Design Update] Visual design of query editor and results
> table" for details.
> We will change the styling of this patch as part of that work.
>
> Best,
> George and Matt
>
>
>
>
>
>
> On Mon, Apr 10, 2017 at 11:13 AM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> Hi
>>
>> On Mon, Apr 10, 2017 at 4:01 PM, Matthew Kleiman <mkleiman@pivotal.io>
>> wrote:
>> > Hi Dave,
>> >
>> >> Unfortunately I found another regression; I have a test table called
>> >> موسيقى (Arabic for music) which if included in a selection of rows,
>> >
>> > Could you share the queries that:
>> > - create this table
>>
>> CREATE TABLE public."موسيقى"
>> (
>> )
>>
>> > - populate it with data
>>
>> It doesn't have any - there are no columns in it.
>>
>> > - access it leading to the regression
>>
>> SELECT * FROM pg_tables
>>
>> or
>>
>> SELECT * FROM pg_class
>>
>> in the Query Tool.
>>
>> > What user interaction are you performing to trigger the behavior (is it
>> > the
>> > query tool or something else)?
>>
>> Run the select query in the query tool, select the rows, then hit Copy.
>>
>> Note that I think the issue with columns is the editor I was using. If
>> I paste into vim, I can't reproduce that. The row issue is
>> consistently reproducible though.
>>
>> > It would also help if you could attach a screenshot of the issue.
>>
>> Attached.
>>
>> >
>> > Thanks,
>> > George & Matt
>> >
>> >
>> >
>> > On Mon, Apr 10, 2017 at 5:52 AM Dave Page <dpage@pgadmin.org> wrote:
>> >>
>> >> Hi Matt,
>> >>
>> >> On Fri, Apr 7, 2017 at 10:56 PM, Matthew Kleiman <mkleiman@pivotal.io>
>> >> wrote:
>> >> > Hi Dave,
>> >> >
>> >> > I've updated the attached patch to include a change to the "Paste
>> >> > Rows"
>> >> > button. It will now be enabled only if there are rows on the
>> >> > clipboard.
>> >>
>> >> Unfortunately I found another regression; I have a test table called
>> >> موسيقى (Arabic for music) which if included in a selection of rows,
>> >> seems to ensure that it and any preceding rows are excluded from the
>> >> copy. If I include it in a selection of columns, then things get weird
>> >> as the pasted output for that row will sometimes - but not always -
>> >> seem to have the column order moved around (maybe because it's an RTL
>> >> language).
>> >>
>> >> Obviously the second issue doesn't apply to the current code, but
>> >> copying rows does seem to work properly at present, so I'm afraid I
>> >> need to bump this back to you again.
>> >>
>> >> > Good luck with the release on Monday morning!
>> >>
>> >> Thanks!
>> >>
>> >> > On Fri, Apr 7, 2017 at 4:12 PM, Matthew Kleiman <mkleiman@pivotal.io>
>> >> > wrote:
>> >> >>
>> >> >> Hi Dave
>> >> >>
>> >> >> The attached patch now includes a fix for the regression you found.
>> >> >> When
>> >> >> the query tool is in edit mode, the user can copy rows and paste
>> >> >> them.
>> >> >> I've
>> >> >> also removed the two lines from copy_data.js that you mentioned so
>> >> >> that
>> >> >> users can copy from the table even after they have copied once.
>> >> >>
>> >> >> I have noticed that the "Paste Rows" button remains enabled even if
>> >> >> only
>> >> >> columns are copied. Although pressing this button won't do anything
>> >> >> unless
>> >> >> rows have been copied to the clipboard, this might prove confusing
>> >> >> to
>> >> >> users.
>> >> >> I am going to look at disabling this button until rows are in the
>> >> >> clipboard.
>> >> >> I will let you know if I have anything for you by the end of today.
>> >> >>
>> >> >> Best,
>> >> >> Matt
>> >> >>
>> >> >> On Fri, Apr 7, 2017 at 10:27 AM, Dave Page <dpage@pgadmin.org>
>> >> >> wrote:
>> >> >>>
>> >> >>> On Fri, Apr 7, 2017 at 2:49 PM, Atira Odhner <aodhner@pivotal.io>
>> >> >>> wrote:
>> >> >>> >> The one tweak I made to the patch was to remove the code that
>> >> >>> >> disabled
>> >> >>> >> the Copy button from the top of copy_data.js. I think the button
>> >> >>> >> should remain enabled to allow the user to copy again, in case
>> >> >>> >> they
>> >> >>> >> use the clipboard for something else and then need to refresh it
>> >> >>> >> with
>> >> >>> >> the data. Of course, it should still be disabled when there is
>> >> >>> >> nothing
>> >> >>> >> selected that can be copied.
>> >> >>> >
>> >> >>> > Yes, the copy button enablement behavior was a bit strange.  I'm
>> >> >>> > glad
>> >> >>> > you
>> >> >>> > found a fix for it. Do you mind sending us your updated patch?
>> >> >>>
>> >> >>> I literally just removed lines 5 & 6 of copy_data.js.
>> >> >>>
>> >> >>> >> Any chance this can be fixed before Monday? I would like to
>> >> >>> >> include
>> >> >>> >> it
>> >> >>> >> in the release if possible
>> >> >>> >
>> >> >>> > I'll drop a bug at the top of our backlog, and Matt will take a
>> >> >>> > look
>> >> >>> > at
>> >> >>> > it
>> >> >>> > today. We'll let you know at the end of the day where we're at
>> >> >>> > with
>> >> >>> > this
>> >> >>> > fix.
>> >> >>>
>> >> >>> Thanks - and sorry to hear your moving onto other things :-(
>> >> >>>
>> >> >>> --
>> >> >>> 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