public inbox for [email protected]
help / color / mirror / Atom feedFrom: Akshay Joshi <[email protected]>
To: Dave Page <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: [pgAdmin4] [Patch] Implementation of the Data Grid and Query Tool
Date: Fri, 15 Apr 2016 12:33:46 +0530
Message-ID: <CANxoLDcxHDoe0yebsUE4rJ=8Rxrz8FdVYM1Zp8uWw2YkD0CvXw@mail.gmail.com> (raw)
In-Reply-To: <CA+OCxoyaQsfpJk+XfpvSGc=Vf9i4o4SjW+g_H9tChYKsbiCf=Q@mail.gmail.com>
References: <CANxoLDe8fRZ=m8NGPc0=YQLS0vTc4-L5xmjyn2J2V1jUt937Mg@mail.gmail.com>
<CA+OCxoz4ejTML6cjwMvOLd7vg4Dcd1koz30StnH_huND61gdzg@mail.gmail.com>
<CANxoLDemUxkGL_z6=OPkA=3E-+ZDDmA3oht2wgDoCmurVv3JYQ@mail.gmail.com>
<CA+OCxoxOOW4j0o+_NsV7JoW+-3aQ_As_xC90U3ShGw_DgWgLvQ@mail.gmail.com>
<CANxoLDcp=sjd=P8U7qUP3tOQThAXDx+GUFyoE6F9xNDKwa5MnA@mail.gmail.com>
<CA+OCxozK661Ha4RD51FtxmEdTzBd2h0nVgemAhfh0opbYOTerA@mail.gmail.com>
<CANxoLDcxkQ+T8XwpVabDo2=E5P0x3e27Gz4Ob3NAsi_9C=m14w@mail.gmail.com>
<CA+OCxowdBvsnin91F2chyWLaH1ivEZmD_N_oY2DEJ11jEe3OjQ@mail.gmail.com>
<CANxoLDd0Ljz7Oq6uaJo181uiAixXHsEpDQr3nFi8VyisYx-9sQ@mail.gmail.com>
<CA+OCxoz0Y-So4gOLVG2UVUEKBu-f9wAh0iEyMPfXH8ao-4Ai6w@mail.gmail.com>
<CANxoLDcmtT5zUyXAU_HvLcoJjFeYWfRx2egLWp_3rdwOsKXV_w@mail.gmail.com>
<CA+OCxowg-0gM-SkmfFmWZDeHAJvhYtuyWdRWHtxrn_sp0rjAPA@mail.gmail.com>
<CANxoLDe-pk8=s0JkW=9ccr5LBx-Fyh-vrPQ-hqLpYYjqfoUycw@mail.gmail.com>
<CA+OCxoyaQsfpJk+XfpvSGc=Vf9i4o4SjW+g_H9tChYKsbiCf=Q@mail.gmail.com>
List-Unsubscribe: <mailto:[email protected]?body=unsub%20pgadmin-hackers>
On Thu, Apr 14, 2016 at 7:40 PM, Dave Page <[email protected]>
wrote:
> Hi
>
> On Thu, Apr 14, 2016 at 1:58 PM, Akshay Joshi <
> [email protected]> 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*
--
Sent via pgadmin-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
Attachments:
[application/octet-stream] Fixed_View_Filtered_Row_issue.patch (1.1K, 3-Fixed_View_Filtered_Row_issue.patch)
download | inline diff:
diff --git a/web/pgadmin/tools/datagrid/templates/datagrid/js/datagrid.js b/web/pgadmin/tools/datagrid/templates/datagrid/js/datagrid.js
index dda607d..73af294 100644
--- a/web/pgadmin/tools/datagrid/templates/datagrid/js/datagrid.js
+++ b/web/pgadmin/tools/datagrid/templates/datagrid/js/datagrid.js
@@ -1,6 +1,7 @@
define(
- ['jquery','alertify', 'pgadmin', 'pgadmin.browser', 'wcdocker'],
- function($, alertify, pgAdmin) {
+ ['jquery','alertify', 'pgadmin','codemirror', 'codemirror/mode/sql',
+ 'pgadmin.browser', 'wcdocker'],
+ function($, alertify, pgAdmin, CodeMirror) {
// Some scripts do export their object in the window only.
// Generally the one, which do no have AMD support.
var wcDocker = window.wcDocker,
diff --git a/web/pgadmin/tools/sqleditor/static/css/sqleditor.css b/web/pgadmin/tools/sqleditor/static/css/sqleditor.css
index 6d2d626..45612f6 100644
--- a/web/pgadmin/tools/sqleditor/static/css/sqleditor.css
+++ b/web/pgadmin/tools/sqleditor/static/css/sqleditor.css
@@ -232,6 +232,7 @@
font-size: 10px;
line-height: 1.428571429;
border-radius: 10px;
+ cursor: auto;
}
.visibility-hidden {
view thread (18+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: [pgAdmin4] [Patch] Implementation of the Data Grid and Query Tool
In-Reply-To: <CANxoLDcxHDoe0yebsUE4rJ=8Rxrz8FdVYM1Zp8uWw2YkD0CvXw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox