Extender api review, round 2

[Kevin Ottens]

Le Tuesday 29 July 2008, Rob Scheepmaker a écrit :
> I've had a lot of feedback on the extender api, and now I've incorporated a
> lot of the suggestions into a more up to date api. Again, I'm looking
> forward to getting feedback to fine tune this api all the way.

As usual, I'll just skip the apidox for now. Here we go!

>     class PLASMA_EXPORT Extender : public QGraphicsWidget
>     {
>         Q_OBJECT
>         Q_PROPERTY(QString emptyExtenderMessage READ emptyExtenderMessage 
WRITE setEmptyExtenderMessage)
>         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.

>     public:
>         Extender(Applet *applet);

Should be explicit.

>         ~Extender();
>
>         void setIsPopup(bool popup);
>         bool isPopup() const;

See above.

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

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

I would go with items() anyway, and rely on the API user to not screw up the 
context all by himself. :-p

>         QList<ExtenderItem*> attachedExtenderItems() const;

attachedItems()?

No detachedItems()? Just wondering if that could be useful/desirable.

>         ExtenderItem *extenderItem(const QString &name) const;

Seems to be searching items by name... So, findItem()?

>     protected:
>         virtual void extenderItemAddedEvent(ExtenderItem *item, const 
QPointF &pos);
>         virtual void extenderItemRemovedEvent(ExtenderItem *item);
>         virtual void extenderItemHoverMoveEvent(ExtenderItem *item, const 
QPointF &pos);
>         virtual void extenderItemHoverLeaveEvent(ExtenderItem *item);

itemAddedEvent(), itemRemovedEvent(), itemHoverMoveEvent() and 
itemHoverLeaveEvent()?

Since we already have itemHoverMoveEvent() and itemHoverLeaveEvent(), I would 
somehow expect a itemHoverEnterEvent().

>     Q_SIGNALS:
>         void itemAttached(ExtenderItem *);
>         void itemDetached(ExtenderItem *);

Why also have the event-y variants of those then? (itemAddedEvent() and 
itemRemovedEvent())

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.

>     private:
>         ExtenderPrivate* const d;
>         friend class ExtenderItem;
>     }

Isn't a semicolon missing here?

>     class PLASMA_EXPORT ExtenderItem : public QGraphicsWidget
>     {
>         Q_OBJECT

Missing Q_PROPERTIES: id, name, title, icon, detached, collapsed

>         public:
>             ExtenderItem(Extender *hostExtender, Applet *sourceApplet,
>                          const QString &name = QString(),
>                          QGraphicsWidget *widget = 0, uint extenderItemId = 
0);

Wow, that's one hell of a ctor!

I would fiddle with the parameters a bit:
 * hostExtender -> parent
 * sourceApplet can probably be removed (can't we find this one from the 
extender we give as parent?)
 * name could be removed if you provide a setName() (which I would advise in 
any case)
 * widget can be removed (you've a setWidget below, right?)
 * extenderItemId is it necessary? the fact that it comes last seems to 
indicate it's not really useful I guess. :-)

Then the ctor would become:
explicit ExtenderItem(Extender *parent);
Of course a call to it would likely be followed by several setter calls, but 
that's a common pattern with Qt4 APIs.

>             ~ExtenderItem();
>
>             uint id();
>             KConfigGroup config();

Almost fine with me. One could wonder if id() is really of any use in the 
public API as soon as you provide a pointer to the item settings anyway (which 
use I would have of an id then?)
 
>             QString name();

As I said above, I'd provide a setter for this one.
 
>             void setWidget(QGraphicsWidget *widget);
>             QGraphicsWidget *widget() const;

Why QGraphicsWidget only? Why not it's parent class QGraphicsItem?

>             void setTitle(const QString &title);
>             QString title() const;

OK with me.

>             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

 
>             void setExtender(Extender *extender);
>             Extender *extender() const;

Ah, now I'm confused... How is that different from the mandatory extender 
provided in the ctor?

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

As for the name itself, I don't really have better proposals...

>             bool isDetached() const;

Fine with me.
 
>             bool collapsed() const;

Should be isCollapsed().

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

>             /**
>              * @return the id of the applet this item is contained in.
>              */
>             uint sourceAppletId() const;

I kept the apidox for this one, they don't match. Is that the id of the applet 
source of this item? Or the id of the applet where the item is currently 
attached?
 
>         public Q_SLOTS:
>             void destroy();

Hmmm, how is that different from "delete item"? Is it "only" to get the 
convenience of doing it via a slot? Or it's trying to workaround some 
limitation in QGV?
 
>             void setCollapsed(bool collapsed);

OK.
 
>             void moveBackToSource();

Cute. :-)
 
>         protected:
>             void paint(QPainter *painter, const QStyleOptionGraphicsItem 
*option, QWidget *widget);
> 
>             void resizeEvent(QGraphicsSceneResizeEvent *event);
> 
>             void mousePressEvent(QGraphicsSceneMouseEvent *event);
>             void mouseMoveEvent(QGraphicsSceneMouseEvent *event);
>             void mouseReleaseEvent(QGraphicsSceneMouseEvent *event);
> 
>             void hoverMoveEvent(QGraphicsSceneHoverEvent *event);
>             void hoverLeaveEvent(QGraphicsSceneHoverEvent *event);
> 
>         private:
>             Q_PRIVATE_SLOT(d, void toggleCollapse())
>             Q_PRIVATE_SLOT(d, void updateToolBox())

Ooooh! Make my previous proposal about actions:
void addToolBoxAction(const QString &name, QAction *action);
QAction *toolBoxAction(const QString &name) const;

>             ExtenderItemPrivate * const d;
> 
>             friend class Extender;
>     };
> 
> 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?
 
>         Extender *extender() const;

I would remove this one. If someone needs to access its extender often he'll 
keep a pointer around.
 
>         friend class Extender;
>         friend class ExtenderItem;
> }

In any case it's really getting along well now. We're at the stage where we're 
nailing down the remaining smaller issues.

Regards.
-- 
Kévin 'ervin' Ottens, http://ervin.ipsquad.net
"Ni le maître sans disciple, Ni le disciple sans maître,
Ne font reculer l'ignorance."
-------------- 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/d53c0121/attachment-0001.pgp