Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1aqxnI-0006tp-A3 for pgadmin-hackers@arkaria.postgresql.org; Fri, 15 Apr 2016 07:03:56 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1aqxnH-0004UW-RC for pgadmin-hackers@arkaria.postgresql.org; Fri, 15 Apr 2016 07:03:55 +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 1aqxnG-0004Tl-PP for pgadmin-hackers@postgresql.org; Fri, 15 Apr 2016 07:03:55 +0000 Received: from mail-ob0-x231.google.com ([2607:f8b0:4003:c01::231]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1aqxn9-0001KA-HA for pgadmin-hackers@postgresql.org; Fri, 15 Apr 2016 07:03:53 +0000 Received: by mail-ob0-x231.google.com with SMTP id bg3so60158631obb.1 for ; Fri, 15 Apr 2016 00:03:47 -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=n+Cqv8oDecLm70jqDEgYJSGKVQSxgvu5mZXXuCDgH5M=; b=piJaQebspNx0Z6rzDvEAWlp22yKYsLVkALtmz1CcwUD5m8G0TZfNBg3wTOnj4MHURE NbndSNEKZlz/M6F/T721EKNRx+XmMzedW4m8pYnCeQSOPFO8mw442jy4rVOIklpRP8lF T6zGfx6RTKu2JDtyAyLBUEt1hliE1WKJYu7v1bROyrZIvH6IFlWCXdEKULTy/bLKXCsE EGUcvvlyT9fKGYYtS1gL6l5v0AuZ1OSOFkBdbpNYBMf6VidO16UHvDUP4zZEszO1hiAp 6jejhUkaiuOL/jg3oiJjhhiIR+7IZoKBu2Ubm6UM/v7TWmKtU1c+y70zaO/WkKAaU95m TR9Q== 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=n+Cqv8oDecLm70jqDEgYJSGKVQSxgvu5mZXXuCDgH5M=; b=mR5X+Xe+0SZ9LWTD5Uh8900AWEXWPZ+S+F/661LI0T3Q1AmyOXnhD/9iBLPW+nuEpv zjbLFAQsz53CfQeP63zsgKLLJPcdsi+oqcWxoYrJJDMjguy3p2zSMHy1tbKzY8fxagTg 83QtYVaip4BV0PplDi+MPhkLQdOTDpsuSWzfsm2howUS7feH1jbPx9G6pwJwrB+LotGN OL8sbWCxHNDea7uMgsCjsML1QIgqWQgK7CZTaYsqhD2w+bC6x59OjoE7KEIak91UGuq0 4RrX8m8iQeSJFR/kPSnOjulbzzg6uwM2Qye5LJcCxcCrOA+wWKJqghni2hKzwjUACKBO 7JMw== X-Gm-Message-State: AOPr4FUeTjeTvNLn5pkpHG9VUfMAeZn2YQAf1CX5mPw9wAMN5hjfNB9n4bNXJblEsR2c75bRwr2lsFAllrI6s8ID MIME-Version: 1.0 X-Received: by 10.182.33.166 with SMTP id s6mr9985478obi.30.1460703826501; Fri, 15 Apr 2016 00:03:46 -0700 (PDT) Received: by 10.202.49.194 with HTTP; Fri, 15 Apr 2016 00:03:46 -0700 (PDT) In-Reply-To: References: Date: Fri, 15 Apr 2016 12:33:46 +0530 Message-ID: Subject: Re: [pgAdmin4] [Patch] Implementation of the Data Grid and Query Tool From: Akshay Joshi To: Dave Page Cc: pgadmin-hackers Content-Type: multipart/mixed; boundary=001a11c2de1cb43bf40530809c66 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 --001a11c2de1cb43bf40530809c66 Content-Type: multipart/alternative; boundary=001a11c2de1cb43bf00530809c64 --001a11c2de1cb43bf00530809c64 Content-Type: text/plain; charset=UTF-8 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-9517Mobile: +91 976-788-8246* --001a11c2de1cb43bf00530809c64 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Thu, Apr 14, 2016 at 7:40 PM, Dave Page <dave.page@enterprisedb.com> wrote:
Hi
<= br>
On Thu, Apr 14, 2016 at 1:58= PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi All=C2=A0
I have fixed review comments given by Dave and couple of t= hem 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 applic= able 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 button= s into one button. We shouldn't have two buttons that do essentially th= e same thing.
  • In Edit Grid mode, the History panel should log all queries (SELE= CTs, UPDATEs, DELETEs etc) as it would in = the Query Tool.
  • In Edit Grid mode, the Messages panel should display any messages fro= m the most recent action =C2=A0as 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 <= span>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 a= s required. Those elements should be grayed whilst it is open.
  • Please adjust the heig= ht 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 w= e 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 &q= uot;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 li= ne only), Rows affected, Runtime and Status, in a row per quer= y executed. Ashesh suggested using a sub-form that can be expa= nded 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 underli= ning for the error word.
  • Query results should have spaces converted to "&nbsp<= /span>;", so that proper indenting is maintained (for example, on EXPL= AIN queries).
  • on s= hutdown pgAdmin server ,=C2=A0appropriate message should be displayed.
    • <= /ul>

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 dr= opdown than it - and pushed the code.
=C2=A0
Remaining review comments:
  • Please add an SQL button. This should show/hide the SQL panel in *both* Qu= ery 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 the resultset.
  • Can the &quo= t;Copy Row" button also populate the clipboard with CSV d= ata for the row?
  • In Edit mode, we need to be able to represent/set values to NULL.
  • The layout o= f 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.
    • <= li>Cut, Copy, Paste functionality.=C2=A0
Agreed on the above.

My only real suggestion on the code itself (which lo= oks good and clean on a quick review), is that:

- = The button bar be moved out into an HTML template
- create.sql sh= ould perhaps be renamed to insert.sql
- It looks like we only all= ow 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 a= dditional issues if/when I find them.
<= div>
=C2=A0 =C2=A0Thanks for committing the patch. I'll w= ork on the above comments. Meanwhile I have found one issue where "Vie= w Filtered Row" is not working, so attached is the patch file to fix t= hat. Can you please review/commit it.
<= div dir=3D"ltr">
=C2=A0
--
Dave Page
VP, Chief Arc= hitect, Tools & Installers
EnterpriseDB: http://www.enterprisedb.com
The Enter= prise PostgreSQL Company

Blog: http://pgsnake.blogspot= .com
Twitter: @pgsnake



--
Akshay= Joshi<= /b>
= Principal Software Engineer=C2=A0


<= b>Phone: +91 20-3058-9517
Mobile: +91 976-788= -8246
--001a11c2de1cb43bf00530809c64-- --001a11c2de1cb43bf40530809c66 Content-Type: application/octet-stream; name="Fixed_View_Filtered_Row_issue.patch" Content-Disposition: attachment; filename="Fixed_View_Filtered_Row_issue.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_in1dahbd0 ZGlmZiAtLWdpdCBhL3dlYi9wZ2FkbWluL3Rvb2xzL2RhdGFncmlkL3RlbXBs YXRlcy9kYXRhZ3JpZC9qcy9kYXRhZ3JpZC5qcyBiL3dlYi9wZ2FkbWluL3Rv b2xzL2RhdGFncmlkL3RlbXBsYXRlcy9kYXRhZ3JpZC9qcy9kYXRhZ3JpZC5q cwppbmRleCBkZGE2MDdkLi43M2FmMjk0IDEwMDY0NAotLS0gYS93ZWIvcGdh ZG1pbi90b29scy9kYXRhZ3JpZC90ZW1wbGF0ZXMvZGF0YWdyaWQvanMvZGF0 YWdyaWQuanMKKysrIGIvd2ViL3BnYWRtaW4vdG9vbHMvZGF0YWdyaWQvdGVt cGxhdGVzL2RhdGFncmlkL2pzL2RhdGFncmlkLmpzCkBAIC0xLDYgKzEsNyBA QAogZGVmaW5lKAotICBbJ2pxdWVyeScsJ2FsZXJ0aWZ5JywgJ3BnYWRtaW4n LCAncGdhZG1pbi5icm93c2VyJywgJ3djZG9ja2VyJ10sCi0gIGZ1bmN0aW9u KCQsIGFsZXJ0aWZ5LCBwZ0FkbWluKSB7CisgIFsnanF1ZXJ5JywnYWxlcnRp ZnknLCAncGdhZG1pbicsJ2NvZGVtaXJyb3InLCAnY29kZW1pcnJvci9tb2Rl L3NxbCcsCisgICAncGdhZG1pbi5icm93c2VyJywgJ3djZG9ja2VyJ10sCisg IGZ1bmN0aW9uKCQsIGFsZXJ0aWZ5LCBwZ0FkbWluLCBDb2RlTWlycm9yKSB7 CiAgICAgLy8gU29tZSBzY3JpcHRzIGRvIGV4cG9ydCB0aGVpciBvYmplY3Qg aW4gdGhlIHdpbmRvdyBvbmx5LgogICAgIC8vIEdlbmVyYWxseSB0aGUgb25l LCB3aGljaCBkbyBubyBoYXZlIEFNRCBzdXBwb3J0LgogICAgIHZhciB3Y0Rv Y2tlciA9IHdpbmRvdy53Y0RvY2tlciwKZGlmZiAtLWdpdCBhL3dlYi9wZ2Fk bWluL3Rvb2xzL3NxbGVkaXRvci9zdGF0aWMvY3NzL3NxbGVkaXRvci5jc3Mg Yi93ZWIvcGdhZG1pbi90b29scy9zcWxlZGl0b3Ivc3RhdGljL2Nzcy9zcWxl ZGl0b3IuY3NzCmluZGV4IDZkMmQ2MjYuLjQ1NjEyZjYgMTAwNjQ0Ci0tLSBh L3dlYi9wZ2FkbWluL3Rvb2xzL3NxbGVkaXRvci9zdGF0aWMvY3NzL3NxbGVk aXRvci5jc3MKKysrIGIvd2ViL3BnYWRtaW4vdG9vbHMvc3FsZWRpdG9yL3N0 YXRpYy9jc3Mvc3FsZWRpdG9yLmNzcwpAQCAtMjMyLDYgKzIzMiw3IEBACiAg IGZvbnQtc2l6ZTogMTBweDsKICAgbGluZS1oZWlnaHQ6IDEuNDI4NTcxNDI5 OwogICBib3JkZXItcmFkaXVzOiAxMHB4OworICBjdXJzb3I6IGF1dG87CiB9 CiAKIC52aXNpYmlsaXR5LWhpZGRlbiB7Cg== --001a11c2de1cb43bf40530809c66 Content-Type: text/plain Content-Disposition: inline Content-Transfer-Encoding: 8bit MIME-Version: 1.0 -- Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers --001a11c2de1cb43bf40530809c66--