[PATCH] minor sublime modifications

[Andreas Pakulat]

On 29.07.08 09:44:35, Manuel Breugelmans wrote:
> 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.

Ah, ok. As I said I don't see the need for the extra methods, just add
a new flag parameter to addToolView or a flags() method to the toolview
factory.

> > >      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). 

I didn't say "boolean flag" I said just "flag" which usually means an
enum type - at least for me :)

> > > +            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.

I just couldn't understand part of the patch without the rest of the
code - specially seeing the links between classes and thus was confused
about all the similar/same named methods on various classes...

Andreas

-- 
Your love life will be happy and harmonious.