Re: Lightspeed for frmQuery and other issues.

-----Original Message-----
From: "Andreas Pflug"<pgadmin(at)pse-consulting(dot)de>
Sent: 30/04/06 12:24:08
To: "Dave Page"<dpage(at)vale-housing(dot)co(dot)uk>
Cc: "pgadmin-hackers(at)postgresql(dot)org"<pgadmin-hackers(at)postgresql(dot)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 arbitrary groups 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 planning on looking at something similar, but with an additional option (offered at paste time, when necessary) to skip OID/serial columns.

> 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 why I put left that info in the object classes (note that at the moment there are only generic reports of course - there will be 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, or drop onto a website or whatever. The internal API of frmReport allows you to include multiple report sections if that's what 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(at)pse-consulting(dot)de> Sent: 30/04/06 01:44:21 To:
> "pgadmin-hackers"<pgadmin-hackers(at)postgresql(dot)org>, "Dave
> Page"<dpage(at)vale-housing(dot)co(dot)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