ColorUtil, as the name implies, is a utility class and it doesn't feel
right to put an instance of it in FOUserAgent. I would extract the
profile caching into a separate class and put that there (or rather in
the FopFactory so multiple rendering runs can use the cache). The
ColorUtil methods should then usually receive an instance of the profile
cache to work with.

You will easily see from the unit tests if anything breaks. I will also
do extensive tests when reviewing your patch. Since it probably needs
some additional work afterwards, we will probably put it in a branch
until it's ready to be merged back into Trunk. Don't be too afraid to
change some method signatures. What needs to be done, needs to be done.

On 11.10.2006 14:07:51 Peter wrote:
> Some more information....found during the last few hours....and finished
> typing in just before your reply came in.
> 
> I have been trying to figure out whether it would help to add an instance of
> ColorUtil to the FOUserAgent (and changing some of its methods and members
> to instance level) 
> 
> Skipping to the conclusion first - in theory it seems possible to make
> ColorUtil#parseColorString an instance method and add a ColorUtil instance
> to the FOUserAgent. In theory, because API wise an FOUserAgent can always be
> made available (given enough API changes) but I have no idea whether it will
> actually be there at runtime (could be null). 
> 
> Actually there might a problem with one or more static function of one of
> the calling classes/methods I just realize.
> 
> Anyway, changes are rather extensive and I am not sure whether they will
> work or break anything I overlooked.
> 
> Please advice - I am willing to give it a try but need to decide asap (I am
> running out of my time slot that was made available for fop dev work for the
> coming weeks).
> 
> Thanks,
> 
> Peter
> 
> 
> What I found out, ColorUtil is referenced from
> 
> 1/ colorEqualTest 
> ==> Not an issue I guess (test)
> 
> 2/ AreaTreeParser.Handler#setTraits 
> ==> Not an issue as an FOUserAgent is available
> 
> 3/ Trait.Background#toString ==> FOUserAgent not needed
> ==> Not an issue as an FOUserAgent is not needed (only colorToString is
> called)
> 
> 4/ NCNameProperty#getColor 
> ==> NO FOUserAgent ==> ISSUE
> 
> 5/ ColorProperty#ColorProperty 
> ==> NO FOUserAgent ==> ISSUE
> 
> 6/ ColorProperty#toString ==> FOUserAgent not needed
> ==> Not an issue as an FOUserAgent is not needed (only colorToString is
> called)
> 
> 7/ XMLRenderer#addTraitAttributes ==> FOUserAgent not needed
> ==> Not an issue as an FOUserAgent is not needed (only colorToString is
> called)
> 
> 8/ BorderProps#toString ==> FOUserAgent not needed
> ==> Not an issue as an FOUserAgent is not needed (only colorToString is
> called)
> 
> 9/ BorderProps#valueOf 
> ==> NO FOUserAgent ==> ISSUE
> 
> To summarize three invocations of ColorUtil#parseColorString give problems
> 
> I went a level further
> 
> 4/ NCNameProperty#getColor 
> ==> NO FOUserAgent ==> But all callers of getColor seem to have access to
> something that in theory should be able to return the FOUserAgent
> ==> No longer an issue
> 
> 
> 5/ ColorProperty#ColorProperty 
> ==> NO FOUserAgent ==> But all callers of ColorProperty constructor seem to
> have access to something that in theory should be able to return the
> FOUserAgent  ==> No longer an issue
> 
> 9/ BorderProps#valueOf 
> ==> NO FOUserAgent ==> ==> But all callers of valueOf (one :)) seem to have
> access to something that in theory should be able to return the FOUserAgent
> ==> No longer an issue
> 
> 
> 
> 
> > -----Original Message-----
> > From: Jeremias Maerki [mailto:[EMAIL PROTECTED]
> > Sent: Wednesday, October 11, 2006 2:05 PM
> > To: fop-dev@xmlgraphics.apache.org
> > Subject: Re: Including an sRGB color profile?
> > 
> > Ok for me, as long as we track the outstanding issues.
> > 
> > On 11.10.2006 12:01:19 Peter wrote:
> > > >
> > > > Would you please try to run URI resolution through
> > > > FOUserAgent.resolveURI()? That way we'll have a uniform URI resolution
> > > > everywhere.
> > >
> > > I understand your concern. I will look into it but the issue is that
> > > ColorUtil is really a bunch of static methods that at this stage have no
> > > access to any instance objects. I am sure that can be changed but I am
> > > concerned this is going to ripple down to a lot of files and I would
> > rather
> > > start out by leaving as much code as possible untouched.
> > >
> > >
> > > > Uhm, that's not so good. We were trying to avoid statics where
> > possible.
> > > > I'd prefer to keep that Map in FOUserAgent (per rendering run), or if
> > > > you want to cache the profiles, we should make a cache class with
> > > > SoftReferences similar to what is implemented in ImageFactory and
> > attach
> > > > that to the FopFactory. But I can look into this. I don't want to put
> > > > too much on your shoulders if you don't want to. It's cool enough that
> > > > you're putting so much energy into this.
> > >
> > > Basically the same issue here. ColorUtil is static....and as a (lousy)
> > > excuse, I mainly just duplicated the colorMap cache "idea"....but I
> > agree
> > > one should avoid those static beasts. Not caching the ColorProfiles on
> > the
> > > other hand seems not a good idea either, from a memory consumption
> > > perspective.
> > >
> > > Again (sorry for repeating myself) I would rather see this included and
> > > continue from there...building up fop-dev experience step by step.
> > 
> > 
> > 
> > Jeremias Maerki



Jeremias Maerki

Reply via email to