Start of extender API draft

[Rob Scheepmaker]

On Friday 30 May 2008 18:38:30 Kevin Ottens wrote:
> Hello,
>
> First of all, sorry for taking so much time to finally get to this mail...

No problem, I really appreciate that you take the time to respond to this. :)
>
> 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:

Ok, compound applets... so you mean that you basically have an applet that 
contains other applets. Somehow that sounds vaguely familiar ;)
Well, I've actually thought about this possibility and have a couple of 
reasons why I think this isn't the way to go. But feel free to debunk those 
reasons and convince me to do otherwise :)
Let's first consider the 2 points you make:
>
> 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.).

Ok, detachable widget will do some own config, but I think the duplication 
will end there. You have to understand that detachable widgets are never meant 
to exist 'stand alone'. They always are part of a 'host applet'. So when you 
detach such a widget from an extender, a new extender applet is created right 
where the widget is dropped, and the detachable widget is added to it. When 
dropped on an existing applet, it is just added to that applet. And in case 
the widget needs some applet api (dataEngine for example), the public api of 
the host applet can be accessed without any trouble. But in most cases 
dataEngine is the only applet specific api function that needs to be accessed, 
so why not avoid the overhead of using full applets for detachables?
>
> 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.). 

(which doesn't mean we don't want to support creating different types of 
course. We don't want people's creativity limited by technical limitations if 
we can avoid it)

> 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'm personally of the opinion that using QGraphicsWidgets is actually a 
simpler approach and provides an easier migration path in most cases. Let me 
explain why:
1) We already have quite some nice qgw's in libplasma, and most likely, that 
collection will become larger over time. Stuff like signalplotters, icons, 
webcontent, meters, pushbuttons, labels. For a lot of detachables, creating 
one of those widgets, or maybe a couple which are combined using layouts, is 
all there is to it. A lot of applets already use these widgets and migration 
to using extenders for these applets will be very painless, as opposed to 
having to create a new applet just for using detachables. Take the twitter 
applet for example. If it wants to make individual tweets detachable, all it 
needs to do is just add the tweets through addDetachableWidget instead of 
directly adding them to the layout, and it's halfway there.
2) Other applets which already display 'lists' of items, and which are obvious 
candidates for extenderification, already use custom implemented QGW's, look 
for example at the notify applet. Again, migration will be easier here.
3) Most detachables won't need the overhead of being full applet and can use 
the hostApplet for certain applet api that they do need.
4) When we support QGW's, we'll get support for full applets almost for free. 
Because I do agree that there are most likely some cases where this actually 
leads to an easier migration path. (clocks being one example that comes to 
mind: we don't have clock widgets, we do have clock applets)
5) Probably a moot point, but I'm philosophically against using applets for 
thing that actually don't have any meaning on it's own, which would be the 
case for quite some detachables. ;)

I hope this makes some design decisions clear. Again, please feel free to 
convince me that I'm entirely wrong here, these kinds of discussion can only 
benefit all.

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

I will ;)

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

That's actually an excellent approach. :) You do point out some weaknesses of 
this api this way.

>
> > namespace Plasma
> > {
> >     class Extender : public Applet
>
> Of course in my biased vision this probably should be all in Applet
> directly. ;-)

I actually already decided to do that. :)

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

Your right, as dimsuz also pointed out, static won't work very well. The thing 
I'm trying to avoid though, is the need to have an instance of the source 
applet to create detachables. Yes, when you detach stuff from an applet, 
remove that source applet, and restart plasma, the detachable should still be 
there. I'm not really sure what's the best way to implement this. Separate 
Factory class maybe?
>
> >             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... 

yes, although when creating widgets, you don't necessarily need to use that 
factory method. That factory method is for re-instantiating the detachables 
after a plasma restart. You're of course free to use it in your applet as 
well.

> I also suspect it's to pass around
> widgets between extenders? If I'm right, then it probably makes sense to
> make it public.

Yes, spot on. :)

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

You're completely right here, this should be protected.

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

Yeah, the idea was that the first function just returns the widgets from the 
hostApplet you're calling the function on, and the second one all widgets that 
have 'this' as the sourceApplet. But I agree, this can be confusing. And all 
an applet subclass probably ever needs is this second function. So 
detachableWidgets could be renamed and private, allDetachableWidgets() could 
probably be called detachableWidgets. Sometimes I'm so caught up in an idea 
that I completely miss how confusing it could be. :p

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

One very common usecase (as explained in the doxygen ;) ) of extenders is: you 
have an applet. When on the desktop it shows a list of detachables. When in 
the panel it doesn't have enough room for that and it hides all its 
detachables in an other extender which appears when you click the applet (of 
hover or whatever). With this method you can set this extender so detachables 
move around automagically. Still people should be able to disable this 
behavior: what if an applet uses multiple extenders for example.

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

Again you're right.

>
> >     };
> > } // 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.

Problem here being that, as I explained earlier this mail we don't always have 
an instance of the source applet since detachables should be able to live on 
independently. Still we need to know what that applet was to be able to 
recreate the detachables it spawned.

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

ehm... Actually I don't really know what I was thinking here :s

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

I hope I've made my vision a bit more clear now. And I want to thank you again 
for taking the time to write such an in depth response. :)

Cheers,
Rob Scheepmaker