Re: Lightspeed for frmQuery and other issues. - Mailing list pgadmin-hackers

Dave Page wrote:
> -----Original Message----- From: "Andreas
> Pflug"<pgadmin@pse-consulting.de> Sent: 30/04/06 01:44:21 To:
> "pgadmin-hackers"<pgadmin-hackers@postgresql.org>, "Dave
> Page"<dpage@vale-housing.co.uk> Subject: Lightspeed for frmQuery and
> other issues.
>
>
>> as announced I realized the virtual listview implementation in
>> frmQuery; the code became amazingly slim. Since there's no more
>> data retrieval, the retrieval time shrinks to precisely 0.00
>> microseconds which should be sufficiently fast to justify omitting
>> this value :-)
>
>
> Cool, nice work. I assume all the clipboard stuff still works?

Depends on what you call "the clipboard stuff". Everything that worked
in 1.4 still works, i.e. single/multiple row copy. To select columns,
there's a special syntax to the SELECT command....

>
>
>> I left grid stuff #ifdef'ed in the source, but it can't work for
>> now. When this is reworked, I'd really appreciate if the interface
>> of ctlSQLResult isn't altered again (there shouldn't be a need to
>> touch frmQuery), and until a different solution is accepted the
>> alternate #define USE_LISTVIEW should remain.
>
>
> If it is fully working, I see no reason to change it again. The only
> other mod I had in mind was full/multiple row pasting in the edit
> grid.

pasting?

>
>
>> At the same time, I noticed how reporting was added to pgAdmin, and
>> was quite horrified. The menu refactoring was done to avoid base
>> class
>
>
> Had a feeling you would be.
>
>
>> cluttering, and now it is massively reinvented. Any adding to
>> menu.h is plain wrong for frmMain menu, any code adding in
>> events.cpp (beyond minor submenu handling, i.e. calling
>> enableSubmenu) too, touching base factory classes even more. All
>> reporting stuff should be implemented in frmReport, not in pgObject
>> or so. Please do refactor it using factories.
>
>
> Well the code was modelled as closely as possible on the 'new object'
> menu code, and given the total lack of comments or other documentaion
> of the factory code it's not exactly easy to understand either intent
> or implementation.

You could have asked...
>
> Here's (from memory) what was done:
>
> - The new menu was added using the menu factories, per the new object
> menu.

Reporting isn't comparable to that. See Edit(filtered/lmited)Grid (or
the new scripting stuff)

>
> - menu ids were added in menu.h because multiple object types need to
> share specific IDs for the same report.

menu ids are supposed to be handled by the factory.
>
> - Each object type, via the base class provides it's own menu, per
> the new object menu.
>
> - event.cpp has a minimal handler for the menu.


>
> - Each report is produced by the object itself (because that's where
> the info is, per the main UI views.

No, it isn't; its switch-coded in the base class. Certainly ok to add
some object specific helper methods to pgXXX classes, but not to create
a form from pgObject.

>
> - Properties/stats reports etc. Are implemented in
> pgObject/pgCollection to minimise code duplication.

Common methods might go here (AFAICS none is necessary), but the work
itself should be done outside. object->CreateReport is the wrong style
to do that; in the sense of the menu factory stuff it's an action that's
performed on an object, not an object's method.

To be precise: pgObject, pgCollection, should be rolled back
except for HasXXX helper methods, base/xxx completely.

>
> If there's something wrong with any particular part of that you'll
> need to explain why, and how you think it should be don in a lot more
> detail, 'cos as far as I can see I've followed existing design pretty
> closely.

No, you did the opposite; you modified base classes that aren't supposed
to be touched. To add features like this is what has to be done:

- add frmXXX with its factories
   Every factory (one factory per menu entry!) has
   - constructor for use in frmMain
   - CheckEnable (virtual)
   - StartDialog (virtual)
- add instantiating the factories in frmMain
- when a new submenu is involved, add enableSubmenu(xxx) to events.cpp.

Actually, even touching events.cpp is too much, should done by
registering it in frmMain too. I'll refactor that, so that only *one*
single core file has to be modified.

Since frmQuery et al aren't supposed to be extended too often, the usual
MNU_XXX style is to be used there, so MNU_QUICKREPORT is fine.

>
>
>> BTW, I wonder why the reporting is creating HTML, not XML.
>
>
> Because XML is good for data exchange, not presentation. You will
> note that it is XHTML 1.0 Strict though.

Isn't XSL/XSLT supposed to deliver the specific rendering? The current
implementation looks very special to me, not reusable e.g. for a
compilation of multiple reports.

Another topic: I realized that the maxRows option (which is obsolete for
frmQuery now) is used for some job statistics. I'm not sure if using
that limit is a good idea.

Regards,
Andreas