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

Thanks - committed.

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

>
>
> On Thu, Apr 14, 2016 at 7:40 PM, Dave Page <dave(dot)page(at)enterprisedb(dot)com>
> wrote:
>
>> 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.
>>
>
> 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