[PUSHED] [PATCH] Object Catalog in Dialog Editor
Noel Power
nopower at suse.com
Fri Aug 17 05:25:22 PDT 2012
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
More information about the LibreOffice
mailing list