extender api review round 3

[Kevin Ottens]

Le Friday 01 August 2008, Rob Scheepmaker a écrit :
> I'm looking forward to getting some feedback on the current api.

I'll probably dup some of the other comments in this threads. My apologies in 
advance. :-)

> namespace Plasma
> {
>     class ExtenderPrivate;
>     class ExtenderItem;
>     class Applet;
>
>     class PLASMA_EXPORT Extender : public QGraphicsWidget
>     {
>         Q_OBJECT
>         Q_PROPERTY(bool isPopup READ isPopup WRITE setIsPopup)

setIsPopup => setPopup (as already pointed), and I still don't like the 
"setPopup" just like Aaron (hence why I proposed a "floating" property 
instead, but Aaron didn't like that name either).

>         Q_PROPERTY(QString emptyExtenderMessage READ emptyExtenderMessage
>                                                 WRITE
> setEmptyExtenderMessage)
>
>     public:
>         explicit Extender(Applet *applet);

Woot! Much better, isn't it? :-)

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

See above.

>         void setEmptyExtenderMessage(const QString &message);
>         QString emptyExtenderMessage() const;
>
>         QList<ExtenderItem*> items() const;
>         QList<ExtenderItem*> attachedItems() const;
>         QList<ExtenderItem*> detachedItems() const;
>
>         ExtenderItem *item(const QString &name) const;
>
>         virtual void saveState();

Shouldn't this be protected?

>     protected:
>         virtual void itemAddedEvent(ExtenderItem *item, const QPointF
> &pos);
>         virtual void itemRemovedEvent(ExtenderItem *item);
>         virtual void itemHoverMoveEvent(ExtenderItem *item, const QPointF
> &pos);
>         virtual void itemHoverLeaveEvent(ExtenderItem *item);

Where is itemHoverEnterEvent()?

>     Q_SIGNALS:
>         void itemAttached(ExtenderItem *);
>         void itemDetached(ExtenderItem *);
>         void geometryChanged();
>
>     private:
>         ExtenderPrivate* const d;
>
>         friend class ExtenderPrivate;
>         friend class ExtenderItem;
>         friend class ExtenderItemPrivate;
> };
>
> }
>
> namespace Plasma
> {
>     class Applet;
>     class Extender;
>     class ExtenderItemPrivate;
>
>     class PLASMA_EXPORT ExtenderItem : public QGraphicsWidget
>     {
>         Q_OBJECT
>         Q_PROPERTY(QGraphicsWidget * widget READ widget WRITE setWidget)
>         Q_PROPERTY(QString title READ title WRITE setTitle)
>         Q_PROPERTY(QString name READ name WRITE setName)
>         Q_PROPERTY(QIcon icon READ icon WRITE setIcon)
>         Q_PROPERTY(Extender * extender READ extender WRITE setExtender)
>         Q_PROPERTY(bool collapsed READ isCollapsed WRITE setCollapsed)
>         Q_PROPERTY(bool detached READ isDetached)
>         Q_PROPERTY(uint autoExpireDelay WRITE setAutoExpireDelay)
>
>         public:
>             ExtenderItem(Extender *hostExtender, uint extenderItemId = 0);
>             ~ExtenderItem();
>
>             KConfigGroup config();

const missing (IIRC already pointed by Rich).

>             void setWidget(QGraphicsWidget *widget);
>             QGraphicsWidget *widget() const;
>
>             void setTitle(const QString &title);
>             QString title() const;
>
>             void setName(const QString &name);
>             QString name() const;
>
>             void setIcon(const QString &icon);
>             void setIcon(const QIcon &icon);
>             QIcon icon() const;
>
>             void setExtender(Extender *extender, const QPointF &pos =
> QPointF(-1, -1));
>             Extender *extender() const;
>
>             void setAutoExpireDelay(uint time = 0);

Please no default here.

>             bool hasAutoExpireDelay() const;

Why not simply uint autoExpireDelay() const; ? At least that makes it 
symmetric, and you can get more information out of the class.

>             bool isDetached() const;
>             bool isCollapsed() const;
>
>             void addAction(const QString &name, QAction *action);
>             QAction *action(const QString &name) const;
>
>             uint sourceAppletId() const;
>
>         public Q_SLOTS:
>             void destroy();
>             void setCollapsed(bool collapsed);
>             void moveBackToSource();
>
>         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())
>
>             ExtenderItemPrivate * const d;
>
>             friend class Extender;
>     };
> } // namespace Plasma
>
>
> class PLASMA_EXPORT Applet : public QGraphicsWidget
> {
>     public:
>         virtual void initExtenderItem(ExtenderItem *item);

Again, I'd try to fit this one somewhere else. Maybe Extender?

>         Extender *extender() const;

Again, I wouldn't keep this one.

The idea here being that Extender is kind of an add-on decorator to Applet, so 
to make that clear API wise we should really avoid any Applet->Extender 
dependency in the public API.

>         friend class Extender;
>         friend class ExtenderItem;
> };
>
> } // Plasma namespace
>
> class PLASMA_EXPORT Corona : public QGraphicsScene
> {
>     void addOffscreenWidget(QGraphicsWidget *widget);
>     void removeOffscreenWidget(QGraphicsWidget *widget);
>
> };
>
> } // namespace Plasma

Still open question on my side:
What would be the use of the new provided API? Before extenders, we had to 
inherit Applet to make a new applet. With the provided API, I have this 
feeling that to get anything useful you'd have to inherit from Applet and 
Extender. Am I right? Any chance we could spare the inheritance from Extender, 
at least for the most common cases?

In fact I think that's why I had more in mind a signal oriented API for 
extender instead of an event oriented one as now (I have in mind all the 
hover, and attach/detach event methods in Extender).

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/20080803/d60d34e1/attachment.pgp