widget snap

[Aaron J. Seigo]

On January 2, 2010, Roman Shtylman wrote:
> I will make those style changes (was not aware of the coding style

great, thanks :)

> The reason for the x1...y4 is to avoid doing the calculation twice (in
> your example to fix it you still had the x1 there and that calculation
> would have to be done twice). I make it const so that if anything the
> compiler sees it won't change and might optimize something away. I

those really aren't expensive operations; i mean, if we really wanted to limit 
the number of operations:

qreal delta = otherSnapRect.right() - snapRect.right();
if (qAbs(delta) < snapDist) {
     deltaX  = delta;
} else if (qAbs(delta = otherSnapRect.left() - snapRect.left()) < snapDist) {
     deltaX = delta;
} else if (...

but it's really not worth doing so given how unreadable the code is versus how 
little would be gained (if anything at all) performance wise.

anyways.. at the very least perhaps move the y# vars down to just above the y# 
if/else code so it's closer to the relevant code and maybe we can also give 
them better names than x# and y#?  maybe rrDelta, rlDelta, llDelta, lrDelta?

anyways, mostly nitpicking here :)

> The use of the 'mask' was the way I was able to get the plasmoids to
> snap to the visible border. Just using the bounding rectangles
> includes the frame which includes the frame shadow. I might have
> missed it but there was no method to get the side for the part of the
> frame without the shadow. I did look at the svg image and saw an item
> called "hint-stretch-borders" that looked to be around the right size,

ah! calling contentsRect() instead of calling boundingRect() might do the 
trick. the contents don't include the borders (if any) of the background, 
which should thereby align the applets nicely ... at least according to their 
content.

now, if there is an applet with a different background image (e.g. none, 
something drawn custom or the translucent versus "standard" background) which 
has set the contentsRect to something different then this may align with the 
contents but not the outer edges (as defined by what is visually identifiable 
as the "edge", so minus the shadowing, etc) ...

that may or may not work nicely visually; would have to try it. i'm guessing 
it would look alright, however, particularly as it would align the actual 
content.

if not, we should look at how to get this information more inexpensively.... 
FrameSvg::mask() returns the region of the alphaMask of the "overlay" (or just 
the frame if there is no overlay) in the svg. seems like a rather round about 
way of doing this.

i wonder if we couldn't just provide this as a shape directly within the svg, 
at least optionally? this would allow an svg to be crafted such that all that 
pixmap-ery wouldn't be necessary. this may require being able to return an svg 
element, but it looks like it would also take some changes to QSvgRenderer. 
hum, hum, hum ..

ok, let's see if contentsRect() does the trick; if not, then let's revisit 
this by first measuring what impact this method has in practice (e.g. is it 
actually re-rendering anything in alphaMask(), or is that pixmap already there 
by this point, etc..)

> I would be happy to look at the handles further and see how they can
> be improved/changed in their behavior and existence in the codebase.

great! :) ok, so now we just have to figure out what that means exactly ;) see 
my (soon) reply to marco's email for more on that.

> I can't commit cause I don't have an svn account (thus I send the patches
> :)

you have no idea how easy that is to fix ;)

	 http://techbase.kde.org/Contribute/Get_a_SVN_Account

if you wish to continue working on this, and i think done "right" it will 
require more than one or two small patches, you should consider getting an 
account so you can work directly with us in trunk. your patch looks sensible 
and you obviously have had at least a bit of a look around the code base ;) so 
getting an account to join us more directly is reasonable.

> In general I like the handles and I think that they could be used to
> make the plasmoids more interactive for those that want it.

agreed

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