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.

On Thu, 2013-07-11 at 07:30 +0100, Nicolas Dufour wrote:

It seems that "small" is some kind of data type extension in MinGW. Replacing it with "sml" (just for testing) fixes the issue for me.

Good catch, thanks. pushed a fix for you.

Martin,

On Thu, 2013-07-11 at 15:38 +0200, su_v wrote:

All objects are moved, including the one which acts as anchor, and aligned to the position of the respective bbox edge of the original position of the anchor object.

Just curious - is this an intentional change?

It sounds like a real regression. Could you post a bug report with an attached svg? It'd be good to get the expected and regressed locations expressed as guidelines on the page too if that's possible.

Thanks for testing :-) I want to make sure this refactor keeps the same functionality and any deviation should be queried. I tested all the modes, but I guess I missed something.

Martin,

---- Martin Owens <doctormo@...400...> wrote: [...]

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)

The main reason we have our own lib2geom copy is to be able to make changes to it! lib2geom changes are a YES-go! :-) However, don't commit the changes only to Inkscape's trunk; you will have to commit them to lib2geom too (otherwise they will be overwritten when updating our local copy from lib2geom's trunk).

Which Rects do have area functions? I think it'd be nice to add an area method to the Rect class in 2Geom.

regards, Johan

On Jul 11, 2013, at 9:57 AM, Martin Owens wrote:

On Thu, 2013-07-11 at 18:40 +0200, jbc.engelen@...2592... wrote:

...

Which Rects do have area functions? I think it'd be nice to add an area method to the Rect class in 2Geom.

2geom/generic-rect.h:181

There's not just area, but height and width too. I wasn't able to call any of them from the Geom::OptRect objects in the code. Maybe I don't understand what it means by `C foo() const {}` and there is some special way to call it?

One point to keep in mind is that an OptRect differs in that it may or may not be there (thus it is optional). But first I'll do a quick rundown on some of the details of C foo() const {} for the benefit of others on the list.

For a breakdown on that bit of code, we need to look at what it references and the context:

In this case the mentioned method is in a template:

template <typename C> class GenericRect : CoordTraits<C>::RectOps { ...

and is

/** @brief Compute rectangle's area. */ C area() const { return f[X].extent() * f[Y].extent(); }

First is with the comment. The @brief is not really needed as it is ancient Trolltech stuff. The more modern Javadoc approach is that the first sentence is automatically the brief summary. Then again, good comments are the main point in question right now.

So... it's saying it will return some random type that we don't know yet, but decided to call "C" at the moment. C area() const { return f[X].extent() * f[Y].extent(); } ----^

Taking a peek at rect.h, we see class Rect : public GenericRect<Coord>

So that means the generic base template should give the Rect class an effective member of:

Coord area() const {...}

The next part on the base is 'const'

C area() const { return f[X].extent() * f[Y].extent(); } -------------^^^^^

That just says that the member is a const method. That promises that if you call this method it will have no externally visible effect on the instance it is being called from. So if you have a reference or pointer to a const Rect, you can still call this method.

void doSomething(Rect const &target) { Coord block = target.area();

If the method was not marked const, then an attempt to use it there would fail.

However, I'm thinking that you might be having an issue with the "Opt" part of "OptRect". To see if the OptRect contains a value or not, call the isEmpty() method on it. If it is empty, then be sure not to try to use it. Or use the bool operator to test it.

OptRect thing; ...

if (thing) { ????? }

Looking at our conversion cheat-sheet, we can see hints of how to access the underlying rect: http://wiki.inkscape.org/wiki/index.php/CheatSheetForConvertingto2geom

I believe in this case that boost::optional<t> needs to use '*', '->' or 'get()' to access the object... when it is actually present. Thus we see

if (thing) { Geom::Rect other = *thing; Coord block = other.area(); ... }

or

if (thing) { Coord block = thing->area(); ... }