Re: [pgAdmin4] [Patch] Implementation of the Data Grid and Query Tool

Hi

On Thu, Apr 14, 2016 at 1:58 PM, Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)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 "&nbsp;", 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