Extender api review, round 2

[Alex Merry]

On Wednesday 30 July 2008 18:26:26 Kevin Ottens wrote:
> setIsPopup should be setPopup to be consistent with the other Qt APIs...
> But then with such a name I wouldn't expect a bool. It seems to be trying
> to be the short form for something.

> Then I would propose something like "floating": isFloating() +
> setFloating(). Not sure it's the best term, but that looks like a decent
> start.

Another option is actsAsPopup and setActsAsPopup.

> >         void setEmptyExtenderMessage(const QString &message);
> >         QString emptyExtenderMessage() const;
>
> A bit verbose, but that's probably ok. I don't see a way to make it shorter, 
> it's not confusing which is more important than concision.

If you know it's a member of Extender, isn't setEmptyMessage/emptyMessage 
enough?

> >         QList<ExtenderItem*> extenderItems() const;
>
> items()? We're withing the Extender class bounds, so contextually should be
> ok I guess. One could still argue that it could make code like this:
> Extender *e = ...;
>
> // Lots of stuff
>
> e->items(); // Dang! Not enough context, wth are "items"?

Which is why people should use meaningful variable names.

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

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

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

> >             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?

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


Other notes:

I don't like the term "source applet".  I think "host applet" would be better.

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.


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.

Other than that, +1 to Kevin's other points.

Alex



-- 
KDE: http://www.kde.org
Ubuntu/Kubuntu: http://www.ubuntu.org http://www.kubuntu.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/plasma-devel/attachments/20080730/9817d850/attachment.pgp