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

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.

*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.

Attached is the patch file which contains complete DataGrid and Query Tool
code with fixed review comments. Can you please review the patch and let me
know the review comments if any else can you please commit the code if it
looks good to you.

On Tue, Apr 12, 2016 at 6:10 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Very nice!
>
>
> On Tue, Apr 12, 2016 at 1:11 PM, Akshay Joshi <
> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>
>> Hi Dave
>>
>> As per suggestion, I have changed History tab to be a grid. Attached is
>> the two screenshot one for Query Tool and other is for the Data Grid where
>> some of the transaction gets rollback while saving the data due to one of
>> the error condition. Please review the screenshots and let me know the
>> comments if any.
>>
>> On Fri, Apr 8, 2016 at 9:49 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>>
>>>
>>> On Fri, Apr 8, 2016 at 3:39 PM, Akshay Joshi <
>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Dave
>>>>
>>>> On Fri, Apr 8, 2016 at 2:32 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Fri, Apr 8, 2016 at 7:43 AM, Akshay Joshi <
>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>
>>>>>>>>>>> - The Query Tool menu icon should be a glyphicon, to match the
>>>>>>>>>>> others.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> There is no glyphicon available which match the Query Tool
>>>>>>>>>> icon. I have found one like below which is "database-search" or can you
>>>>>>>>>> please suggest some other icon.
>>>>>>>>>> [image: Inline image 1]
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> That one looks perfect.
>>>>>>>>>
>>>>>>>>
>>>>>> We can't use this icon because it's not come with
>>>>>> Bootstrap , I have picked this from "http://glyphicons.com/" and I
>>>>>> am not sure we can use it as per the Licence.
>>>>>>
>>>>>
>>>>> At the risk of annoying everyone immensely, on reflection I'm thinking
>>>>> we should use Font Awesome as our default generic icons, and fall back to
>>>>> Bootstrap's Glyphicons. I really hadn't realised how much larger the FA set
>>>>> is.
>>>>>
>>>>> For this particular issue, could we use FA's stacking? e.g. something
>>>>> like: https://jsfiddle.net/pa8x6nt3/
>>>>>
>>>>> If not, how about using the execute icon we discussed, e.g. fa-bolt?
>>>>>
>>>>
>>>> I have used fa-bolt. Apart from that I have completed below review
>>>> comments
>>>>
>>>
>>> OK.
>>>
>>>
>>>>
>>>>
>>>> - 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. *Note*: - With current implementation
>>>> "Object" menu is re-created dynamically depending on the node clicked, so
>>>> we will have to re-create the "View Data" menu as well and it require
>>>> change in the generic code. For the time being "View Data" menu is visible
>>>> in "Object" menu when appropriate node is selected.
>>>> - 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, that textbox should be read-only, but should
>>>> display the SQL used (including any LIMIT/FILTER clauses). Please
>>>> refer the attached screenshot (Modified-Data-Grid).
>>>> - 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? *Note*: - As discussed I have added
>>>> one more container to show the title. Please refer the attached screenshots
>>>> (Modified-Query-Tool)
>>>> - Query results should have spaces converted to "&nbsp;", so that
>>>> proper indenting is maintained (for example, on EXPLAIN queries)
>>>> - To fix this we have added css style "*white-space: pre-wrap;*",
>>>> but it changes the backgrid cell size. Please refer the
>>>> screenshot (Modified-Data-Grid).
>>>>
>>>> Hmmm. I'd rather that the heights didn't change (or at least the rows
>>> were resizeable like in pga3. Can we set the column to hide vertical
>>> overflow, unless it gets focus or something?
>>>
>>>
>>>> Please review the screenshots and please let me know will it looks
>>>> good.
>>>>
>>>
>>> Bar the cell size, certainly it's good :-)
>>>
>>> --
>>> Dave Page
>>> Blog: http://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EnterpriseDB UK: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>
>>
>>
>> --
>> *Akshay Joshi*
>> *Principal Software Engineer *
>>
>>
>>
>> *Phone: +91 20-3058-9517 <%2B91%2020-3058-9517>Mobile: +91 976-788-8246*
>>
>
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

--
*Akshay Joshi*
*Principal Software Engineer *

*Phone: +91 20-3058-9517Mobile: +91 976-788-8246*