Thanks - committed. On Fri, Apr 15, 2016 at 8:03 AM, Akshay Joshi <[email protected] > wrote:
> > > 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-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
