Re: Lightspeed for frmQuery and other issues.

-----Original Message-----
From: "Andreas Pflug"<pgadmin@pse-consulting.de>
Sent: 30/04/06 12:24:08
To: "Dave Page"<dpage@vale-housing.co.uk>
Cc: "pgadmin-hackers@postgresql.org"<pgadmin-hackers@postgresql.org>
Subject: Re: Lightspeed for frmQuery and other issues.

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

Then you have some work to finish - both the edit and query grid could copy individual cells, rows, columns or
arbitrarygroups of cells to the clipboard - functionality that I for one am already using on a regular basis. 

Please fix.

>  pasting?

Pasteing? I dunno. Anyway in MS Query Analyser you can paste entire rows, or sets of new rows into a table. I had been
planningon looking at something similar, but with an additional option (offered at paste time, when necessary) to skip
OID/serialcolumns. 

> You could have asked...

You've been somewhat unreceptive to questions recently :-(

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

OK

> menu ids are supposed to be handled by the factory.

How would that work - have a factory for each report type?

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

So therefore it's in the object. An object is an instance of the sum of the class and all its base classes.

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

So, the code goes where - in frmReport? That would require it to have knowledge of each object type which is exactly
whyI put left that info in the object classes (note that at the moment there are only generic reports of course - there
willbe some object specific ones in the future). 

> - add frmXXX with its factories
>    Every factory (one factory per menu entry!) has

Menu entry == report type, or report type per object type?

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

Ok, (I think).

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

XSL/XSLT could be used, but I really don't see the need. These are only meant as quick outputs for the user to print,
ordrop onto a website or whatever. The internal API of frmReport allows you to include multiple report sections if
that'swhat you are thinking of. 

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

Oh, you mean limiting the number of stats rows retrieved? Got a better idea? Hardcode a value, or just remove it
altogether?

/D

-----Unmodified Original Message-----
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