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
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
