widget snap

[Aaron J. Seigo]

On January 1, 2010, Roman Shtylman wrote:
> I was sitting around trying to organize some of my plasmoids... and
> realized that you can't easily make a stack of plasmoids all exactly
> aligned ... at least not that I could figure out. I thought it would

snapping is a nice idea. putting it in the handle is pretty much the "only" 
way to do it right now, but it's not a great solution: it means that click-
dragging on applets that support that, which is many/most of them, will work 
differently than when dragging on the handle. i have already observed people 
not figuring out how to drag widgets from the desktop to the panel because of 
this "handles are magical" behaviour.

it's also going to break / do unhappy things for any containment that doesn't 
(for whatever reason) want such behavior.

this really does belong to the Containment, but it needs support in the 
handle. adding to this mess is that the handle is in libplasma at all: it 
isn't used by all Containments (think of panels, for instance, or amarok).

so, what are the handles for then? well, they do two things:

* provide quick and discoverable access to features like resize, remove and 
configuration

* guarantee that we have a click-and-drag surface for applets that may not 
provide one

the handle can't simply be moved into, e.g., DesktopContainment as really all 
plasma-desktop "desktop" containments should have them. to me, it sounds like 
we need to think about how handles are done and do them better. they need to 
be:

* sharable between Containments (for consistency and code sharing)

* allowed to be specific to the application or the Containment

* be able to coordinate with the Containment on layout issues

* have logic for things like "moving to another Containment" moved outside the 
handle to API that is available to the handle but which is actually "native" 
to the Applet class so that we get rid of the "when you use the handle, it 
behaves this way; when you click on the applet it behaves that way" behaviour

as for the patch as posted, here are my thoughts/comments/questions:

* instead of 'srect' how about 'snapRect' or 'snapToBoundingRect' or something 
that actually says what it is? cryptic names for variables make code harder to 
read and therefore maintain 

* why is the background FrameSvg's _mask_ being used to generate the rect? 
that's going to be a lot more pixmap allocation and disk cache hitting since i 
don't think the masks are created for Applet backgrounds otherwise. looks 
incorrect, and could be significant from a performance perspective

* we use the KDE Libs coding style. please conform to that:

	http://techbase.kde.org/Policies/Kdelibs_Coding_Style

in particular things like:

+                    if (qAbs(y1) < snapDist)
+                        moveBy.setY(y1);
+                    else if (qAbs(y2) < snapDist)
+                        moveBy.setY(y2);

should be:

+                    if (qAbs(y1) < snapDist) {
+                        moveBy.setY(y1);
+                    } else if (qAbs(y2) < snapDist) {
+                        moveBy.setY(y2);

* there is a lot of pre-calculation of values which are only used once later 
on, e.g. "const qreal x1 = drect.right() - srect.right();" which is used 14 
lines later. this means that when i want to see what "x1" is to understand 
what is going on, i have to backtrack 14 lines. why not just:

+                    if (qAbs(x1) < snapDist) {
+                        moveBy.setX(otherSnapRect.right() - 
snapRect.right());

seems much more readable?

* finally, i wouldn't bother with the moveBy QPointF; it doesn't match with 
the QGraphicsItem API very well and is just another object to allocate. i'd 
just define two qreals (dx and dy?) and do sth like:

+                    if (dx != 0 || dy != 0) {
+                        m_applet->moveBy(dx, dy);
+                        break;
+                    }

that's just a minor point really, though :)


the big issue is "doing this correctly". as such, my feeling is that we should 
hold on to this patch and do it right for 4.5. there's no rush it in imho, 
even though it is very nice to have.

Roman, would you be interested in working on this further and using it as an 
excuse to improve the whole "how handles work" thing? it shouldn't be a very 
big project, but one that would definitely have some nice benefits for all 
users of Plasma.

-- 
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 Qt Development Frameworks