Re: New Patch for GQB

Hi, Dave.

I read your commentaries a few days ago but I can't reply at that time.
:)

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

Yes, :( I tried to finish everything but I think that even with
that two features missing (having and group by) the GQB will be a tool
that might attract more people to use postgreSQL. But for future
generations or people that wants to contribute to postgresql projects.
I leave this little text explaning from my point of view the causes of
that issue, as a way this persons don't make same mistake again. At
least I hope this :)

I think delay problem was created because lacks of a good user
interface design from the begining by me. Because sometimes I take
decisions without consulting anyone's opinion. And after implementing
that features just found that don't works in the way I wanted it. Then
I have to implement again that feature, doing a big of changes of core
parts of the project, and losing time that I must use to work on new
features. This was the biggest problem from my point of view. Still I
can finish that features but this will probably left the latest
release on GSOC on a more unstable way, and that is not what I want to
do it. Now I understand Dave's statments about let more time for
testing :( ... but it's too late... :'(

> So, comments:
>
> - The treeview doesn't resize vertically

My mistake, this is corrected only when output panel is closed. I'm
going to fix it.

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

I'm going to try to add this feature, is very easy to add, but I
really don't fully understand the data dictionary from postgreSQL and
don't want to introduce a new errors on the database reverse
engineering part. But I hope this should be implemented for latest
release of GSOC (but if not, this will be implemented after GSOC not
problem)

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

This sound more easy, than really it's, because that tree it's the
only data structure that holds all base objects from gqb (tables that
contains the columns). And I need to add something new as base object
like a hash table or something like that, and then construct the tree
from this new data structure and not directly as I'm doing right now.
Dave do you think this change should be done at this moment or after
GSOC ends?

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

Right, is a good point. Excellent I'm going to implement this.

> - 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;
>
>
>
No problem, I'm going to fix it.

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

But I'm using get system font function yet? can you help me to think
other way of accomplish this goal?

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

Ok.

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

Yes, and I don't notice that because I was focus on others aspects of
GQB. I'm going to fix it.

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

Yes, it's a problem of my MVC pattern implementation, I'm going to fix it.

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

ok I'm going to fix it.

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

Should I leave the TODO items or remove it from source code leaving
only real comments?

> - 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)

Yes, you told this sometime ago, but I don't fixed it until now, I'm
going to do it.

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

Yes, but as I explain before if I refresh the tables browser tree when
F5 is pressed then all tables objects are going to be replaced and the
sentence you're working on GQB is going to be erased leaving the
canvas part of GQB empty (or I should create a probably too complicate
sync function between data dictionary & the tree). And again, The only
easy way I think, it is create a new data structure that holds this
info instead the same tree (because reasons I explain above).? and the
last one fix this before or after the last delivery of GSOC? any
suggestion on this point?

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

Thanks for your opinions Dave.

Regards,
Luis.