Re: New Patch for GQB - Mailing list pgadmin-hackers

On Tue, Aug 12, 2008 at 2:33 PM, Luis Ochoa <ziul1979@gmail.com> wrote:
> New Patch for GQB, improvements and bugs fixed.
>
> -. Better Destructors.
> .- Fix Source Code Comments
> -. Views not available
> .- Move Images to images folder
>
> http://rapidshare.com/files/136775512/OnlyPatch.zip.html
>
> P.S. I have binary version for windows please write an email if you want,
> and in visual studio should be created two filters gqb and include/gqb and
> add files in same folders (or use projects provided on patch).
>
> Any Comments or suggestions?

Yup - looking much better :-). FYI, I expect to be tweaking this for a
short while after GSoC finishes before it's committed - I hope you'll
still be helping out! For what it's worth, I do consider the project a
success, even though it's not 100% complete yet.

So, comments:

- The treeview doesn't resize vertically

- As you note, there are no views there yet :-(

- Hitting F5 on the treeview should refresh it. It *shouldn't*
re-execute the query in the other tab!

- Hitting F5 on the diagram (or pressing the Execute/Execute to file
button should create the query and execute (asking the user for
permission to update the query if the text is modified). I don't think
we need a separate button.

- The SQL formatting has got it's indents backwards (note also that we
use a 2 space indent for SQL)!

 SELECT
pg_class.relnamespace,
foo.data
 FROM
public.foo,
pg_catalog.pg_class
 WHERE
pg_class.relname = foo.id
 ORDER BY
foo.id ASC;

should be:

SELECT
  pg_class.relnamespace,
  foo.data
FROM
  public.foo,
  pg_catalog.pg_class
WHERE
  pg_class.relname = foo.id
ORDER BY
  foo.id ASC;

- The font in the tables still looks a little small (I'm on Windows today).

- The 'Order Criteria' label should probably be 'Ordering' or 'Sorting'

- The column headers on the 'Selected Columns and Order Criteria tabs
are twice as high as the headers on the Columns Criteria tab (which
looks the best imho and is least wasteful of space).

- If I add a table and it lands on another (which isn't currently
selected), if I try to pick up the new one which is on the top, the
bottom one actually gets focus.

- There seems to be some gratuitous white-space changes in the code.
We use 4-space indents in the code, with public/private labels in
class declarations fully left justified, and the actual items indented
once.

- Comments should generally be indented at the same level as the code
they refer to. It's also a little clearer to leave a space between the
// and the text.

- I spotted at least one pointer to a wxString being passed around (in
gqbBrowser::refreshTables). You need to fix those as it breaks
reference counting and is less efficient, per the wxWidgets docs
(http://docs.wxwidgets.org/stable/wx_wxstringoverview.html#wxstringoverview)

All in all - very good progress though. I think only the F5 and table
selection issues are at all serious - the rest are relatively minor
issues.

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com