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

I wonder why FObj makes a copy of this list, and does not refer it

Regards, Simon

Simon Pepping
email: spepping AT leverkruid.nl
home page: http://www.leverkruid.nl

Reply via email to