[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-panel-devel
Subject:    Re: Extender api review.
From:       Rob Scheepmaker <r.scheepmaker () student ! utwente ! nl>
Date:       2008-07-25 23:13:58
Message-ID: 200807260113.58624.r.scheepmaker () student ! utwente ! nl
[Download RAW message or body]

On Friday 25 July 2008 20:56:58 aseigo@bddf.ca wrote:
> yes, it's awesome to see this come to fruition. so.. API reviews...

Thanks for the feedback. I agree with most suggestions. Some remarks:

> the apidox there. what about ExtenderItem?

Yeah, probably better...

> * Applet API polution .. this adds a lot of API to applet for something
> not everyone will use. perhaps in this case what we really want is
> something like a Decorator, perhaps called Extender? this would not be a
> subclass, so would avoid the issues of a subclass and would encapsulate
> this whole behaviour in a separate class that any Applet derivative can be
> used with. i think this will be a lot easier for people to understand.
> Extender would almost certainly need to be a friend of applet, and a bunch
> of the API you have introduced would go into AppletPrivate

Sounds interesting, but it's not entirely clear to me how this would work. Do we get things like: 
extender()->extenderItems() where all non virtual extender related functions are put in a Extender class 
that forwards most of those functions to the appropriate function in Applet::Private?

> * instead of addDetachableWidget ... this would make sense as an
> ExtenderItem constructor, with an added setter if this needs to be
> adjusted at runtime.

So the developer takes care of adding it to a layout itself? Hmm, probably not a bad idea to seperate 
the layouting from the extender handling.

> * createDetachableWidget (createExtenderItem?) should probably do the
> actual creation and call a virtual protected method that only needs to do
> the applet specific stuff (QGraphicsView *initExtenderItem(name, config))

Not a bad idea to split this up, but I don't get the QGraphicsView *...

> * detachableActions -> i think this should probably be something that is
> added to ExtenderItem itself, and probably doesn't belong in the Applet
> API

Like extenderItem->addAction/removeAction/action() ? I think too.

> * setForcePanelCollapse ... i don't like putting this into Applet at all.
> what about applets with extender items that aren't icons? use case: the
> clock puts the calendar, each timezone time and the next 3 items on your
> calendar into extenders. in a panel, the Extender should *always* be popup
> only, and in other form factors, that's really up to the applet itself. i
> suppose that's what this is attempting to do? if so, then a simple
> Extender::isPopup and Extender::setIsPopup is probably all that's needed?

When we go for the extender decorator approach, that would make sense.

> * setSupportDetachables -> this would be implied by having a Extender
> associated with the Applet, and so could be completely internal. in any
> case, adding a layout to an applet sounds dangerous. in fact, it would
> seem to imply that it would be impossible to have self-hosted content in
> an applet (e.g. a clock) *and* extenders. the Extender should be
> completely external to the Applet, unless purposefully embedded.

So the applet itself could place an extenderitem in a layout in attachedEvent()? not a bad idea.. gives 
some more flexibility to applets.

> * Corona::appletFromScreenPos(const QPoint &pos) and
> Corona::containmentFromScreenPos(const QPoint &pos) -> why is this needed?
> don't the drop events (which is what i assume this is used for?) have a
> scene coordinate? doing anything to/from screen position is going to be a
> bit brittle, i think.

The problem is, that event->scenePos works while within one view, but when you move to another view, 
event->scenePos still assumes you are still in that view. I think it is unavoidable.

> * why do you need appletForName(const QString &appletName, uint appletId =
> 0)? shouldn't it *always* be by id? and in that case, it's already pretty
> easy to find, no?

Well, assume you have applet x, detach one or more extenderitems from it, remove it, think: that's 
stupid and add that applet again. It makes sense for the extenderitems to see that applet still as 
source applet (for the 'return to source' functionality), while it's id is different.

> * extenderContainment: i imagine this a Containment subclass internal to
> Corona? why is it needed outside of Corona at all?

Hmm, currently applet uses it to point it's toplevel view on that containment (applet::view needs an 
containment in it's constructor). But that could probably move to pimpl

>
> btw, i'd like to have a goal of having this in trunk/ by the start of
> Akademy. is that realistic to you? personally i think we could have this
> hammered out with the next 7-10 days with time to spare. it doesn't have
> to be perfect in the implementation, just reasonable in the API, and it's
> already working, so ... =)

Well, August 4th I'll be going to hungary for 2 weeks, so I'll do the api refactoring over the next 9 
days. Should be doable... after all it already works. A pitty that akademy is during the sziget 
festival, a great week long festival on an island in the danube.
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]