Start of extender API draft

[Kevin Ottens]

Hello,

First of all, sorry for taking so much time to finally get to this mail...

Le Saturday 24 May 2008, Rob Scheepmaker a écrit :
> As you may know, I'll be implementing 'extenders' for GSoC this year.
> I've started drafting up some api for extenders and was looking for some
> feedback. Note that this api is still incomplete and unpolished, but I
> want some feedback on the general direction I'm taking here.
> Of course there are lots of ways something like this could be
> implemented. I've tried to explain why I made most design decision, but
> if you have any questions, please ask and I'll try to explain better. :)
> I'm also open to entirely different approaches. This is nothing more
> then an early draft and a lot can change.
> If you want to know what extenders actually are, I'd suggest you read my
> blog post about it at:
>
> http://pindablog.wordpress.com/2008/04/23/gsoc-extenders-project/

Before I really begin, just a few preliminary comments. I'm not really 
convinced (yet?) that the extenders require two new classes. For me they 
sound like allowing "compound applets" (applets embedding other applets). 
There's also a two reasons for going this "compound" way IMO:

1) If the "detachable widgets" are a new class, I've this feeling that in the 
end they'll duplicate quite a lot of Applet features (need for layout 
management, config, formfactor, etc.).

2) It's probably easier to design more complex applets making use of extenders 
with a compound approach. That particularly strucked me looking at your 
examples. Most of the applets are views on some piece of data, and in the 
example you gave they're always of the same type (a set of jobs, a set of 
timezones, etc.). Let's take the "jobs" example. The easiest path is to 
design an applet able to display one job, make sure it's ok for the layout, 
form factor and such. Once I'm happy with that, having a "kuiserver applet" 
is basically creating a compound applet able to create "jobs" sub-applets. 
That also sounds like a nice upgrade path for people designing their complex 
applets: make simple pieces and glue them together.

I hope that helps, I just wanted to point those out first, because that 
probably bias quite a bit my reviewing. So please keep those two points in 
mind.

Ok, now let's go with the API review itself. As usual I'm removing the 
comments since I prefer APIs to be easily discoverable without having to read 
tons of doxygen comments (for more information see my first Plasma::Service 
review, IIRC I explained it all).

> namespace Plasma
> {
>     class Extender : public Applet

Of course in my biased vision this probably should be all in Applet 
directly. ;-)

>     {
>         public:
>             static QGraphicsWidget *createDetachableWidget(Extender
> *hostApplet, const KConfigGroup &config);

Why is this one public? And even static? Is that useful outside of the 
extender implementation?
I doubt it, it probably should be non static (a clue for that is that you pass 
the hostApplet as parameter, so why not calling on the hostApplet), and for 
sure it should be protected since it's only the business of the applet 
subclass to decide when to create detachable widgets.

>             void addWidget(QGraphicsWidget *widget, const KConfigGroup
> &config, const QPointF &position = QPointF());

I *guess* it's to add to an extender a widget which was previously created 
using the static factory method above... I also suspect it's to pass around 
widgets between extenders? If I'm right, then it probably makes sense to make 
it public.

>             void removeWidget(QGraphicsWidget *widget);

Same reason.

>             QList<DetachableWidget *> detachableWidgets();

Sounds fine. If the API is reworked to match my preliminary comments it'll 
probably require a rename though.

>             virtual bool acceptDetachable(DetachableWidget *widget);

I think this one should be protected. The check to accept a new detachable is 
most likely internal to the extender lifecycle.

>         protected:
>             QList<DetachableWidget *> allDetachableWidgets();

This one is confusing... Why the public "detachableWidgets()" and this one? 
Which kind of filtering the other method is doing if this one give 
them "all"?

>             void setAssociatedExtender(Extender *applet);

I've no idea what this one is for... I have an extender, why would I want to 
associate it with another one?

>             QWidget *createTopLevelWindow();

Probably not something we want to expose to applet writers. I would expect 
such a thing to be completely internal. So it would go as a method in the 
d-pointer.

>     };
> } // namespace Plasma
>
> namespace Plasma
> {
>     class DetachableWidget : public QGraphicsWidget

Of course, with my bias I'd say this class shouldn't exist... It's simply an 
applet embedded in a compound applet.

>     {
>         public:
>             DetachableWidget(QGraphicsWidget *widget, const QString
> &sourceAppletName, int sourceAppletId, KConfigGroup &config)
>
>             KConfigGroup config();

Fine. Even if it somehow looks like the beginning of duplicating Applet 
facilities (my bias at play again).

>             QString sourceAppletName();
>             int sourceAppletId();
>             ExtenderApplet *sourceApplet();

I would only expect the sourceApplet() method here. All the other ones can be 
queried through the sourceApplet itself if I'm not mistaken.

>             void setDetachable(bool);
>             bool detachable();

Hmmm... Interesting, so it's a "DetachableWidget" which I can make "non 
detachable"? Is it still a detachable widget in this case? :-)
I'd be tempted to say that it shouldn't be there.

>             bool isDetached();

Sounds fair.

>     }
> } // namespace Plasma

So that's it for my first review, with a couple of bias on my own view on what 
extenders are from a technical standpoint. Then obviously it shouldn't be 
taken litteraly if that totally violate your own vision or if I missed the 
big picture. I'd like to see this "extenders as applet compounds" view 
examinated though, might be a path to also give us the applet grouping almost 
for free.

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: 194 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/panel-devel/attachments/20080530/3f794539/attachment.pgp