Review Request: New Applet handle system

Aaron Seigo aseigo at kde.org
Fri Sep 3 20:06:18 CEST 2010 


> On 2010-09-03 15:59:17, Aaron Seigo wrote:
> > the patch does not apply cleanly to trunk; it needs to be updated / regenerated.
> > 
> > instead of putting all of these classes into private/, i think it may make more sense to start a new dir in kdeliba/plasma/ called e.g. handles/, much as we did for animations.
> > 
> > i've also included some comments on ControlElement as a starting point.
> 
> Giulio Camuffo wrote:
>     the patch was done against the last revision. i don't know what to do about it.

your editor removed spaces at the ends of three lines (one in applet.cpp, one in containment.cpp and one in extender.cpp) but the patch does not show this change in it. not sure exactly what bit is messing up here (i'm guessing the svn diff), but i'll commit with those spaces gone and you should be able to re-generate the diff successfully at that point.

try r1171395 of kdelibs/plasma.


> On 2010-09-03 15:59:17, Aaron Seigo wrote:
> > trunk/KDE/kdelibs/plasma/private/controlelement_p.h, line 38
> > <http://svn.reviewboard.kde.org/r/5155/diff/2/?file=34958#file34958line38>
> >
> >     a pointer to the handle shouldn't be necessary.
> 
> Giulio Camuffo wrote:
>     but the handle provides a pointer to the containment and some things like position(). The ResizeControl must ask the handle to know which corner to keep static.

the handle should not be required for such implementation specifics:

* the containment comes from whatever creates the element. e.g. the Applet. 

* details such as which corner to "anchor" on should probably be sent to the Control by the Handle during mouse events

i think one of the things that makes this difficult right now is that the handle is simply forwarding on mouse events. it would be more useful to know, e.g., if the item is simply triggerable (meaning all it needs to know is that the mouse was pressed and then subsequently released in its bounding rect, in the case of the current handles) or if the item needs to do mouse tracking. in both cases, a triggered call would be made (in the mouse tracking case, on mouse press; in the no-tracking case, on successfully mouse release) and then only mouse movement would need to be forwarded in the mouse tracking case (e.g. resize control). in the mouse tracking case, additional information could be sent, such as the anchor corner, using a subclass of QGraphicsMouseEvent. information about the cause of the triggering (e.g. which mouse button(s)) could be passed to the trigger(..) method.

in the end, the ControlElement and the Handle should be very loosely coupled: it should be possible to change the Handle to a completely different class without touching ControlElement.


- Aaron


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5155/#review7383
-----------------------------------------------------------


On 2010-09-01 16:22:54, Giulio Camuffo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5155/
> -----------------------------------------------------------
> 
> (Updated 2010-09-01 16:22:54)
> 
> 
> Review request for Plasma, Aaron Seigo and Marco Martin.
> 
> 
> Summary
> -------
> 
> This is a rewamp of the Applet handle system. Through its modular architecture it easily allows modifications and reuse of code.
> 
> It features a base Handle class, AbstractHandle, and a base class for the control elements, ControlElement. I developed an handle based on the actual AppletHandle, DesktopHandle, and the control elements for the usual operations.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/plasma/CMakeLists.txt 1170608 
>   trunk/KDE/kdelibs/plasma/applet.h 1170608 
>   trunk/KDE/kdelibs/plasma/applet.cpp 1170608 
>   trunk/KDE/kdelibs/plasma/containment.h 1170608 
>   trunk/KDE/kdelibs/plasma/containment.cpp 1170608 
>   trunk/KDE/kdelibs/plasma/extenders/extender.cpp 1170608 
>   trunk/KDE/kdelibs/plasma/extenders/extenderitem.cpp 1170608 
>   trunk/KDE/kdelibs/plasma/private/abstracthandle.h PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/abstracthandle.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/applet_p.h 1170608 
>   trunk/KDE/kdelibs/plasma/private/applethandle.cpp 1170608 
>   trunk/KDE/kdelibs/plasma/private/applethandle_p.h 1170608 
>   trunk/KDE/kdelibs/plasma/private/configurecontrol.h PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/configurecontrol.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/containment_p.h 1170608 
>   trunk/KDE/kdelibs/plasma/private/controlelement.h PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/controlelement.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/controlelement_p.h PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/desktophandle.h PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/desktophandle.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/maximizecontrol.h PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/maximizecontrol.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/movecontrol.h PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/movecontrol.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/removecontrol.h PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/removecontrol.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/resizecontrol.h PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/resizecontrol.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/rotatecontrol.h PRE-CREATION 
>   trunk/KDE/kdelibs/plasma/private/rotatecontrol.cpp PRE-CREATION 
> 
> Diff: http://svn.reviewboard.kde.org/r/5155/diff
> 
> 
> Testing
> -------
> 
> It isn't finished. It's missing the touch events management (which, however, it's hard for me to do, 'cause i don't have any touch screen device) and a better drag and drop system between containments. I'd like, however, to know what you think about what i've done, especially about the architecture.
> 
> What's here works, though.
> 
> 
> Thanks,
> 
> Giulio
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20100903/a29e4683/attachment.htm

More information about the Plasma-devel mailing list