[PATCH] minor sublime modifications

[Manuel Breugelmans]

On Monday 28 July 2008 22:14:30 Andreas Pakulat wrote:
> On 28.07.08 12:46:20, Manuel Breugelmans wrote:
>
> Here are the comments on the actual patch:
> > Index: interfaces/iuicontroller.h
> > ===================================================================
> > --- interfaces/iuicontroller.h	(revision 838440)
> > +++ interfaces/iuicontroller.h	(working copy)
> > @@ -69,12 +69,20 @@
> >
> >      virtual void switchToArea(const QString &areaName, SwitchMode
> > switchMode) = 0;
> >
> > +    /** Register a toolview and try to add it to the UI.
> > +        A user will be able to create new tool views like this one. */
> >      virtual void addToolView(const QString &name, IToolViewFactory
> > *factory) = 0; +    /** Spawn a toolview, return true if successful.
> > +        Use it for one-off views, which should not be added by a user
> > directly. */ +    virtual bool showToolView(const QString &name,
> > IToolViewFactory* factory) = 0;
>
> ?? I have no idea what the difference between showToolView and
> addToolView is from this comment. What does "one off" mean?

'one-off' stands for 'once'. I agree that these methods are not ideally named.

> >      virtual void removeToolView(IToolViewFactory *factory) = 0;
> >
> >      /** @return active mainwindow or 0 if no such mainwindow is
> > active.*/ virtual KParts::MainWindow *activeMainWindow() = 0;
> >
> > +    /** Do not store view configuration for toolviews with id @param
> > toolViewId */ +    virtual void excludeFromSave(const QString&
> > toolViewId) = 0; +
>
> This should be a flag for addToolView. Either that or we should have a
> virtual function on a base class of the widget thats returned from the
> factory that actually does the saving. (which we will have at some point
> anyway). Then a subclass of this base-widget can decide to override it
> and instead of calling the super-implementation it only does what it
> wants to do.

As you want it ... adding boolean parameters is bad taste though (also see  
http://doc.trolltech.com/qq/qq13-apis.html). 

> > +            if
> > (d->saveBlackList.contains(view->document()->documentType())) continue;
>
> That should be two lines for better readability.
>
> About the rest: the new virtuals and that stuff I'm not sure thats ok
> all through. I'd have to apply the patch and re-check everything, not
> time tonight for that unfortunately and also not tomorrow.

UiToolViewFactory wraps an IToolVIewFactory, instead of implementing it. I 
don't see a problem (assuming you refer to viewCreated()). The only small 
change in semantics is the time at which it gets invoked, a wee bit earlier in 
the chain now.


Manuel