Extender api review, round 2

[Rob Scheepmaker]

Thanks for the suggestions :)

On Wednesday 30 July 2008 19:26:26 Kevin Ottens wrote:
> Le Tuesday 29 July 2008, Rob Scheepmaker a écrit :
>
> 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).

Actually it moves the extender to somewhere in the topleft quadrant of the scene (negative coordinates). 
However, I'm actually thinking now that PopupApplet should handle this.

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

Since it's in the Extender class, we might also name it setEmptyMessage(...)

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

Agreed, I've actually changed all extenderItem in the Extender class to item.

>
> >         QList<ExtenderItem*> attachedExtenderItems() const;
>
> attachedItems()?
>
> No detachedItems()? Just wondering if that could be useful/desirable.

Might be useful... you can find it out yourself, but as a convenience function it's not a bad idea.

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

Possibly, but I quite like item(...) , as analog to action(...)

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

Yeah, I left that out since I didn't need it for the default implementation of Extender but new 
implementations might need it, so I'll add it.

>
> >     Q_SIGNALS:
> >         void itemAttached(ExtenderItem *);
> >         void itemDetached(ExtenderItem *);
>
> Why also have the event-y variants of those then? (itemAddedEvent() and
> itemRemovedEvent())

The might not be entirely necessary, but since you'll need to implement those events when subclassing, 
It's currently quite clear what to do when subclassing Extender (there are just 4 virtual functions and 
you'll just have to implement all four of them... just looking for the virtuals is way I usually check 
how to subclass a certain class), while you're still able to watch these events in your applet in case 
you want to, say, change the appearance of detached items for your applet.

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

Hover events are just meant to be able to insert spacer or stuff like that, which is entirely Extender's 
job.

> >     class PLASMA_EXPORT ExtenderItem : public QGraphicsWidget
> >     {
> >         Q_OBJECT
>
> Missing Q_PROPERTIES: id, name, title, icon, detached, collapsed

agreed.

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

Yeah, usually you won't need that. But loadExtenderItems uses that to set the id's to what they were 
instead of creating new ones... So the item gets the correct kconfiggroup assigned to it. But indeed, 
the rest could be removed. Maybe just keep parent and extenderItemId (which default to 0)..
>
> 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?)

agreed... it's a leftover of the earlier api... should have removed it.

>
> >             QString name();
>
> As I said above, I'd provide a setter for this one.

done :)

> >             void setWidget(QGraphicsWidget *widget);
> >             QGraphicsWidget *widget() const;
>
> Why QGraphicsWidget only? Why not it's parent class QGraphicsItem?

Hmm, that's actually a very good point. Currently I actually check the size hints of the widget though. 
to set sane size hints for the extender item. maybe I should try to cast the widget to QGW, use the 
sizehints in that case, and assume a minimumsize of the current size if its a QGI? Supporting QGI would 
be more flexible...

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

Agreed... I'll think I'll go for the first suggestion...

> >             void setExtender(Extender *extender);
> >             Extender *extender() const;
>
> Ah, now I'm confused... How is that different from the mandatory extender
> provided in the ctor?

This way the item can be moved between extenders... I'm not entirely sure if there are cases where that 
is useful though. I guess it's only needed if we're going to support multiple extenders/applet... which 
I think we'll eventually want.

> >             void setExpirationTimeWhenAttached(uint time = 0);
>
> Wow, what a name! :-)

For now I'm going with setAutoExpireDelay()

> >             /**
> >              * @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?

Hmm, the apidox was from an earlier revision... but I thought you didn't read apidox ;)
Anyway, it's the id of the applet source.

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

No, the actual reason is that destroy also removes the item's configuration data. That's not something 
that you want to happen when you quit plasma ;). But maybe I should rename this function to delete?

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

might be a bit clearer. However it would make me think it has something to do with the desktop/panel 
toolbox. Maybe handleToolBoxAction(...)? Hmm, handle sounds like a verb here... I'm not yet sure about 
this one.

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

It would avoid the binary compatibility issue... However, I think the virtual somehow 'feels' more 
natural. I'll think about this...

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

Yeah, i'll probably remove this.

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

And some serious breakages here an there as a product of the api review ;) But yeah, it's progressing 
nicely :)