Extender api review, round 2

[Aaron J. Seigo]

On Wednesday 30 July 2008, Kevin Ottens wrote:
> Le Tuesday 29 July 2008, Rob Scheepmaker a écrit :

> >         Q_PROPERTY(bool isPopup READ isPopup WRITE setIsPopup)
>
> 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.
>
> So this property probably needs a rename altogether... I'll be cheating for
> a minute and look at the apidox of the relevant methods: "takes care of
> placing itself", "out of ordinary view" (odd term btw "ordinary view"). I
> guess it's used to change the state of the extender if it's inside the
> applet, or above the view in its own toplevel widget (or something like
> this... somehow referring to the "PopupApplet" class).
>
> Then I would propose something like "floating": isFloating() +
> setFloating(). Not sure it's the best term, but that looks like a decent
> start.

this is almost always used with classes or methods using the term "popup". 
"floating" isn't used anywhere. with isPopup it's at least a decent hint that 
it has something to do with popups (menus, PopupApplet, etc). i don't think 
"floating" has any such useful connections.


> >         ExtenderItem *extenderItem(const QString &name) const;
>
> Seems to be searching items by name... So, findItem()?

agreed.

> Aren't those signals enough? In fact it also makes me wonder if the hover
> events are useful, and if they are if they would deserve being signals only
> too.

good point, i wonder what the use of the hovert events are now as well, now 
that you mention it.

> >             void addAction(const QString &name, QAction *action);
> >             QAction *action(const QString &name) const;
>
> Hmmm, I was wondering what it was for, so I looked at the apidox (yeah,
> cheater!). It asks for a rename I guess. I would propose something like:
> void addHandleAction(const QString &name, QAction *action);
> QAction *handleAction(const QString &name) const;

i don't agree; this exposes an implementation detail (that the action is used 
in the handle) and addAction/action are consistent with QWidget API.

> > class PLASMA_EXPORT Applet : public QGraphicsWidget
> > {
> >     public:
> >         virtual void initExtenderItem(ExtenderItem *item);
>
> I can't help but wonder if that could be in another class. Could it be made
> a signal in Extender itself?

good idea if it works; does the consumer always have access to the Extender 
object in this case? that would be my only concern. i suppose it would have 
to, since it's the creation of an Extender object in the applet that enabed 
Extender support in the applet in the first place.

a signal is also a bit more flexible, as it makes it easier to connect this 
operation to other classes.

> >         Extender *extender() const;
>
> I would remove this one. If someone needs to access its extender often
> he'll keep a pointer around.

and what about items outside of the Applet that would like access to the 
Extenders? i wonder if Containment, Corona or any other classes would benefit 
from that?

on the down side:
	it's one more pointer in each applet.
	it enforces a one extender per applet mechanism

on the up side:
	using an extender is simply a matter of:
		new Extender(this)
	it enforces a one extender per applet mechanism ;)

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Trolltech

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