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
2013/8/18 mathog <mathog@...1176...>:
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?
Obviously, it converts one inch to pixels.
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);
Indeed, this is obfuscated. It should be just:
p0[X] = Inkscape::Util::Quantity::convert(p0[X], "px", "in")
Another problem is that the name chosen for this function is extremely long. I have no idea who invented the "Util" namespace, this should all be in the Inkscape namespace preferably as a global function (gratuitous use of namespace is a plague in our code), or what does "Quantity" mean. Ideally the code should look like this:
p0[X] = Inkscape::convert_unit(p0[X], "px", "in");
I think this is more expressive that p0[X] * IN_PER_PX.
Regards, Krzysztof
On Tue, 2013-08-20 at 17:36 +0200, Krzysztof Kosiński wrote:
Another problem is that the name chosen for this function is extremely long. I have no idea who invented the "Util" namespace, this should all be in the Inkscape namespace preferably as a global function (gratuitous use of namespace is a plague in our code), or what does "Quantity" mean. Ideally the code should look like this:
p0[X] = Inkscape::convert_unit(p0[X], "px", "in");I think this is more expressive that p0[X] * IN_PER_PX.
Put this at the top of the code:
using namespace Inkscape::Util;
Then it /should/ be a case of calling:
p0[X] = Quantity::convert(p0[X], "px", "in");
Which makes a lot of sense. The only thing more convoluted would be:
p0[X] = convert(Quantity(p0[X], "px"), "in");
Martin Owens,
On 20-Aug-2013 08:59, Martin Owens wrote:
Could somebody please summarize what 12471 does?
Because if we are still doing things like
p0[X] = Inkscape::convert_unit(p0[X], "px", "in");
or
p0[X] = Quantity::convert(p0[X], "px", "in");
then it seems that whatever else 12471 implements, coordinates are still dimensionless. So all of this mucking around with syntax is in the end still only specifying a single multiplicative factor, albeit one that maybe now can be changed while the program is running. In other words, bottom line, 95% (or more) of the files modified in this revision did not need to be touched if the appropriate modifications had been made to the include file, so that PX_PER_IN and friends used the new methods. Moreover, if the syntax is changed again, at some point, then the same hundreds of lines will need to be reedited. Functionally though, all of those lines will _still_ be doing the same thing. That is why defines are used, so that massive code reediting is not required when these sorts of underlying changes occur.
I just don't understand the rationale for getting rid of the defines and needlessly modifying so many lines of code. Is there a programming movement in vogue now that holds that "# define" is evil, and that the more double colons on a line, the better???
David Mathog mathog@...1176... Manager, Sequence Analysis Facility, Biology Division, Caltech
On Aug 20, 2013, at 8:59 AM, Martin Owens wrote:
On Tue, 2013-08-20 at 17:36 +0200, Krzysztof Kosiński wrote:
Another problem is that the name chosen for this function is extremely long. I have no idea who invented the "Util" namespace, this should all be in the Inkscape namespace preferably as a global function (gratuitous use of namespace is a plague in our code), or what does "Quantity" mean. Ideally the code should look like this:
p0[X] = Inkscape::convert_unit(p0[X], "px", "in");
I think this is more expressive that p0[X] * IN_PER_PX.
Put this at the top of the code:
using namespace Inkscape::Util;
Then it /should/ be a case of calling:
p0[X] = Quantity::convert(p0[X], "px", "in");
Which makes a lot of sense. The only thing more convoluted would be:
p0[X] = convert(Quantity(p0[X], "px"), "in");
However... often "using namespace..." is considered not a best approach. A comparison is frequently made to "import foo.*" in Java code. Conceptually one should have a 'main' namespace for a single .cpp file, and that namespace could be designated by the "using namespace" directive. Alternatively, the same effect can be achieved by having an explicit "namespace foo {" block.
In this specific case, instead of "using namespace..." we can go with
using Inkscape::convert_unit;
So the using declaration, as opposed to the using directive, can be more helpful, calls out to the developer what might be covered in the file, avoids name collisions possible when more than one using directive is involved, etc. So a few "using xxxx;"'s instead of a single all-encompasing "using namespace yyy;"'s is usually a better choice.
On the other hand, keeping *something* in regards to a namespace explicit can significantly boost the legibility and ease of maintaining code. e.g.
std::string foo;
instead of
string foo;
(for this type of issue, it's very common to find warnings to not do "using namespace std". That's often employed for brevity in examples in class, etc., but should not be used in real code)
The main problem we hit in this specific case (as Krzystof called out) is that our namespace is too long.
Great news!!!! C++ already addresses that.
To make code in a specific C++ source file less verbose yet still keep it legible, we can just employ an alias.
namespace qnty = Inkscape::Util::Quantity; ... p0[X] = qnty::convert(p0[X], "px", "in");
On 21-8-2013 8:20, Jon Cruz wrote:
The main problem we hit in this specific case (as Krzystof called out) is that our namespace is too long.
Great news!!!! C++ already addresses that.
To make code in a specific C++ source file less verbose yet still keep it legible, we can just employ an alias.
namespace qnty = Inkscape::Util::Quantity;... p0[X] = qnty::convert(p0[X], "px", "in");
This is a great tip, I did not know this, thanks!
- Johan
On 20-Aug-2013 08:36, Krzysztof Kosiński wrote:
Ideally the code should look like this:
p0[X] = Inkscape::convert_unit(p0[X], "px", "in");I think this is more expressive that p0[X] * IN_PER_PX.
We disagree on that. I still think the direction of the conversion is ambiguous and that the (still) longer expression is less readable. The older version did not require a maintenance programmer to consult the documentation, the newer one does. Of course one could wrap the new one up in another method with an explicit name, like:
p0[X] = Inkscape::px_to_in(pO[X]);
That is at least unambiguous in regards to the conversion being performed, but it is still no improvement over the older:
p0[X] *= IN_PER_PX;
Starting with the KISS concept, as embodied in the previous code, what is it that is gained by using the more verbose "method form" that outweighs the simplicity of the earlier define? Is it that you want the conversion to be in a functional form? If so, there is always
(in one .h file, where the previous IN_PER_PX was defined) #define PX_TO_IN(A) Inkscape;:convert_unit(A,"px","in");
and (everywhere else)
po[X] = PX_TO_IN(po[X]);
I have no problem with doing it that way.
Regards,
David Mathog mathog@...1176... Manager, Sequence Analysis Facility, Biology Division, Caltech
2013/8/20 mathog <mathog@...1176...>:
p0[X] = Inkscape::px_to_in(pO[X]);
That is at least unambiguous in regards to the conversion being performed, but it is still no improvement over the older:
p0[X] *= IN_PER_PX;
In the former case, the statement reads naturally from left to right: p0[X] is converted from pixels to inches. In the latter, you need t think a few second about what is being performed.
However, that's still not perfect, because in the former example, 'px' is visually nearer to the quantity that's actually expressed in inches after this call. So possibly the best way to write this would be:
p0[X] = inches_from_pixels(p0[X]);
then it seems that whatever else 12471 implements, coordinates are still dimensionless. So all of this mucking around with syntax is in the end still only specifying a single multiplicative factor, albeit one that maybe now can be changed while the program is running.
True, that seems to be the case. However, even if the situation only improved a little, removing 3 different unit definitions is still a good thing. Changing the 'convert' calls to something more readable should be possible simply by running grep + sed over the source.
Regards, Krzysztof
On Wed, 2013-08-21 at 02:00 +0200, Krzysztof Kosiński wrote:
p0[X] = inches_from_pixels(p0[X]);
What concerns me about this syntax is that it's a many 2 many implication; i.e that there might be other calls like feet_from_rods available to them.
It's possible that to and from pixel conversion is really the standard and there we have a one to many two way concept that should just be 'p = from_inches( i )' and 'i = to_inches( p )'. I understand why explicitly stating pixels is attractive but it gives off the wrong impression.
Then I don't /like/ saying any units in the code at all. If we're specifying units for non-default-in-file-format code, then something might be amiss. Although I haven't studied the code well enough to work out why we're converting to inches here anyway.
Martin,
P.S. Lay off complaining about double colons, namespacing can be used for good as well as evil.
On 20-Aug-2013 19:10, Martin Owens wrote:
Although I haven't studied the code well enough to work out why we're converting to inches here anyway.
Some of these conversions are found in the input/output code that support other file formats, where dimensions are specified/required in units other than pixels. For instance in the EMF and WMF sections these conversion factors to PX are used: meter, inch, millimeter, world. (Page and Device coordinates are also present, but going to and from those requires a full transform matrix instead of a single scale value.) It seems pretty likely that 12471 will break something in here when the px/in ratio changes from the current default, and I need to test that. So, can somebody please tell me where in the GUI 12471 introduced a control that changes that ratio?
Thanks,
David Mathog mathog@...1176... Manager, Sequence Analysis Facility, Biology Division, Caltech
On 21-8-2013 18:19, mathog wrote:
On 20-Aug-2013 19:10, Martin Owens wrote:
Although I haven't studied the code well enough to work out why we're converting to inches here anyway.
Some of these conversions are found in the input/output code that support other file formats, where dimensions are specified/required in units other than pixels. For instance in the EMF and WMF sections these conversion factors to PX are used: meter, inch, millimeter, world. (Page and Device coordinates are also present, but going to and from those requires a full transform matrix instead of a single scale value.) It seems pretty likely that 12471 will break something in here when the px/in ratio changes from the current default, and I need to test that. So, can somebody please tell me where in the GUI 12471 introduced a control that changes that ratio?
There is no control in the GUI for this. But you can change the ratios in /share/ui/units.xml.
Cheers, Johan
On 20-8-2013 18:53, mathog wrote:
On 20-Aug-2013 08:36, Krzysztof Kosiński wrote:
Ideally the code should look like this:
p0[X] = Inkscape::convert_unit(p0[X], "px", "in");I think this is more expressive that p0[X] * IN_PER_PX.
We disagree on that. I still think the direction of the conversion is ambiguous and that the (still) longer expression is less readable. The older version did not require a maintenance programmer to consult the documentation, the newer one does.
The older version makes it look like the conversion factor is a constant (like it was), instead of a function call (like you propose). I agree with Krzysztof that the new version is nicer. Perhaps one could play with spacing a bit to make it clearer:
p0[X] = Inkscape::convert_unit(p0[X],"px", "in");
But really I don't see much need to look up documentation for this one. To me it seems pretty obvious that the code converts p0 from px to in. I think we should be able to change this code to
p0 = Inkscape::convert_unit(p0,"px", "in");
Overload the conversion function with one supporting Geom::Point, and Geom::Rect maybe? (don't know how often one would want conversion for Rect)
Cheers, Johan
On 27-Aug-2013 14:20, Johan Engelen wrote:
I think we should be able to change this code to
p0 = Inkscape::convert_unit(p0,"px", "in");
Or use a method instead of a function? The old way where conversions were done by multiplying by PX_PER_IN and friends was C like, the one above is about half way between C and C++. Wouldn't all the way to an object oriented programming style be:
p0 = p0.in2px();
and
p0 = p0.px2in();
That style would be fine by me - it is perfectly clear what they do. Sure, you would need a pair of conversion methods for each new unit to px, but only into and out of px, not between all possible combination of units. Ie:
p0 = po.in2px().px2cm();
Regards,
David Mathog mathog@...1176... Manager, Sequence Analysis Facility, Biology Division, Caltech
On Tue, Aug 27, 2013 at 7:06 PM, mathog <mathog@...1176...> wrote:
On 27-Aug-2013 14:20, Johan Engelen wrote:
I think we should be able to change this code to
p0 = Inkscape::convert_unit(p0,"px", "in");Or use a method instead of a function? The old way where conversions were done by multiplying by PX_PER_IN and friends was C like, the one above is about half way between C and C++. Wouldn't all the way to an object oriented programming style be:
p0 = p0.in2px();
and
p0 = p0.px2in();
It seems like halfway between C and C++ because it is. That syntax is used in older parts of the code that don't use the new Quantity class objects that store values with their units. Where Quantities are used [1], the syntax is:
quantity.value("in")
if one wanted the value in inches, no matter what unit the quantity was in. This syntax doesn't work with older sections of code without extensive rewrites, as the older sections store everything as regular floating point numbers, not objects.
-Matthew
[1] an example: http://bazaar.launchpad.net/~matthewpetroff/inkscape/gsoc-2013-unit-improvem...
On 27-Aug-2013 18:56, Matthew Petroff wrote:
On Tue, Aug 27, 2013 at 7:06 PM, mathog <mathog@...1176... [1]>
Wouldn't all the way to an object oriented programming style be:
p0 = p0.in2px();
It seems like halfway between C and C++ because it is. That syntax is used in older parts of the code that dont use the new Quantity class objects that store values with their units. Where Quantities are used [1], the syntax is:
quantity.value("in")
if one wanted the value in inches, no matter what unit the quantity was in. This syntax doesnt work with older sections of code without extensive rewrites, as the older sections store everything as regular floating point numbers, not objects.
Sure, but could one not put something like this (exact syntax is probably off, I did not try to compile it, and the conversion might be backwards) into src/2geom/Point.h
Point in2px const { return Point(Inkscape::convert_unit(_pt[X],"px", "in"),Inkscape::convert_unit(_pt[Y],"px", "in")); }
to implement:
p0 = p0.in2px();
What I'm getting at is that when I am maintaining code the last thing I want to see is low level plumbing, like convert_unit, the exact function being used to convert units. The desired level of abstraction was present before 12471, with PX_PER_IN and friends. Most of those were expanded by the preprocessor into fairly long expressions, none of which I cared about, and none of which were exposed to the programmer. Replacing the old include file method with convert_unit() is fine, but I _still_ don't want or need to see those details, and I especially do not want to burn >20 characters per unit change instance on each source code line.
Regards,
David Mathog mathog@...1176... Manager, Sequence Analysis Facility, Biology Division, Caltech
On Wed, 2013-08-28 at 09:57 -0700, mathog wrote:
p0 = p0.in2px();
I do agree with you that unit handling should be subsumed into these objects. But not like this.
in2px is a despicably static name and I hate it. It's got variables in it instead of describing what should be done, it says what from and to.
p0.in("px") p0.in("in")
Martin,
On 28-8-2013 18:57, mathog wrote:
On 27-Aug-2013 18:56, Matthew Petroff wrote:
On Tue, Aug 27, 2013 at 7:06 PM, mathog <mathog@...1176... [1]>
Wouldn't all the way to an object oriented programming style be:
p0 = p0.in2px();It seems like halfway between C and C++ because it is. That syntax is used in older parts of the code that dont use the new Quantity class objects that store values with their units. Where Quantities are used [1], the syntax is:
quantity.value("in")
if one wanted the value in inches, no matter what unit the quantity was in. This syntax doesnt work with older sections of code without extensive rewrites, as the older sections store everything as regular floating point numbers, not objects.
Sure, but could one not put something like this (exact syntax is probably off, I did not try to compile it, and the conversion might be backwards) into src/2geom/Point.h
Point in2px const { return Point(Inkscape::convert_unit(_pt[X],"px", "in"),Inkscape::convert_unit(_pt[Y],"px", "in")); }
to implement:
p0 = p0.in2px();What I'm getting at is that when I am maintaining code the last thing I want to see is low level plumbing, like convert_unit, the exact function being used to convert units. The desired level of abstraction was present before 12471, with PX_PER_IN and friends.
Troll??? When doing a unit conversion, you don't want to write the function that does the unit conversion? But it *is* ok to write "in2px()" ?
-Johan
On 28-Aug-2013 10:05, Johan Engelen wrote:
When doing a unit conversion, you don't want to write the function that does the unit conversion? But it *is* ok to write "in2px()" ?
It is a trade off, with in2px being close or at the minimal unambiguous representation of the action of the method. "in2px" says what happens, and it hides all other details. The next programmer who sees the code, will not need to refer to the manual to determine the direction of the conversion.
I still maintain that my original suggestion was the proper course of action. Roll back 12471, add its implementation of the new convert code, which is just a couple of files, then cause that code to be used elsewhere in the existing, unmodified code, by changing the pre-12471 include file like this
#define PX_PER_MM Inkscape::Util::Quantity::convert(1, "mm", "px")
and so forth. That way convert() is available, but adding it does not result in the needless modification of 40 something other files, making every one of them more difficult to read and less clear than the originals (IMO).
David Mathog mathog@...1176... Manager, Sequence Analysis Facility, Biology Division, Caltech
On Wed, 2013-08-28 at 11:40 -0700, mathog wrote:
"in2px" says what happens, and it hides all other details.
Actually what happens is just '2', the other two bits either side are static arguments. It might be clear but it's sheet-glass code. Clear but inflexible and down right naughty in a OO codebase.
I wouldn't be complaining, but you keep on suggesting it like it's fine.
Martin,
His next sentenced explained his meaning...
On Wed, Aug 28, 2013 at 3:01 PM, mathog <mathog@...1176...> wrote:
On 28-Aug-2013 13:35, Martin Owens wrote:
sheet-glass code
I have never encountered that term before and could not find it with Google. Reference?
David Mathog mathog@...1176... Manager, Sequence Analysis Facility, Biology Division, Caltech
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more! Discover the easy way to master current and previous Microsoft technologies and advance your career. Get an incredible 1,500+ hours of step-by-step tutorial videos with LearnDevNow. Subscribe today and save! http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clk... _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
2013/8/28 mathog <mathog@...1176...>:
It is a trade off, with in2px being close or at the minimal unambiguous representation of the action of the method. "in2px" says what happens, and it hides all other details. The next programmer who sees the code, will not need to refer to the manual to determine the direction of the conversion.
I think we are arguing about the bike shed at this point. https://en.wikipedia.org/wiki/Parkinson%27s_law_of_triviality
The point is that these conversions between raw floating point numbers shouldn't be there at all. Instead, the code should be modified to use the Inkscape::Util::Quantity class, which contains both the number and the unit in which it is expressed.
Regards, Krzysztof
On 28-8-2013 1:06, mathog wrote:
On 27-Aug-2013 14:20, Johan Engelen wrote:
I think we should be able to change this code to
p0 = Inkscape::convert_unit(p0,"px", "in");Or use a method instead of a function? The old way where conversions were done by multiplying by PX_PER_IN and friends was C like, the one above is about half way between C and C++. Wouldn't all the way to an object oriented programming style be:
p0 = p0.in2px();and
p0 = p0.px2in();
No it wouldn't. C++ or OOP are not about adding methods or classes everywhere.
- Johan
participants (7)
-
Johan Engelen -
Jon Cruz -
Josh Andler -
Krzysztof Kosiński -
Martin Owens -
mathog -
Matthew Petroff