I recently became aware of revision 12471, and overall, I am afraid that the way in which it was implemented is detrimental to the readability and future maintenance of the Inkscape code. Presumably the underlying function it implements is to allow changes in the ratio PX/IN, and other units of measurement, and I have no issue with that goal. However, prior to 12471 all of the unit relationships were neatly set up in a set of defines in src/unit-constants.h which defined things like PX_PER_IN which were used elsewhere in the code. These evaluated to static values, and the new ones do not, but that does not negate the usefulness of the define.
12471 removes all of these defines and makes substitutions throughout the code where
PX_PER_IN
is replaced by
Inkscape::Util::Quantity::convert(1, "in", "px")
My first criticism is that the new quantity is at first glance, indeterminate. Is it converting IN to PX or vice versa, what does "1" do?
My second criticism is that eliminating the defines results in hundreds of substitutions in 12471 that are entirely unnecessary. All that was required was to change the defines in src/unit-constants.h, for instance from
#define PX_PER_IN (PT_PER_IN / PT_PER_PX)
to
#define PX_PER_IN Inkscape::Util::Quantity::convert(1, "in", "px")
after which virtually no changes elsewhere in the code are required. By doing that, the existing and relatively clear code, like:
p0[X] = (p0[X] * IN_PER_PX * dwDPI);
need not become the obfuscated (to my mind) form:
p0[X] = (p0[X] * Inkscape::Util::Quantity::convert(1, "px", "in") * dwDPI);
Before 12471 PX_PER_IN already evaluated to a complex expression, and the whole point of the define was to let the compiler handle that mess and only present to the programmer a short easily understood term. 12471 is a step backwards in this regard.
Regards,
David Mathog mathog@...1176... Manager, Sequence Analysis Facility, Biology Division, Caltech