Thanks for your great response Jon,
I've committed two intemediatary refactors to make the logic easier to move out:
http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/12411 http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/12413
It reduces code size by 68 lines as well as reducing the interaction between the action class and the AlignAndDistribute class. I checked up on the two mentioned hacks/bugs I cleared up and testing for them shows no ill effect with the new code.
There is an addition to the Selection class: largestItem(...) and smallestItem(...) which in turn call a private _sizeistItem(...) that allows any function to return the largest/smallest item by width, height or area. I'd have liked to use the width/height/area Geom::OptRect functions but they're missing in this specific class and I couldn't really tell why they were available to some Rects and not others since the code looks the same. (obviously libgeom changes are a no-go)
I'll move the actually align function into helper, though I may need to look at other helper functions to understand their structure. (lack of confidence in spawning new files and placing them where-ever in the code base) Any help here would be most welcome.
Thanks for your advice,
Best Regards, Martin Owens
On Wed, 2013-07-10 at 00:14 -0700, Jon Cruz wrote:
On Jul 9, 2013, at 7:22 AM, Martin Owens wrote:
I've been looking at the align-and-distribute code and want to clear it up for use with my new functionality.
My idea is to move the core function into selection like this:
Selection::align(Geom::OptRect bbox, AlignSide align, bool as_group);
That way align-and-distribute can specify the bbox from all it's options and other functions can align selections. It should reduce repetition and allow my code to call more directly into the effective code without navigating the ui specific code.
I'd like your thoughts before I spend some time on this refactor.
It might also be worth moving distribute too, I have some great UI ideas for how to do distribute with knot handles too.
In general that seems like a good idea.
However, I think that we might want to have the operation outside of the selection class. That falls in line with a principal of favoring non-member functions.
One could ask "is self aligning an inherit property of the selection-ness of a selection?" In this case it seems to me that it is not. That leaves it similar to STL's algorithms. You can #include <algorithm> to get all sorts of helpful things that can operate on containers, but are not member methods of those container classes.
SomeHelper::align(Selection &selection, Geom::OptRect const &bbox, bool as_group);
Notice also that I flipped bbox to be a reference to a constant OptRect (remember declarations are read right-to-left). Among other things, that avoids duplicating structs as calls are made. Also... "SomeHelper" could be the name of some namespace, and not a class. This seems like it should not require an instance of some object to drive the functionality.
And we can utilize "using" to get that namespace out of the way. At the top of some file using the align functionality, we can have
using SomeHelper::align;... align(selection, bbox, true);
or perhaps give it a convenient abbreviation for use in a given .cpp file:
namespace xyz = SomeHelper;... xyz::align(selection, bbox, true);
The latter case might be better, as it keeps the functions marked as to where/why they come from.
Then there is the final parameter. When reading the code they boolean does not convey as much context as to what is going on. Some simple enum can work to keep the code more legible and maintainable:
xyz::align(selection, bbox, ALIGN_FLG_GROUP);Of course, the enum values need to have short meaningful names. I'm not sure I've captured the essences of the meaning of the boolean, so that name probably needs tuning.