I've looked at your changes--I like them, and I'm thankful to have someone on our team to be able to redesign the properties as you have. Getting rid of the 250 autogenerated or so classes will be a welcome improvement.
But the biggest improvement is IMHO the easy ability to create special maker subclasses to handle the corner cases. Take a look at IndentPropertyMaker for the calculation of start and end-indent and at BorderWidthPropertyMaker for the special handling of border-width when border-style is NONE.
Comments right now:
1.) Unlike what I was saying earlier, I don't think we should move from Property.Maker to a new PropertyMaker class after all, your design looks fine. I've noticed most subclasses of Property.Maker are within subclasses of Properties themselves (e.g., LengthProperty, LengthProperty.Maker, etc.) so it looks like a neat, clean design.
2.) The new FOPropertyMapping.java class appears (1)
autogenerated, and (2) to be an XSLT masterpiece at
that as well. If it is indeed in good shape, I'd like
you to submit it to Bugzilla as the new
fo-property-mapping.xsl, replacing the old one of that
name in src/codegen.
Initially my new FOPropertyMapping was generated by XSLT but that is now a long time ago and I have made lots of manual changes since then. The XSLT script only handled the most common property information and was just a hack to get me started. The output isn't a complete java file, it doesn't link the subproperties to the base properties and it doesn't deal with the classname of any of the complex properties.
(We won't apply it however, until we no longer need the current autogenerated fo-property-mapping.xsl, i.e., until all the old properties have been tossed out.)
This way if we have to make wide-ranging changes to FOPropertyMapping, we'll have a XSLT source file we can conveniently work with. (Note that putting it in codegen does *not* mean that it will be automatically autogenerated anymore--it won't, just as constants.xsl no longer is--we'll pull it out of the main Ant build target at that time and keep it the separate, manual xsltToJava target in our build file[1].
[1] http://cvs.apache.org/viewcvs.cgi/xml-fop/build.xml?rev=1.97&view=auto )
Comments on FOPropertyMapping:
I like removing all these autogenerated classes, but I think we can still keep some processing at compile-time for more of a performance gain, as follows:
3) I think the runtime construction of the generic
properties (genericColor, genericCondBorderWidth,
etc.) may not be necessary. We can still have those
xslt-generated into classes (6-8 classes total), but
this time we check them into FOP (again, keeping the
xsl available for manual re-generation when needed). But most of the generic classes are so small (your
initialization of GenericCondPadding is only 4 lines
of code), that going back to creating concrete classes
would be noticeably beneficial either, so I'm not
recommending this change.
The generic properties are just templates that carries default data values to be used later on, so I don't fully see how we could xslt- generate them as anything other than containers of default values. Your last statement is a bit difficult to parse so I'm not sure what exactly you are recommending.
One thing that *does* stick out, however, is the 100 or so addKeyword() calls for genericColor (the largest of the generic properties):
genericColor.addKeyword("antiquewhite", "#faebd7"); genericColor.addKeyword("aqua", "#00ffff"); genericColor.addKeyword("aquamarine", "#7fffd4"); .....
I'd like us to have a static array of these
values--i.e., something done compile-time, that
genericColor can just reference, so we don't have to
do this keyword initialization.
I probably need an example of what you thinking are here. Right now in HEAD all the color keywords are stored in a HashMap created in GenericColor so the keywords initialization is already done. Putting the keywords in static array would require us to somehow search the array and I don't see how that will be much faster.
You should perhaps also be aware that the values in a static array gets assigned to the array one element at a time. So
static int[] a = { 101,102,103,104,105,106,107,108 };
becomes in bytecodes:
Method static {}
0 bipush 8
2 newarray int
4 dup
5 iconst_0
6 bipush 101
8 iastore
9 dup
10 iconst_1
11 bipush 102
13 iastore
14 dup
15 iconst_2
16 bipush 103
18 iastore
...and so on for each index. (In case you don't know bytecode by heart, iconst and bipush both push a constant on the stack and iastore pops 3 items from the stack; an index, a value and an array and assign the value to the index in the array).
>4) I'd also like us to, rather than call setInherited() and setDefault() for each of the properties during initialization, for the Property/Property.Maker classes to just reference that information from two (new) static arrays, added to Constants.java. We can also get rid of these two setter methods as well (ideally there shouldn't be setters for these attributes anyway--they should remain inherent to the Property.)
This change will allow us to take advantage of the
fact that we are now on int-constants. getDefault(PR_WHATEVER), for example, is just
Constants.DefaultArray[PR_WHATEVER].
I think 'Default' is a bad example, noone ever tries to get the default value except for the property maker itself, but your argument holds for isInherited().
Still, I disagree. If one want to know is a property is inherited, the proper way to get the information should be to call propertyMapping[PR_WHATEVER].isInherited().
5) Similar to (b) above, several of the makers also have a "useGeneric()" initialization requirement:
m = new CondLengthProperty.Maker(PR_PADDING_END); m.useGeneric(genericCondPadding);
For those Makers that require it, I'd like the constructor to be expanded to this:
m = new CondLengthProperty.Maker(PR_PADDING_END, genericCondPadding);
Again, getting rid of the useGeneric() function. This is for more speed, encapsulation, and also shrinking FOPropertyMapping class a bit.
A very good idea. +1.
regards, finn
