This patch disable the code generation of the property maker classes and instead creates instances of the base maker types and configures them with setter methods. The patch is *not* intended to be committed as is, but as meant as inspiration. In addition to the stated change of property maker classes, the patch also contain many of the bug fixes I've submitted recently.
Created attachment 9777 [details] A unified diff against HEAD
You should have no complaints from anyone on reducing the amount of automatic code generation we use. But just to confirm though, *not only* would the generation of the 250 or so Maker classes we have, one for each property, be stopped--but also, we would no longer be having those classes at all either, correct? (I.e., FOP 1.0 goes down from 915 classes or so to roughly 700?)--That would be fantastic! Also, leaving out the time saving as a result of moving off Strings, etc., i.e., all the time benefits of the previous huge patch, what is the performance loss/gain as a result of *this* change? (Hopefully) negligable? Thanks, Glen
Never mind--I was typing this up while you were typing up your FOP-DEV email. Thanks for the explanations.
A new revision of the patch where the functionality of corresponding and compound properties are placed in seperate files. As a result, the main Property.Maker class becomes smaller and easier to understand. Performance is 4.5% faster with this patch, mostly due to the caching of the default values of subproperties. The main path through the code is shorter for non-compound properties and the same for compound properties and properties with corresponding. Functionally the patch is neutral, so the it gets the same result. There are some places in the patch marked with TODO where a solution to minor bugs in the existing code has been added as comments. With this revision I think the patch is ready to go in.
Created attachment 9981 [details] A unified diff against HEAD
Please give me a couple of days to comment--say 48 hours. My initial thoughts are "I like what I see", esp. since appeared to you appeared to unnest the Property.Maker, which looks good, but I'd like to study the code further. Everything looks good though. [Sorry for the delays this week--I was held up on other things--also was sending some emails to W3C trying to understand which properties can be specified to which FO. See http://lists.w3.org/Archives/Public/xsl- editors/2004JanMar/] One difference we have, I can work on, is that I'm not yet ready to get rid of all those interfaces. The current interface implementation is awful and useless--code-generated, separated into 45 different files, not visible while one is coding, etc., etc. I'd like to have them retained, but put into (1) file, actually, just added to the Constants interface (as inner interfaces), say adding about 600 lines in that interface for them all. (I can modify the XSLT code to accomplish that.) We get rid of those 45 files, and they will be no longer autogenerated with each build (but, as with the current Constants.java, we retain the XSLT to re- generate it when we like.) Reason why? I *think*, over the long-term, it is much more programmer-friendly because many/most developers use IDE's with code-complete. I.E., you type in the property value interface name, hit the ".", and then you automatically see the 5-7 values relevant for that property. This saves the programmer the headache of looking at the spec each time for which prop values you need to code against, or trying to recall from a huge Constants list the actual values you need, and also making sure all the property options have been coded against. I think it will be a nice sanity-saver for coders. If not, we can always excise them later from Constants.java. Thoughts on this? The only other issue right now--not necessarily related to your patch--is that I'd like us, where possible, to start abstracting the propertyList away from the Layout and Renderers, possibly also the Area Tree. (This was an idea originally put forth by Victor a few weeks back, to general agreement on the list.) FObj already has a getProperty() function which just wraps its internal propertyList.getProperty()--and it's already used in a dozen or so places. Instead of layoutManager calling prop = propList.getProperty() left and right, it will be prop = fObj.getProperty(). This will give us some freedom in how we implement properties without needing to make changes to other parts of our code (i.e., we can even get rid of propertyList.) Thanks, Glen
Finn, You have a change that doesn't appear relevant to this bug in InlineStackingLayoutManager.java: Index: src/java/org/apache/fop/layoutmgr/InlineStackingLayoutManager.java =================================================================== @@ -292,6 +292,10 @@ // Reset state variables clearPrevIPD(); // Clear stored prev content dimensions } + // I don't have a clue if this is right. + if (childLC == null) { + childLC = new LayoutContext(lc); + } // We only do this loop more than once if a childLM returns // a null BreakPoss, meaning it has nothing (more) to layout. -------------------------------- Should we skip this change? Thanks, Glen
Created attachment 10016 [details] current status of patch (down to about 4900 lines from 6200).
Glen, yes please skip that change to InlineStakingLM. The change avoid a NPE when generating the docbook example I've used for timing but I have never analysed the problem so I doubt the change is the correct fix.
Applied. Futher comments from xml-dev: http://marc.theaimsgroup.com/?l=fop-dev&m=107308498806385&w=2 http://marc.theaimsgroup.com/?l=fop-dev&m=107445625709662&w=2 http://marc.theaimsgroup.com/?l=fop-dev&m=107434372324847&w=2
batch transition pre-FOP1.0 resolved+fixed bugs to closed+fixed