Thread: Lightspeed for frmQuery and other issues.
Hi Dave, 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 :-) 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. 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 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. BTW, I wonder why the reporting is creating HTML, not XML. I added a scripting option, which will (for std. objects) call the query tool with reengineered script as usually performed with the sticky bit set. IMNSHO we can drop the option now, because both behaviours are now available at the same time. Scripting of SELECT, INSERT and UPDATE (typing the col names can be boring) coming soon. Regards, Andreas
-----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? > 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 pastingin the edit grid. > 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 orother documentaion of the factory code it's not exactly easy to understand either intent or implementation. Here's (from memory) what was done: - The new menu was added using the menu factories, per the new object menu. - menu ids were added in menu.h because multiple object types need to share specific IDs for the same report. - 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. - Properties/stats reports etc. Are implemented in pgObject/pgCollection to minimise code duplication. If there's something wrong with any particular part of that you'll need to explain why, and how you think it should be donin a lot more detail, 'cos as far as I can see I've followed existing design pretty closely. > 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. > I added a scripting option, which will (for std. objects) call the query > tool with reengineered script as usually performed with the sticky bit > set. IMNSHO we can drop the option now, because both behaviours are now > available at the same time. Scripting of SELECT, INSERT and UPDATE > (typing the col names can be boring) coming soon. Cool. Regards, Dave -----Unmodified Original Message----- Hi Dave, 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 :-) 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. 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 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. BTW, I wonder why the reporting is creating HTML, not XML. I added a scripting option, which will (for std. objects) call the query tool with reengineered script as usually performed with the sticky bit set. IMNSHO we can drop the option now, because both behaviours are now available at the same time. Scripting of SELECT, INSERT and UPDATE (typing the col names can be boring) coming soon. Regards, Andreas
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
-----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
Dave Page wrote: > > -----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 arbitrary groups of > cells to the clipboard - functionality that I for one am already > using on a regular basis. EditGrid is untouched. copying of arbitrary groups of cells did *never* work correct. The current implementation will omit cells if it doesn't know how to handle it. This is not acceptable: The ctl needs to be rewritten to not accept selections that can't be executed. As I already may have mentioned earlier, I don't intend to spent time on the crappy wxGrid. So if anybody else likes to do so, he/she should pay attention not to change the ctlSQLResult interface for that. Regards, Andreas
-----Original Message----- From: "Andreas Pflug"<pgadmin@pse-consulting.de> Sent: 30/04/06 14:48: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. > EditGrid is untouched. > copying of arbitrary groups of cells did *never* work correct. The > current implementation will omit cells if it doesn't know how to handle > it. This is not acceptable: The ctl needs to be rewritten to not accept > selections that can't be executed. I've never run into such a bug, nor has it been reported. How can it be reproduced? Had it been reported, it would have beenlooked at of course. > As I already may have mentioned earlier, I don't intend to spent time on > the crappy wxGrid. So if anybody else likes to do so, he/she should pay > attention not to change the ctlSQLResult interface for that. With or without a bug, removing useful functionality is completely unnacceptable. You would be understandably pissed offif a feature you used was removed, and I would ensure the author reimplemented it, or rollback the change. Bug aside,please fix the copy to clipboard to match what was there. /D -----Unmodified Original Message----- Dave Page wrote: > > -----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 arbitrary groups of > cells to the clipboard - functionality that I for one am already > using on a regular basis. EditGrid is untouched. copying of arbitrary groups of cells did *never* work correct. The current implementation will omit cells if it doesn't know how to handle it. This is not acceptable: The ctl needs to be rewritten to not accept selections that can't be executed. As I already may have mentioned earlier, I don't intend to spent time on the crappy wxGrid. So if anybody else likes to do so, he/she should pay attention not to change the ctlSQLResult interface for that. Regards, Andreas
Dave Page wrote: > > -----Original Message----- From: "Andreas > Pflug"<pgadmin@pse-consulting.de> Sent: 30/04/06 14:48: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. > > >> EditGrid is untouched. copying of arbitrary groups of cells did >> *never* work correct. The current implementation will omit cells if >> it doesn't know how to handle it. This is not acceptable: The ctl >> needs to be rewritten to not accept selections that can't be >> executed. > > > I've never run into such a bug, nor has it been reported. How can it > be reproduced? Simply select some non-adjacent cells in arbitrary cols and rows. Had it been reported, it would have been looked at of > course. > > >> As I already may have mentioned earlier, I don't intend to spent >> time on the crappy wxGrid. So if anybody else likes to do so, >> he/she should pay attention not to change the ctlSQLResult >> interface for that. > > > With or without a bug, removing useful functionality is completely > unnacceptable. You would be understandably pissed off if a feature > you used was removed, and I would ensure the author reimplemented it, > or rollback the change. Bug aside, please fix the copy to clipboard > to match what was there. The implementation did not meet the primary requirement, so I objected clearly and early. You decided to commit an half-brewn "enhancement" based on a technique that was known not to meet the TODO, i.e. doing it with a virtual control. You can't be too surprised I basically rolled back that beast, to have the first step made before the second. The reporting stuff is going to frustrate you and me as well. I tried to fix at least the menuing stuff, but had to retract, realizing that you kind-of reinvented the most ugly interaction between the MNU_ enumeration and schema objects acting on that I removed months ago. This basically means that it can't be refactored partially, but essentially needs rewrite. The current implementation style is that of pgAdmin 0.1 to 1.2, which was becoming more and more painful when adding new features. I'm less than inclined to accept having this creeping back in. Regards, Andreas
-----Original Message----- From: "Andreas Pflug"<pgadmin@pse-consulting.de> Sent: 30/04/06 15:27:52 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. > Simply select some non-adjacent cells in arbitrary cols and rows. That shouldn't work - there's no sane way to handle it. It should be prevented though. > The implementation did not meet the primary requirement, so I objected > clearly and early. You decided to commit an half-brewn "enhancement" > based on a technique that was known not to meet the TODO, i.e. doing it > with a virtual control. You can't be too surprised I basically rolled > back that beast, to have the first step made before the second. When asked for further info about *that* todo, you didn't give any helpful answers :-( However, I am not complaining about that change - I am complaining that a copy/paste feature was removed - that is unrelatedto the virtual listview/grid issue. > The reporting stuff is going to frustrate you and me as well. I tried to > fix at least the menuing stuff, but had to retract, realizing that you > kind-of reinvented the most ugly interaction between the MNU_ > enumeration and schema objects acting on that I removed months ago. This > basically means that it can't be refactored partially, but essentially > needs rewrite. The current implementation style is that of pgAdmin 0.1 > to 1.2, which was becoming more and more painful when adding new > features. I'm less than inclined to accept having this creeping back in. Like I said, I'll look at changing it to work like the other menus you suggested - but if you're saying the factories cannotdo what is required, then they either need fixing, or it can stay as is. /D -----Unmodified Original Message----- Dave Page wrote: > > -----Original Message----- From: "Andreas > Pflug"<pgadmin@pse-consulting.de> Sent: 30/04/06 14:48: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. > > >> EditGrid is untouched. copying of arbitrary groups of cells did >> *never* work correct. The current implementation will omit cells if >> it doesn't know how to handle it. This is not acceptable: The ctl >> needs to be rewritten to not accept selections that can't be >> executed. > > > I've never run into such a bug, nor has it been reported. How can it > be reproduced? Simply select some non-adjacent cells in arbitrary cols and rows. Had it been reported, it would have been looked at of > course. > > >> As I already may have mentioned earlier, I don't intend to spent >> time on the crappy wxGrid. So if anybody else likes to do so, >> he/she should pay attention not to change the ctlSQLResult >> interface for that. > > > With or without a bug, removing useful functionality is completely > unnacceptable. You would be understandably pissed off if a feature > you used was removed, and I would ensure the author reimplemented it, > or rollback the change. Bug aside, please fix the copy to clipboard > to match what was there. The implementation did not meet the primary requirement, so I objected clearly and early. You decided to commit an half-brewn "enhancement" based on a technique that was known not to meet the TODO, i.e. doing it with a virtual control. You can't be too surprised I basically rolled back that beast, to have the first step made before the second. The reporting stuff is going to frustrate you and me as well. I tried to fix at least the menuing stuff, but had to retract, realizing that you kind-of reinvented the most ugly interaction between the MNU_ enumeration and schema objects acting on that I removed months ago. This basically means that it can't be refactored partially, but essentially needs rewrite. The current implementation style is that of pgAdmin 0.1 to 1.2, which was becoming more and more painful when adding new features. I'm less than inclined to accept having this creeping back in. Regards, Andreas
Quoting Andreas Pflug <pgadmin@pse-consulting.de>: > Simply select some non-adjacent cells in arbitrary cols and rows. I stated that in the beginning, and asked if anyone had any suggestions on what to do in that case. If you completely arbitrarily hightlight cells, there really isn't a way to store them in the clipboard. Because no one had any suggestions on how to handle it, yourself included, the decision was made to just copy the first group of cells highlighted. > The implementation did not meet the primary requirement, so I objected > clearly and early. You decided to commit an half-brewn "enhancement" > based on a technique that was known not to meet the TODO, i.e. doing it > with a virtual control. You can't be too surprised I basically rolled > back that beast, to have the first step made before the second. Users dont care about virtual controls or not. Users care about basic functionality like being able to copy subsets of data into the clipboard. Only behing able to copy entire rows at a time is a half brewn implementation. It would have been much easier for you to refactor things into a virtual table than to do this, and it would've made everyone happy. Also, you're forgetting that the only reason I didn't implement code similar to this originally is because you were rather insistant that the data retrieval code should not be changed in any way. It's not exactly reasonable to place restrictions on other people's work simply so that you can later complain that the work isn't good enough and have an excuse to rip it out. Ed ---------------------------------------------------------------- This message was sent using IMP, the Internet Messaging Program.
Dave Page wrote: > > When asked for further info about *that* todo, you didn't give any > helpful answers :-( Well, anybody not knowing how to code a virtual ListView shouldn't start on that... > > However, I am not complaining about that change - I am complaining > that a copy/paste feature was removed - that is unrelated to the > virtual listview/grid issue. It wasn't removed explicitely, but the underlying class that didn't meet the requirement was backed out. You've painted the wall before wallpapering it. > > >> The reporting stuff is going to frustrate you and me as well. I >> tried to fix at least the menuing stuff, but had to retract, >> realizing that you kind-of reinvented the most ugly interaction >> between the MNU_ enumeration and schema objects acting on that I >> removed months ago. This basically means that it can't be >> refactored partially, but essentially needs rewrite. The current >> implementation style is that of pgAdmin 0.1 to 1.2, which was >> becoming more and more painful when adding new features. I'm less >> than inclined to accept having this creeping back in. > > > Like I said, I'll look at changing it to work like the other menus > you suggested - but if you're saying the factories cannot do what is > required, then they either need fixing, or it can stay as is. It's not a problem of the factories, they do what they should and will work *if used*. But you basically circumvented them, and did all the stuff they're supposed to do once again. Regards, Andreas
Quoting Dave Page <dpage@vale-housing.co.uk>: > 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. Enterprise Manager handles the issue by skipping over any identity (serial) columns when pasting rows. That's how I figured it should be done. OID's would of course be treated the same way. I was also planning on looking into it when I had a chance to get back to pgAdmin. If you want, I should be able to look into it midweek or so. Let me know if you want to do it or if I should. Ed ---------------------------------------------------------------- This message was sent using IMP, the Internet Messaging Program.
Edward Di Geronimo Jr. wrote: > Quoting Andreas Pflug <pgadmin@pse-consulting.de>: > >> Simply select some non-adjacent cells in arbitrary cols and rows. > > > I stated that in the beginning, and asked if anyone had any suggestions > on what to do in that case. If you completely arbitrarily hightlight > cells, there really isn't a way to store them in the clipboard. Because > no one had any suggestions on how to handle it, yourself included, the > decision was made to just copy the first group of cells highlighted. I still don't see a need for that extended handling, because the ctl always allowed row selections (and column selections can be achieved from SELECT ...., a basic SQL feature... ) > >> The implementation did not meet the primary requirement, so I objected >> clearly and early. You decided to commit an half-brewn "enhancement" >> based on a technique that was known not to meet the TODO, i.e. doing it >> with a virtual control. You can't be too surprised I basically rolled >> back that beast, to have the first step made before the second. > > > Users dont care about virtual controls or not. They do. It's the speed issue, esp. on non-win32. > Users care about basic > functionality like being able to copy subsets of data into the > clipboard. Only behing able to copy entire rows at a time is a half > brewn implementation. It would have been much easier for you to > refactor things into a virtual table Wrong. Look at the current implementation, which basically takes version 5021 and *removes* 100 lines of code. > Also, you're forgetting that the only reason I didn't implement code > similar to this originally is because you were rather insistant that > the data retrieval code should not be changed in any way. It's not > exactly reasonable to place restrictions on other people's work simply > so that you can later complain that the work isn't good enough and have > an excuse to rip it out. Not quite, I always insisted on doing it "the virtual way" and made quite clear that I'd enforce it. Explaining how to do it required more work than to do it, I said that earlier. See how it works now, and do it with wxGrid if you like. Regards, Andreas
Edward Di Geronimo Jr. wrote: > Quoting Dave Page <dpage@vale-housing.co.uk>: > >> 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. > > > Enterprise Manager handles the issue by skipping over any identity > (serial) columns when pasting rows. That's how I figured it should be > done. OID's would of course be treated the same way. > > I was also planning on looking into it when I had a chance to get back > to pgAdmin. If you want, I should be able to look into it midweek or > so. Let me know if you want to do it or if I should. So this was a frmEditGrid issue, not frmQuery. I didn't touch that. Please note that there are many editing issues here (very platform specific), which is, I know it's boring to read again, a wxGrid issue. Regards, Andreas
Quoting Andreas Pflug <pgadmin@pse-consulting.de>: > So this was a frmEditGrid issue, not frmQuery. I didn't touch that. > Please note that there are many editing issues here (very platform > specific), which is, I know it's boring to read again, a wxGrid issue. Yes, it's Edit Grid. I fully realize wxGrid sucks, but, wxWidgets as a whole hasn't come across as particuarlly well designed. You have to use the tools you have, not the ones you want. Ed ---------------------------------------------------------------- This message was sent using IMP, the Internet Messaging Program.
-----Original Message----- From: "Andreas Pflug"<pgadmin@pse-consulting.de> Sent: 30/04/06 16:19:51 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. > > It wasn't removed explicitely, but the underlying class that didn't meet > the requirement was backed out. You've painted the wall before > wallpapering it. Quickreport sat over that class as well - is that now broken too? Please restore the functionality or I will back out the patch until it is completed in it's entirety. You complain about the work that led to significant speed increase from the original code, but at least we busted a gut tomake sure it didn't break any existing functionality. > It's not a problem of the factories, they do what they should and will > work *if used*. Like I said, I'll look at this. I didn't grok that the failed attempt you mentioned was only a minor rejig. /D -----Unmodified Original Message----- Dave Page wrote: > > When asked for further info about *that* todo, you didn't give any > helpful answers :-( Well, anybody not knowing how to code a virtual ListView shouldn't start on that... > > However, I am not complaining about that change - I am complaining > that a copy/paste feature was removed - that is unrelated to the > virtual listview/grid issue. It wasn't removed explicitely, but the underlying class that didn't meet the requirement was backed out. You've painted the wall before wallpapering it. > > >> The reporting stuff is going to frustrate you and me as well. I >> tried to fix at least the menuing stuff, but had to retract, >> realizing that you kind-of reinvented the most ugly interaction >> between the MNU_ enumeration and schema objects acting on that I >> removed months ago. This basically means that it can't be >> refactored partially, but essentially needs rewrite. The current >> implementation style is that of pgAdmin 0.1 to 1.2, which was >> becoming more and more painful when adding new features. I'm less >> than inclined to accept having this creeping back in. > > > Like I said, I'll look at changing it to work like the other menus > you suggested - but if you're saying the factories cannot do what is > required, then they either need fixing, or it can stay as is. It's not a problem of the factories, they do what they should and will work *if used*. But you basically circumvented them, and did all the stuff they're supposed to do once again. Regards, Andreas
Dave Page wrote: > > -----Original Message----- > From: "Andreas Pflug"<pgadmin@pse-consulting.de> > Sent: 30/04/06 16:19:51 > 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. > >>It wasn't removed explicitely, but the underlying class that didn't meet >>the requirement was backed out. You've painted the wall before >>wallpapering it. > > > Quickreport sat over that class as well - is that now broken too? That's in frmQuery only, right? If so, the local solution is fine. If it's more global, it should go somehow to dlgClasses. > > Please restore the functionality or I will back out the patch until it is completed in it's entirety. Which functionality? > > You complain about the work that led to significant speed increase from the original code, but at least we busted a gutto make sure it didn't break any existing functionality. Again: I said early enough the base was wrong. And I didn't see *any* speed increase on linux. Probably, any virtual solution will be ok, but any non-virtual solution *cant* be ok. > > >>It's not a problem of the factories, they do what they should and will >>work *if used*. > > > Like I said, I'll look at this. I didn't grok that the failed attempt you mentioned was only a minor rejig. It should have been, but things are horribly intervowen. I'd have to understand in detail what's going on to continue, because apparently removing MNU_XXX entries would kill methods too. Regards, Andreas
Quoting Andreas Pflug <pgadmin@pse-consulting.de>: > I still don't see a need for that extended handling, because the ctl > always allowed row selections (and column selections can be achieved > from SELECT ...., a basic SQL feature... ) Very often in my work, I would not know exactly what columns I need the data from until after I see the results of the query. Someone would walk into my office and say "What's wrong with Joe Smith's account?" I'd run a query to get an overview of the account, and what columns I needed would change depending on what looked to be wrong. It's a lot easier to click on the cell I need and hit copy than to run another query to narrowing things down. >> Users dont care about virtual controls or not. > > They do. It's the speed issue, esp. on non-win32. The implementation I did was 4x faster than the old implementation, which apparently had been acceptable for a long time, but also offered more features. There was no tradeoff to that work, unless you had a phobia of grids. Now you're into trading features for speed. >> Users care about basic functionality like being able to copy >> subsets of data into the clipboard. Only behing able to copy >> entire rows at a time is a half brewn implementation. It would >> have been much easier for you to refactor things into a virtual >> table > > Wrong. Look at the current implementation, which basically takes > version 5021 and *removes* 100 lines of code. Sticking the new data retrieval code into a virtual table still would have been less work than ripping out the grid and implementing a virtual list. Most of the code you removed was there either to implement the additional clipboard support your code doesn't handle, or to work around your earlier instance on keeping the retrieval the same. > Not quite, I always insisted on doing it "the virtual way" and made > quite clear that I'd enforce it. Explaining how to do it required more > work than to do it, I said that earlier. See how it works now, and do > it with wxGrid if you like. You said this months ago when I did the initial grid patch: > Please *dont* try to handle data retrieval different from what it is > now. We do have the problem of long GUI updates, but we also have > consistent behaviour on the libpq side, making it comparable to psql > and so forth. This must remain guaranteed You refused to clarify when asked what was and wasn't acceptable, and basically gave the impression that nothing could possibly satisfy you. So, the only thing that seemed reasonable to do was to disturb as little code as possible. Hence how we ended up where we're at now. Ed ---------------------------------------------------------------- This message was sent using IMP, the Internet Messaging Program.
Edward Di Geronimo Jr. wrote: > Quoting Andreas Pflug <pgadmin@pse-consulting.de>: > >> I still don't see a need for that extended handling, because the ctl >> always allowed row selections (and column selections can be achieved >> from SELECT ...., a basic SQL feature... ) > > > Very often in my work, I would not know exactly what columns I need the > data from until after I see the results of the query. Someone would > walk into my office and say "What's wrong with Joe Smith's account?" > I'd run a query to get an overview of the account, and what columns I > needed would change depending on what looked to be wrong. It's a lot > easier to click on the cell I need and hit copy than to run another > query to narrowing things down. > >>> Users dont care about virtual controls or not. >> >> >> They do. It's the speed issue, esp. on non-win32. > > > The implementation I did was 4x faster than the old implementation, > which apparently had been acceptable for a long time, but also offered > more features. There was no tradeoff to that work, unless you had a > phobia of grids. Yes I do have a wxGrid phobia. Making the frmEditGrid 80 % solution to 90 % was painful enough, and things tend to not get easier on the last 10 %. If you like fixing it in frmEditGrid, fine. The main speed issue was on gtk, and my small 1k row testcase did *not* show improvement. The current solution is ok for 10^n rows, as long as libpq and the OS can handle it. All previous versions, including yours, simply stalled. Regards, Andreas
-----Original Message----- From: "Andreas Pflug"<pgadmin@pse-consulting.de> Sent: 30/04/06 17:23:04 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. > That's in frmQuery only, right? If so, the local solution is fine. If > it's more global, it should go somehow to dlgClasses. Yes, but it used wxGrid methods to build the report, thus, like the copy functionality, would have needed fixing to workwith a listview (actually, frmReport has a listview->reporttable method that could be used). > > Please restore the functionality or I will back out the patch until it is completed in it's > > entirety. > Which functionality? The ability to copy columns and arbitrary groups of cells as well as rows. > Again: I said early enough the base was wrong. And I didn't see *any* > speed increase on linux. Probably, any virtual solution will be ok, but > any non-virtual solution *cant* be ok. The point was that it was a distinct improvement, even if not the ultimate solution - anyway, that's irrelevant now so let'smove on. > It should have been, but things are horribly intervowen. I'd have to > understand in detail what's going on to continue, because apparently > removing MNU_XXX entries would kill methods too. Are you doing this or me? I'm happy to work on it if you'll provide feedback when needed. If you're doing it though, letme know and I'll do something else. /D -----Unmodified Original Message----- Dave Page wrote: > > -----Original Message----- > From: "Andreas Pflug"<pgadmin@pse-consulting.de> > Sent: 30/04/06 16:19:51 > 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. > >>It wasn't removed explicitely, but the underlying class that didn't meet >>the requirement was backed out. You've painted the wall before >>wallpapering it. > > > Quickreport sat over that class as well - is that now broken too? That's in frmQuery only, right? If so, the local solution is fine. If it's more global, it should go somehow to dlgClasses. > > Please restore the functionality or I will back out the patch until it is completed in it's entirety. Which functionality? > > You complain about the work that led to significant speed increase from the original code, but at least we busted a gutto make sure it didn't break any existing functionality. Again: I said early enough the base was wrong. And I didn't see *any* speed increase on linux. Probably, any virtual solution will be ok, but any non-virtual solution *cant* be ok. > > >>It's not a problem of the factories, they do what they should and will >>work *if used*. > > > Like I said, I'll look at this. I didn't grok that the failed attempt you mentioned was only a minor rejig. It should have been, but things are horribly intervowen. I'd have to understand in detail what's going on to continue, because apparently removing MNU_XXX entries would kill methods too. Regards, Andreas
-----Original Message-----
From: "Edward Di Geronimo Jr."<edigeronimo@xtracards.com>
Sent: 30/04/06 16:20:56
To: "pgadmin-hackers@postgresql.org"<pgadmin-hackers@postgresql.org>
Subject: Re: [pgadmin-hackers] Lightspeed for frmQuery and other issues.
> Enterprise Manager handles the issue by skipping over any identity
> (serial) columns when pasting rows. That's how I figured it should be
> done. OID's would of course be treated the same way.
I'd like serials to be optional - for example, you could pull some rows out, manipulate in excel, then bung them back
withthe same IDs. OIDs needn't be of course.
> I was also planning on looking into it when I had a chance to get back
> to pgAdmin. If you want, I should be able to look into it midweek or
> so. Let me know if you want to do it or if I should.
If you can, that'd be good. There's plenty more for me to do :-)
/D
-----Unmodified Original Message-----
Quoting Dave Page <dpage@vale-housing.co.uk>:
> 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.
Enterprise Manager handles the issue by skipping over any identity
(serial) columns when pasting rows. That's how I figured it should be
done. OID's would of course be treated the same way.
I was also planning on looking into it when I had a chance to get back
to pgAdmin. If you want, I should be able to look into it midweek or
so. Let me know if you want to do it or if I should.
Ed
----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.
---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly
Dave Page wrote: > > > Yes, but it used wxGrid methods to build the report, thus, like the > copy functionality, would have needed fixing to work with a listview I already changed that to the new interface. > (actually, frmReport has a listview->reporttable method that could be > used). I don't quite understand that. > The ability to copy columns and arbitrary groups of cells as well as > rows. > > >> Again: I said early enough the base was wrong. And I didn't see >> *any* speed increase on linux. Probably, any virtual solution will >> be ok, but any non-virtual solution *cant* be ok. > > > The point was that it was a distinct improvement, even if not the > ultimate solution - anyway, that's irrelevant now so let's move on. An improvement that ignores basics is useless. >> It should have been, but things are horribly intervowen. I'd have >> to understand in detail what's going on to continue, because >> apparently removing MNU_XXX entries would kill methods too. > > > Are you doing this or me? I'm happy to work on it if you'll provide > feedback when needed. If you're doing it though, let me know and I'll > do something else. Since most of the work is specific, you'd better do it. I can assist with menu/general stuff, which should reduce to some "new reportSomethingFactory" in frmMain::CreateMenus(). Submenus are now handled automatically, so even the events.cpp is totally generic now. I'm going to extend programmers-readme.txt ASAP. Regards, Andreas
-----Original Message----- From: "Andreas Pflug"<pgadmin@pse-consulting.de> Sent: 30/04/06 18:44:01 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. > > (actually, frmReport has a listview->reporttable method that could be > > used). > I don't quite understand that. void frmReport::AddReportTableFromListView(ctlListView *list); > Since most of the work is specific, you'd better do it. I can assist > with menu/general stuff, which should reduce to some "new > reportSomethingFactory" in frmMain::CreateMenus(). Submenus are now >handled automatically, so even the events.cpp is totally generic now. Ok - it's a holiday weekend here though, so might not be until later this week. > I'm going to extend programmers-readme.txt ASAP. Wuh? Where's that then? Does it document the factories? /D -----Unmodified Original Message----- Dave Page wrote: > > > Yes, but it used wxGrid methods to build the report, thus, like the > copy functionality, would have needed fixing to work with a listview I already changed that to the new interface. > (actually, frmReport has a listview->reporttable method that could be > used). I don't quite understand that. > The ability to copy columns and arbitrary groups of cells as well as > rows. > > >> Again: I said early enough the base was wrong. And I didn't see >> *any* speed increase on linux. Probably, any virtual solution will >> be ok, but any non-virtual solution *cant* be ok. > > > The point was that it was a distinct improvement, even if not the > ultimate solution - anyway, that's irrelevant now so let's move on. An improvement that ignores basics is useless. >> It should have been, but things are horribly intervowen. I'd have >> to understand in detail what's going on to continue, because >> apparently removing MNU_XXX entries would kill methods too. > > > Are you doing this or me? I'm happy to work on it if you'll provide > feedback when needed. If you're doing it though, let me know and I'll > do something else. Since most of the work is specific, you'd better do it. I can assist with menu/general stuff, which should reduce to some "new reportSomethingFactory" in frmMain::CreateMenus(). Submenus are now handled automatically, so even the events.cpp is totally generic now. I'm going to extend programmers-readme.txt ASAP. Regards, Andreas
Dave Page wrote: > > -----Original Message----- > From: "Andreas Pflug"<pgadmin@pse-consulting.de> > Sent: 30/04/06 18:44:01 > 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. > > >>>(actually, frmReport has a listview->reporttable method that could be >>>used). > > >>I don't quite understand that. > > > void frmReport::AddReportTableFromListView(ctlListView *list); Ok, I see that method, and see it used 4x in pgObject, but dunno what it's about. A quick glance makes it appear strange to me, as all xxReportYY methods in pg* files. Can't comment on what they're doing. Regards, Andreas
Quoting Andreas Pflug <pgadmin@pse-consulting.de>: > An improvement that ignores basics is useless. That's what we've been trying to tell you... Anyway, here's a patch that fixes what Andreas broke. Ed ---------------------------------------------------------------- This message was sent using IMP, the Internet Messaging Program.