Re: [PUSHED] [PATCH] Object Catalog in Dialog Editor

Hi János
On 17/08/12 09:49, Noel Power wrote:
> Anyway I think if this runs ok ( I will try it later ) I  would be 
> inclined to just check it in, the benefits from this change far 
> outweigh any new minor bugs that might be introduced. I will attempt 
> to review or at least scan the resulting new code rather than trying 
> to read the diff

ok, I ran it and it works nicely !! ( it's even possible to still dock 
the properties browser, although it stacks it a bit weirdly :-) ) I 
committed the patch, some minor nitpicks

the Layout class has some virtual methods with no implementation, this 
sounds like these need to be pure virtual making Layout an abstract 
class ( which makes sense ) When I looked a little close at some of 
those methods I notice

Layout::ConfigurationChanged is not used at all either in Layout itself 
of in any of the classed derived from it

similarly ExcuteGlobal & GetState  I'd prefer if those were pure 
virtual, actually ExecuteGlobal isn't used in either class :-) so, it's 
not clear whether you actually intend to use them for something or not 
but it's confusing when you have like
basides1.cxx:701
    if (pLayout)
        pLayout->ExecuteGlobal(rReq);
which does nothing

    virtual void UpdateDebug (bool bBasicStopped = false);

virtuals with defaults are not really recommended, I removed the 
default, actually for this method, would be greate if we could remove it 
from this base class altogether, it doesn't really fit since Dialog has 
no concept of the debugger.  The Layout class is a container and that 
method depends on assumptions about the contents of the container. 
Anyway I didn't see an easy way out of removing it at this point, 
probably something to keep in mind to try and remove from here in the 
future, it wont do any harm to leave it there.

I committed e03553ab7515b60851dfca2c16a2fcae7a49f441 to address some of 
the above hope that is ok

some other things to consider,
* probably best to have your Layout class deal with DockingWindows 
rather than BasicDockingWindows, afaik ( could be wrong ) the properties 
browser dialog needs to be a SfxDockingWindow ( to get framework updates 
etc. )
* would be really nice to be able to get the watch window or stack 
window back after you close them ( currently they seem to be only 
created when the ide is opened), at least I couldn't find a way to get 
them back after closing them
* split behaviour, I think we could improve that ( yes I know it is 
working more or less the way it was, it's not your fault ) now 
personally I would like to see
   * the split behaviour more dynamic, for example if I make the stack 
window floating and now there is just the watch window in the area I 
would expect the watch window to fill the available space and the 
splitter to disappear.
   * and the reverse, if say the watch window occupies the full area 
and I drag the the stack window into same area I would like to see the 
split position determined by the size of the new window docked
   * preserving state between dialog and module, I think this will be 
important because the area used in the dialog editor is quite different 
and I would expect someone would dock/size/position the windows in a 
different arrangement between the Module and Dialog views

all in all I really think you have done some great work here and I am 
sure more great stuff to come from you,

thanks

Noel