[PUSHED] [PATCH] Object Catalog in Dialog Editor

[János Uray]

> 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
>
IMO, not all empty virtual functions need to be pure virtual. Pure virtual
functions are needed when the derived class really need to do something
(e.g. returning a value). But GetState() is not that case. This function is
virtual only to give the derived classes the possibility to modify the
state of some buttons in the toolbar, but it isn't required (in other
words, it's a listener function).
And Layout was still abstract -- in the sense that it can only be
instantiated by a derived class, because its ctor and dtor are protected.

Layout::ConfigurationChanged is not used at all either in Layout itself of
> in any of the classed derived from it
>
Yes, I forgot to remove that. A working ConfigurationChanged() is in
ModulWindowLayout::SyntaxChanged.

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
>
It did something, but then I refactored ObjectCatalog to BasicIDEShell, and
ExecuteGlobal became empty.
But it may do something useful later, e.g. when we'll have buttons to
toggle StackWindow and/or WatchWindow.


>     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.
>
It's name is UpdateDebug() because it updates the StackWindow and
WatchWindow -- both are needed to debug Basic. I'm not very good at
inventing names. This function allowed to remove GetStackWindow() and
GetWatchWindow(), which both violate encapsulation and disallow
BasicIDEShell to handle layouts generally.
It's better not to be pure virtual since only ModulWindowLayout does
something useful -- all other classes (currently there is only one other
class) just do nothing, so an empty body is suitable for default
implementation.

Uray M. János
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20120821/eefeaad9/attachment-0001.html>