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

Поиск
Список
Период
Сортировка
От Akshay Joshi
Тема Re: [pgAdmin4] [Patch] Implementation of the Data Grid and Query Tool
Дата
Msg-id CANxoLDcxHDoe0yebsUE4rJ=8Rxrz8FdVYM1Zp8uWw2YkD0CvXw@mail.gmail.com
обсуждение исходный текст
Ответ на Re: [pgAdmin4] [Patch] Implementation of the Data Grid and Query Tool  (Dave Page <dave.page@enterprisedb.com>)
Ответы Re: [pgAdmin4] [Patch] Implementation of the Data Grid and Query Tool  (Dave Page <dave.page@enterprisedb.com>)
Список pgadmin-hackers


On Thu, Apr 14, 2016 at 7:40 PM, Dave Page <dave.page@enterprisedb.com> 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 "&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
Mobile: +91 976-788-8246
Вложения

В списке pgadmin-hackers по дате отправления:

Предыдущее
От: Ashesh Vashi
Дата:
Сообщение: Re: Dialogues not closing
Следующее
От: Dave Page
Дата:
Сообщение: Re: Dialogues not closing