Re: [patch] Column selection on SQLEditor

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(at)pgadmin(dot)org> wrote:

> Hi
>
> On Mon, Apr 10, 2017 at 4:01 PM, Matthew Kleiman <mkleiman(at)pivotal(dot)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(at)pgadmin(dot)org> wrote:
> >>
> >> Hi Matt,
> >>
> >> On Fri, Apr 7, 2017 at 10:56 PM, Matthew Kleiman <mkleiman(at)pivotal(dot)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(at)pivotal(dot)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(at)pgadmin(dot)org>
> wrote:
> >> >>>
> >> >>> On Fri, Apr 7, 2017 at 2:49 PM, Atira Odhner <aodhner(at)pivotal(dot)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
>