Extender api review, round 2

[Rob Scheepmaker]

On Wednesday 30 July 2008 21:32:24 Alex Merry wrote:
> Which is why people should use meaningful variable names.

+1

> >  * sourceApplet can probably be removed (can't we find this one from the
> > extender we give as parent?)
>
> In this case (and, I think, anyway) Extender needs an
> Applet* applet() const;
> method.

well, extender item and extender are friends so it can obtain the source applet without this function. 
It might still be useful to have it, but I can't really think of any usecases.

> >  * extenderItemId is it necessary? the fact that it comes last seems to
> > indicate it's not really useful I guess. :-)
>
> And if it is, you can always have setId().
>
> Although, looking at the apidox, the id argument seems to be for internal
> use when recreating extenderitems.  It's used for the config.
>
> Maybe a better way would be to have a factory method in the Extender class?
> And make this constructor protected.

I don't really like the idea of needing to subclass Extender when you want to use extenders.

> > >             void setIcon(const QString &icon);
> > >             QIcon icon() const;
> >
> > Hmmm, it's not symmetric. I would propose:
> > setIconName(const QString &name) + QString iconName() const
> > or
> > setIcon(const KIcon &icon) + KIcon icon() const
>
> A pattern I've seen in other classes is
>     void setIcon(const KIcon &icon);
>     void setIcon(const QString &name);
>     void KIcon icon() const;
> (or have QIcon in place of KIcon, which really only exists for its
> constructor), where setIcon(const QString &name) is just a convenience
> method that calls setIcon(KIcon(name)).

Good idea.
>
> > >             void setExpirationTimeWhenAttached(uint time = 0);
> >
> > Wow, what a name! :-)
> > In any case don't provide the default, otherwise when you read a call
> > like: item->setExpirationTimeWhenAttached()
> > You'll really wonder what it's about. :-)
>
> You could either go for setExpirationTime(), and trust to people not
> assuming it will make it go away when it's detached (or reading the docs),
> or setAttachedExpirationTime().
>
> How about Timeout instead of ExpirationTime?

currently I have setAutoExpireDelay which I quite like...
>
> > >         Extender *extender() const;
> >
> > I would remove this one. If someone needs to access its extender often
> > he'll keep a pointer around.
>
> I think you have to think about the use pattern for this.  As far as I can
> see, from an applet writer's point of view, you add an extender to an
> applet, rather than the other way around.  So I would expect (a) you can
> add more than one extender to an applet and (b) an applet() or hostApplet()
> method in Extender, but not an extender() method in Applet.  And I would
> expect to have to keep hold of my own extenders.
>
> If I create an extender with "ext = new Extender(this);", I wouldn't expect
> extender() to necessarily return anything unless I called a
> setExtender(ext) method in the mean time.
>
> Question: who owns Extenders?  Applet, or the creator of the Extender?  If
> the Applet owns it, then I can't create it on the heap (ie: as a
> non-pointer member of my subclass), and a factory method might be safer. 
> If the creator owns it, they have to keep a reference to it to clean it up
> anyway.

Applet is the creator of the extender, so I don't really get your question.
>
> Other notes:
>
> I don't like the term "source applet".  I think "host applet" would be
> better.

well... host applet is something different from source applet. The source applet is the applet that 
created the extender item, and is needed so extender can call the initExtenderItem for an extender item 
after a plasma restart. Host applet I consider to be the applet that... well... hosts the item. It's the 
applet that contains the extender that is parent the item. So basically: isDetached = (sourceApplet != 
hostApplet)

> Question: what happens to detached extenders when the original applet is
> destroyed?  I assume they go away as well, since they seem to be tied to an
> Extender which is tied to an Applet.  In which case, "host applet" gives a
> much better idea of this link.

No, that is not the idea. After an extender item is moved to another extender, that extender then owns 
the item. You can safely remove the source applet afterwards. When the applet is well coded (not 
depending on the applet), that is. Maybe add a property to extenderitem to indicate if it can life 
without it's source applet?

> Also, I don't like the asymmetry of creating extenders with associated
> widgets directly the first time, but also having the initExtenderItem()
> method. Especially as initExtenderItem() doesn't sound like it's only
> called on restore.  But I'm not sure what a good solution would be, apart
> from renaming it restoreExtenderItem(), which only slightly improves it
> IMO.

Of course you can create an ExtenderItem, set it's configuration to whatever it represents and then call 
initExtenderItem. Makes sense imo.