On Sun, Jan 25, 2004 at 11:38:14PM +0100, Finn Bock wrote: > [Simon Pepping] > > >I have just catched up with the massive changes to the property > >system. Allow me to share a few observations: > > Thanks for your comments. How do you otherwise think it compares to the > previous generated property makers?
I could not recall how it was done before the patch. My notes only describe FOElementMapping, and I have not searched back in CVS. I find the set of Makers as done in FOElementMapping quite baroque. The current FOPropertyMapping is quite concise and elegant. On the other hand, I am a supporter of generating code from a description of base data in an XML file, and so saw a good side to the old method as well. I have not studied it in detail though. > >1. If I see correctly, PropertySets is not yet used. > > Correct. Its intended use is when we, at some point in the future, want > to store more than just the specified properties in the FObjs. > > > PropertyList is > > still a HashMap keyed on property name. Is this waiting for > > some other changes to be made? > > Yes. Either PropertyList should have a Property[] array indexed directly > by the propId or the HashMap should be keyed by an Integer object with > the same value as the propId: > > super.get(integerArray[propId]); > > where integerArray is initialized as: > > integerArray[1] = new Integer(1); > integerArray[2] = new Integer(2); > ... > > Which of these it will be depends on how much information we decide to > store in the Fobj. Your patch mentions the sparse array PropertyList.values for this purpose. You made much effort to construct it. A propos, the loop in PropertySets.initialize is not easily understood; I needed the explanation you gave in an email to understand it. It would be good to add an explanation to the code. If it helps, this is how it documented it in my own notes: 'For each FO type the BitSet of allowed properties is merged with the BitSet of allowed properties of its possible direct children. When for any FO type the mergeContent subroutine modifies its BitSet, it sets the boolean variable 'loop' to true to signal that another iteration of the loop is required. By iterating over the loop until no further modifications are made, one makes sure that the merging process propagates from below to the top, that is, from any FO type to its farthest possible ancestor. This ensures that every FO type registers the allowed properties of itself and of all FO types that may ever appear in its subtree.' I think the name 'modified' or 'dirty' for the loop variable would be more descriptive. > >2. In FOPropertyMapping the word 'generic' is used in two different > > senses: in s_generics and getGenericMappings() on the one hand, > > and in genericBoolean etc., createGenerics() and useGeneric() on > > the other hand. This may be confusing. One might e.g. be tempted to > > think that s_generics contains the objects genericXxx only, which > > is not the case. > > You are absolutely correct. I've took the names from the code which > existed at the time. I'm also terrible at naming things so other > suggestions will be welcome. I suggest that we rename s_generics and > getGenericMappings to s_fomapping and getFoMappings because they deal > with the properties from the xsl:fo spec. It seems indeed best to rename these two. OTOH, getGenericMappings is the only symbol that is used by other classes. But because it is probably used only once, in FObj, it is not a problem to rename it. s_generics is a mapping of propId to Property.Maker, so it should be called something like PropertyMappings, or PropertyMakers (what does s_ stand for?). FO is very general, it could equally be used for element mappings. > >3. getGenericMappings rebuilds s_generics every time it is called. In > > the current code it seems to be called only once, by FObj when the > > class is loaded. Would it not be better if getGenericMappings > > itself would ensure that the array is built only once? > > Yes, but I would like to take the question a bit further and ask where > such information should be cached? Storing it in static variables caches > it in the classloader which makes it difficult to control the release of > the memory. > > Perhaps it would be better to store the information in the Driver or > FOUserAgent, IOW to put the caching under user control. > > If caching at the classloader level is the best way to do it, then it > makes perfect sense to do what you suggest. I agree with Glen's reply. This is information which the application needs to hold only once, and which may persist. The user has nothing to do with this mapping. Current FOP interleaves FOtree building with area tree building and rendering; only after the last pagesequence FO node has been built could it be released (or does area tree building access the makers as well, for non-resolved properties?), which provides hardly any gain; in runs with several documents it is even a loss. I wonder why FObj makes a copy of this list, and does not refer it directly. Regards, Simon -- Simon Pepping email: spepping AT leverkruid.nl home page: http://www.leverkruid.nl
