Finn, 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.
Advertising
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. (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. 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. 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]. 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. Sorry for the long post. I'll probably have other comments in other areas, but this is all I've studied for now. Thoughts? Thanks, Glen __________________________________ Do you Yahoo!? Yahoo! Hotjobs: Enter the "Signing Bonus" Sweepstakes http://hotjobs.sweepstakes.yahoo.com/signingbonus
