Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1aqyYc-0000Zw-Ob for pgadmin-hackers@arkaria.postgresql.org; Fri, 15 Apr 2016 07:52:50 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1aqyYc-00084P-8R for pgadmin-hackers@arkaria.postgresql.org; Fri, 15 Apr 2016 07:52:50 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1aqyYa-00080x-J1 for pgadmin-hackers@postgresql.org; Fri, 15 Apr 2016 07:52:48 +0000 Received: from mail-io0-x22b.google.com ([2607:f8b0:4001:c06::22b]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1aqyYX-0002Kc-Ox for pgadmin-hackers@postgresql.org; Fri, 15 Apr 2016 07:52:47 +0000 Received: by mail-io0-x22b.google.com with SMTP id g185so127583732ioa.2 for ; Fri, 15 Apr 2016 00:52:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc; bh=nUCm62Cdr/U3jXar7YPSoiARecwpgEP4fwv9AI8UoZg=; b=JQ0o9vDtbGi1srhGuNDr/dg2JWEgwUsIAmLQWHCgTavL4op1ONzKY6ozSBkWCa4Ga0 4c6MvAjqUh0WwhUlGS6eJTrCVi45M/zMNowC5WICnlnXN03ZYVpolpaML2GmTCuqRHr+ qEfPDYPomhuaJtQnVB5aHIjLa7vR4/R4vT/gH9Gsk0r1U7WsdUl71QzlVQoMftdBDxNc fIYNIp9eeQXmGf0GOLreW68ReynGDLUyP9jm5nTQkJTecLyTZbfFod/ewQsQG9y4DzqV vl02pICNLhU1QpbdwiDl7sIe7OJQfqXKtf+817SGLClp9wHFMLZ6y44lcERZVAq5S0tJ cXLQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=nUCm62Cdr/U3jXar7YPSoiARecwpgEP4fwv9AI8UoZg=; b=j92Swxk/0eWYgwdT5ICCSfgRp4RjZi/mJb0zfJzBZoeciiKnrgsiOCs4Z3TiXoYfV1 vbOk1X1soHFXNUCfG8deBCRhd/zsA4NUg4EhALykom7JZgGAVHvfvDUdzG/D5HHxJ/Od 3yr+mA4ksd8MCcA+qM+wTFV91yzVn/0+QR9hQhQYuiG6sP45/58uQYBxwb6xqxF0SkH+ /OYQidjxZ2SX77rW6Gzbbikv7b9g7BfoFG7lz8OVIbkaGfFwkrFWxqKqqlQhRxslL39o BK14eoBAldS3ZCcM+WrugIJX514Y4F19H86si9nzs+iW1wYlUPojxA/E7MITJM4WNiuu PwUQ== X-Gm-Message-State: AOPr4FUr7XVQ4YbVIAOthcA5ah+kwJrg1N0YfZ6xOzn4Kmu/JuX+pjyOqNMrZywxQ0nq5pCZgd6muDwwRVNi7YzpLkf/pq0Px6s5+b5jdKv8u+RDZZ7hezPDtmU1GHaYGZVsU6e9gTvR3RblFoOjTHSs624fOtXYVfQBapGasH8Wdn5I38LYOOhVXYureNXo3pCcGJnD5A== X-Received: by 10.107.135.202 with SMTP id r71mr20758542ioi.151.1460706764982; Fri, 15 Apr 2016 00:52:44 -0700 (PDT) Received: from mail-ig0-f179.google.com (mail-ig0-f179.google.com. [209.85.213.179]) by smtp.gmail.com with ESMTPSA id 207sm11951036iou.33.2016.04.15.00.52.43 for (version=TLSv1/SSLv3 cipher=OTHER); Fri, 15 Apr 2016 00:52:43 -0700 (PDT) Received: by mail-ig0-f179.google.com with SMTP id gy3so13506859igb.0 for ; Fri, 15 Apr 2016 00:52:43 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.50.66.171 with SMTP id g11mr3148041igt.96.1460706763030; Fri, 15 Apr 2016 00:52:43 -0700 (PDT) Received: by 10.64.105.131 with HTTP; Fri, 15 Apr 2016 00:52:42 -0700 (PDT) In-Reply-To: References: Date: Fri, 15 Apr 2016 08:52:42 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [pgAdmin4] [Patch] Implementation of the Data Grid and Query Tool From: Dave Page To: Akshay Joshi Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary=047d7bdc08a6bb90550530814bef X-Pg-Spam-Score: -2.6 (--) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgadmin-hackers Precedence: bulk Sender: pgadmin-hackers-owner@postgresql.org --047d7bdc08a6bb90550530814bef Content-Type: text/plain; charset=UTF-8 Thanks - committed. On Fri, Apr 15, 2016 at 8:03 AM, Akshay Joshi wrote: > > > On Thu, Apr 14, 2016 at 7:40 PM, Dave Page > wrote: > >> Hi >> >> On Thu, Apr 14, 2016 at 1:58 PM, Akshay Joshi < >> akshay.joshi@enterprisedb.com> wrote: >> >>> Hi All >>> >>> I have fixed review comments given by Dave and couple of them are >>> remaining >>> >>> *Fixed Review Comments:* >>> >>> - The View Data menu option should be on the Object menu, which >>> should mirror the Context menu, except options should be disabled when not >>> applicable instead of hidden. >>> - The Query Tool menu icon should be a glyphicon, to match the >>> others. >>> - Please merge the functionality of the Refresh and Execute buttons >>> into one button. We shouldn't have two buttons that do essentially the same >>> thing. >>> - In Edit Grid mode, the History panel should log all queries ( >>> SELECTs, UPDATEs, DELETEs etc) as it would in the Query Tool. >>> - In Edit Grid mode, the Messages panel should display any messages >>> from the most recent action as it would in the Query Tool. >>> - In Edit Grid mode, that textbox should be read-only, but should >>> display the SQL used (including any LIMIT/FILTER clauses) >>> - Please remove the border from the SQL box, such that it fills all >>> available space. >>> - The Filter box should be in a modal overlay over the top of the SQL >>> box/Results tabs as required. Those elements should be grayed whilst it is >>> open. >>> - Please adjust the height of the Delete icon in the Edit Grid, such >>> that it doesn't force the row height to be higher than it should be. >>> - I think the names of the tabs are far too long. Can we change them >>> to "Query 1", "Query 2" etc, then rename them to the filename if the >>> user saves/loads a file? >>> - We should add an "Edit" button, which opens a drop-down menu. This >>> would eventually include options as found on the Edit menu in the pgAdmin3 >>> query tool, such as the "Clear SQL" option. >>> - Ashesh and I discussed changing the History tab to be a grid, >>> showing: Date/Time, Query (first line only), Rows affected, Runtime >>> and Status, in a row per query executed. Ashesh suggested using a >>> sub-form that can be expanded for each row, which could show the full query >>> and error details (SQL State etc). New rows should be added to the >>> top of the list. >>> - Errors should be highlighted in the SQL box - a marker in the >>> margin to note the line, and spellcheck-style underlining for the error >>> word. >>> - Query results should have spaces converted to " ", so that >>> proper indenting is maintained (for example, on EXPLAIN queries). >>> - on shutdown pgAdmin server , appropriate message should be >>> displayed. >>> >>> >> Awesome! >> >> I've made a few minor style changes - mostly colouring, but I also >> widened the Execute button and it was easier to push it's dropdown than it >> - and pushed the code. >> >> >>> *Remaining review comments*: >>> >>> - Please add an SQL button. This should show/hide the SQL panel in >>> *both* Query Tool and Edit Grid modes. >>> - If a field has been edited, but not saved, can we highlight it >>> somehow? Maybe make the text dark blue? >>> - The "Add Row" button only works if you're on the last page of the >>> resultset. >>> - Can the "Copy Row" button also populate the clipboard with CSV >>> data for the row? >>> - In Edit mode, we need to be able to represent/set values to NULL. >>> - The layout of the result tabs should be maintained if new Query >>> Tool or Edit Grid tabs are opened. >>> >>> *TODO's* (apart from above) >>> >>> - Open/Save SQL file. >>> - Cut, Copy, Paste functionality. >>> >>> Agreed on the above. >> >> My only real suggestion on the code itself (which looks good and clean on >> a quick review), is that: >> >> - The button bar be moved out into an HTML template >> - create.sql should perhaps be renamed to insert.sql >> - It looks like we only allow updates if we have a pkey. Should we allow >> use of OIDs when present, if a pkey isn't there? >> >> I'll continue to log additional issues if/when I find them. >> > > Thanks for committing the patch. I'll work on the above comments. > Meanwhile I have found one issue where "View Filtered Row" is not working, > so attached is the patch file to fix that. Can you please review/commit it. > >> >> -- >> Dave Page >> VP, Chief Architect, Tools & Installers >> EnterpriseDB: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> >> Blog: http://pgsnake.blogspot.com >> Twitter: @pgsnake >> > > > > -- > *Akshay Joshi* > *Principal Software Engineer * > > > > *Phone: +91 20-3058-9517 <%2B91%2020-3058-9517>Mobile: +91 976-788-8246* > -- Dave Page VP, Chief Architect, Tools & Installers EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company Blog: http://pgsnake.blogspot.com Twitter: @pgsnake --047d7bdc08a6bb90550530814bef Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Thanks - committed.

On Fri, Apr 15, 2016 at 8:03 AM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:


=
On Thu, Apr 14, 2016 at 7= :40 PM, Dave Page <dave.page@enterprisedb.com> wrote:
Hi

= On Thu, Apr 14, 2016 at 1:58 PM, Akshay Joshi <akshay.joshi@en= terprisedb.com> wrote:
Hi All=C2=A0

I have fixed review comments g= iven by Dave and couple of them are remaining

F= ixed Review Comments:
    The View Data menu option should be on t= he Object menu, which should mirror the Context menu, except options should= be disabled when not applicable instead of hidden.
    <= span>
  • The Query Tool menu icon should b= e a glyphicon, to match the others.
  • Please merge the functionality of th= e Refresh and Execute buttons into one button. We shouldn't have two bu= ttons that do essentially the same thing.
  • <= span style=3D"font-size:12.8px">In Edit Grid mode, the History panel should= log all queries (SELECTs, UPDATEs, DELETEs= etc) as it would in the Query Tool.
    • In Edit Grid mode, the Messages panel sho= uld display any messages from the most recent action =C2=A0as it would in t= he Query Tool.
  • In Edit Grid mode, that textbox should be read-only, but = should display the SQL used (including any LIMIT/FILTER clause= s)
  • Please = remove the border from the SQL box, such that it fills all ava= ilable space.
  • The Filter box should be in a modal overlay over the top of the S= QL box/Results tabs as required. Those elements should be grayed whi= lst it is open.
  • Please adjust the height of the Delete icon in the Edit Grid, such th= at it doesn't force the row height to be higher than it should be.
  • I think the names of the ta= bs are far too long. Can we change them to "Query 1", "Query= 2" etc, then rename them to the filename if the user sav= es/loads a file?
  • We should add an "Edit" button, which opens a drop-down me= nu. This would eventually include options as found on the Edit menu in the = pgAdmin3 query tool, such as the "Clear SQL" option.=
  • Ash= esh and I discussed changing the History tab to be a grid, showing: = Date/Time, Query (first line only), Rows affected, Runtime and= Status, in a row per query executed. Ashesh suggested using a= sub-form that can be expanded for each row, which could show the full quer= y and error details (SQL State etc). New rows should be added = to the top of the list.
  • Errors should be highlighted= in the SQL box - a marker in the margin to note the line, and= spellcheck-style underlining for the error word.
  • <= span style=3D"font-size:12.8px">Query results should have spaces converted = to "&nbsp;", so that proper indenting is maintai= ned (for example, on EXPLAIN queries).
  • on shutdown pgAdmin server ,=C2=A0appropriate message should = be displayed.

Awesome!

I've made a few minor style ch= anges - mostly colouring, but I also widened the Execute button and it was = easier to push it's dropdown than it - and pushed the code.
=
=C2=A0
Remaining review comments:
  • Please add= an SQL button. This should show/hide the SQL pan= el in *both* Query Tool and Edit Grid modes.=C2=A0
  • If a field has been edited, but no= t saved, can we highlight it somehow? Maybe make the text dark blue?=
  • The "Add Ro= w" button only works if you're on the last page of the resul= tset.
  • Can the "Copy Row" button also populate the clipboard with CSV data for the row?
  • In Edit mode, we need to be able to represent/set val= ues to NULL.
  • The layout of the result tabs should be maintained if new Query Tool or = Edit Grid tabs are opened.
TODO's (apart from above)
<= div>
  • Open/Save SQL fil= e.
  • Cut, Copy, Paste functio= nality.=C2=A0
Agreed o= n the above.

My only real suggestion on the code i= tself (which looks good and clean on a quick review), is that:
- The button bar be moved out into an HTML template
= - create.sql should perhaps be renamed to insert.sql
- It looks l= ike we only allow updates if we have a pkey. Should we allow use of OIDs wh= en present, if a pkey isn't there?

I'll co= ntinue to log additional issues if/when I find them.

=C2=A0 =C2=A0Thanks for commi= tting the patch. I'll work on the above comments. Meanwhile I have foun= d one issue where "View Filtered Row" is not working, so attached= is the patch file to fix that. Can you please review/commit it.
= =C2=A0
--
Dave Page
VP, Chief Architect= , Tools & Installers
EnterpriseDB: http://www.enterprisedb.com
The Enterprise = PostgreSQL Company

Blog: http://pgsnake.blogspot.com
Twitt= er: @pgsnake



<= /div>--
Akshay Joshi<= /div>
Principal S= oftware Engineer=C2=A0
=

Phone: +91 20-3058-9517
Mobile: +91 976-788-824= 6



--
Dave Page
VP, Chief Architect, Tools & Installe= rs
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
=
Blog: http://= pgsnake.blogspot.com
Twitter: @pgsnake
--047d7bdc08a6bb90550530814bef--