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

List:       kde-panel-devel
Subject:    Re: Extender api review.
From:       "Aaron J. Seigo" <aseigo () kde ! org>
Date:       2008-07-26 21:45:32
Message-ID: 200807261545.36226.aseigo () kde ! org
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


On Friday 25 July 2008, Rob Scheepmaker wrote:
> On Friday 25 July 2008 20:56:58 aseigo@bddf.ca wrote:
> > * 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?

right; well, in many cases it could just use members in Applet::Private with 
the bulk of the implementation in Extender itself. my point was that Extender 
will likely need access to Applet internals.

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

ExtenderItems should be self-managing as much as possible; what i was 
suggesting is that instead of doing applet->addDetachableWidget one would do 
new ExtenderItem(Extender *, ..). the Extender knows what Applet it is 
associated with and can do the work that way; it's a more natural API style 
(at least to me) and will keep the impact on Applet's API down.

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

should have been QGraphicsItem * ... sorry.

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

yes

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

the default attachedEvent should do this for them i think (and that should be 
documented in the apidox). this way the default behaviour is maintained, but 
it is overrideable if desired.

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

which event are you using this with, and with what object (QGraphicsView? 
QGraphicsItem? some other QWidget class?)

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

does this make sense for all applets? if Twitter were to use Extender to show 
its tweets, and i have two Twitter widgets with different accounts, it dosn't 
make sense at all. similarly for clocks where there are different time zones 
associated with each ... i just don't think this heuristic fits well.

in any case, this code probably belongs internally to Extender or ExtenderItem 
rather than adding to the public API of Corona. i don't see any other users of 
it.

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

ah, i see. there's one containment per Extender then?

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Trolltech


["signature.asc" (application/pgp-signature)]

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