Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1aqhyY-0005rD-JG for pgadmin-hackers@arkaria.postgresql.org; Thu, 14 Apr 2016 14:10:30 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1aqhyY-00084U-66 for pgadmin-hackers@arkaria.postgresql.org; Thu, 14 Apr 2016 14:10:30 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1aqhyW-000815-Uw for pgadmin-hackers@postgresql.org; Thu, 14 Apr 2016 14:10:29 +0000 Received: from mail-ig0-x235.google.com ([2607:f8b0:4001:c05::235]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1aqhyP-0002l6-Nm for pgadmin-hackers@postgresql.org; Thu, 14 Apr 2016 14:10:28 +0000 Received: by mail-ig0-x235.google.com with SMTP id gy3so148853141igb.0 for ; Thu, 14 Apr 2016 07:10:21 -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=xy8OjyCipPtrlZk7kMrIyPoh6FsP/AI9rafDPCAfaBQ=; b=lwZcllXql9ZMufhcGPFK/R0IxU0UWiNWJp7qQDb4vdAq4i//ghEu0co+K8wlYoQM9n f36AZ4yxPp9/yT+Gb9/Akof0lGFwD0kP6oH39csr/dZ7bLjiDTcxC7jhSqxw8x57APzX mXY4yW0YCZ25IKMXgrTSK3uvoMd5eLS6X6fVoIzL8NiTBbmqGgCYVpDtiimq8jMPuj7l LGh0fCMJ65+AM/F8dnbkausNXR2mkV3CKj4ZAoL1TAXM52BwbmpDG8iklZjpmMqdCxiS wgqZ5af447mXVnmW9RdYPRCwukc7wFlpVXa9RFy9BecLVpSt1j9BErNkVeA/oWzL6dSJ js7Q== 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=xy8OjyCipPtrlZk7kMrIyPoh6FsP/AI9rafDPCAfaBQ=; b=E5z+jtDE7I2xi1rfzCLFB32fn6NOesP5LGJ6QTbVNi0hjOnzL3nALi1LWFtnAfXGHk /A3Rat0KnMsdM1EgQAjSi4rvh5AgDYNIugV3yZ2BGMBvonc8t6xMW+s8D/WNaHii/kAZ p824vw7Fd9aw8WdlInYmifO9dj0WZWK3mfRAdz7gTBjLu6V8cIaQR4WXtdTzHvKuqcdU ciDkysg5vJt1jBd9AjTBxy/gh15gd5g+IEthsgjmqDONUXv0nhiH72tt8aPIxmwpyYP7 3bWcVy+OCb47AjyrSSeJKikka9M9Inu7BlD2pwxZHIbLMUWDnulYxvEMX66AwhfteI4b tIvw== X-Gm-Message-State: AOPr4FV3YYW5jioh6ayveZjV00hDJslYuoXvAFiUkQppgigAeDfMmn6tcINUj38stcHuYmM+nhk5Xp4mhHXBHfodVk2FNcHza7SvNMfEYlEb7ct88fZM3liTFbgRcqEeGlOoI6n+neBAzuMENIjTyv19YwYV+rmKNl717RJ6JMvpUeqYrbPsJmmlIVHIhnamNQ6zNvUQeA== X-Received: by 10.50.29.73 with SMTP id i9mr17535628igh.32.1460643019824; Thu, 14 Apr 2016 07:10:19 -0700 (PDT) Received: from mail-io0-f171.google.com (mail-io0-f171.google.com. [209.85.223.171]) by smtp.gmail.com with ESMTPSA id cy7sm3161306igc.17.2016.04.14.07.10.17 for (version=TLSv1/SSLv3 cipher=OTHER); Thu, 14 Apr 2016 07:10:17 -0700 (PDT) Received: by mail-io0-f171.google.com with SMTP id u185so104470079iod.3 for ; Thu, 14 Apr 2016 07:10:17 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.107.148.210 with SMTP id w201mr16910247iod.63.1460643017219; Thu, 14 Apr 2016 07:10:17 -0700 (PDT) Received: by 10.64.105.131 with HTTP; Thu, 14 Apr 2016 07:10:17 -0700 (PDT) In-Reply-To: References: Date: Thu, 14 Apr 2016 15:10:17 +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=001a113fd4022fe76605307274f4 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 --001a113fd4022fe76605307274f4 Content-Type: text/plain; charset=UTF-8 Hi On Thu, Apr 14, 2016 at 1:58 PM, Akshay Joshi 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. -- Dave Page VP, Chief Architect, Tools & Installers EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company Blog: http://pgsnake.blogspot.com Twitter: @pgsnake --001a113fd4022fe76605307274f4 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi

On Thu, Apr 14, 2016 at 1:58 PM, Akshay Joshi <<= a href=3D"mailto:akshay.joshi@enterprisedb.com" target=3D"_blank">akshay.jo= shi@enterprisedb.com> wrote:
Hi All=C2=A0

I have fixed review comm= ents given by Dave and couple of them are remaining

Fixed Review Comments:
  • The View Data menu opti= on should be on the Object menu, which should mirror the Context menu, exce= pt options should be disabled when not applicable instead of hidden.=
  • The Q= uery Tool menu icon should be a glyphicon, to match the others= .
  • Please merge the functionality of the Refresh and Execute buttons into o= ne button. We shouldn't have two buttons that do essentially the same t= hing.
  • In Edit Grid mode, the History panel should log all queries (S= ELECTs, 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 =C2=A0as it would in the Query Too= l.
  • In Edit Grid mode, that textbox should be read-only, but s= hould display the SQL used (including any LIMIT/FILTER clauses= )
  • Please remove the border from the SQL box, such that it fil= ls all available space.
  • The Filter box should be in a modal overlay over t= he top of the SQL box/Results tabs as required. Those elements= should be grayed whilst it is open.
  • Please adjust the height of the Delet= e icon in the Edit Grid, such that it doesn't force the row height to b= e 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 "Ed= it" button, which opens a drop-down menu. This would eventually includ= e options as found on the Edit menu in the pgAdmin3 query tool, such as the= "Clear SQL" option.
  • Ashesh and I disc= ussed changing the History tab to be a grid, showing: Date/Time, Query (fir= st 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 lis= t.
  • Errors should be highlighted in the SQL box - a marker in the margin to note the line, and spellchec= k-style underlining for the error word.
  • Query results should have spaces converted to "&= amp;nbsp;", so that proper indenting is maintained (for e= xample, on EXPLAIN queries).
  • on shutdown pgAdmin server ,=C2=A0<= /span>appropriate message should be displa= yed.

Awesome!<= /div>

I've made a few minor style changes - mostly c= olouring, but I also widened the Execute button and it was easier to push i= t's dropdown than it - and pushed the code.
=C2=A0
= Remaining review comments:
    =
  • Please add an S= QL button. This should show/hide the SQL panel in *both= * Query Tool and Edit Grid modes.=C2=A0
  • 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 th= e resultset.
  • Can the "Copy Row" button also populat= e the clipboard with CSV data for the row?
  • In Edit mode, we n= eed to be able to represent/set values to NULL.
  • The layout of the result t= abs 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.=C2=A0
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 mov= ed out into an HTML template
- create.sql should perhaps be renam= ed 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 ther= e?

I'll continue to log additional issues if/w= hen I find them.
=C2=A0
--
Dave Page
VP, Chief Architect, Tools & Installers
Enterpr= iseDB: http://www= .enterprisedb.com
The Enterprise PostgreSQL Company

Blog: http://pgsnake.blogs= pot.com
Twitter: @pgsnake
--001a113fd4022fe76605307274f4--