Extender api review, round 2
huntedhacker at tiscali.co.uk
Wed Jul 30 21:32:24 CEST 2008
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
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
> > 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;
> * 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
> 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:
> 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
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.
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.
Ubuntu/Kubuntu: http://www.ubuntu.org http://www.kubuntu.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 197 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/plasma-devel/attachments/20080730/9817d850/attachment.pgp
More information about the Plasma-devel