So I think I have it working but not without substantial changes

A summary

1)FopFactory
============
*) Added a synchronized Map, colorSpaceMap to cache ...ColorSpace
objects(life can be easy). Don't know whether this has to be synchronized.
Have not looked into the details of FOP MT features.

*) Added a public method 
        public ColorSpace getColorSpace(
        String base, 
        String iccProfileSrc) 

Kind of awkward to have to pass in the base but the only way I could see to
work around that is to have the cache in the user agent iso the factory

2) AreaTreeParser.Handler#setTraits
===================================
*) Change invocation of ColorUtil.parseColorString (2x). It now needs a
first FOUserAgent argument
*) Change invocation of BorderProps.valueOf. Expects a first FOUserAgent
argument

3) NCNameProperty#getColor
==========================
*) New first FOUserAgent argument
*) Invoke ColorUtil.parseColorString with the FOUserAgent argument

4) NumericProperty#getColor
============================
*) New first FOUserAgent argument

5) PropertyParser#parsePrimaryExpr
==================================
*) ColorProperty constructor takes new first FOUserAgent argument

6) SystemColorFunction#eval
===========================
*) ColorProperty constructor takes new first FOUserAgent argument


7) ColorProperty
================
*) Constructor new first FOUserAgent argument
*) getColor - new first FOUserAgent argument

8) CommonBorderPaddingBackground
================================
*) getColor - new first FOUserAgent argument

8) NumberProperty
=================
*) getColor - new first FOUserAgent argument

9) Property
===========
*) getColor - new first FOUserAgent argument

10) ColorUtil has been changed to take "advantage" of these changes


I have not added a lot of tests to check the arguments which are accessed to
get hold of the FOUserAgent and the unit tests gave one NPE because one of
the arguments to one of the functions was null. I am trying to decide
whether more defensive checks for null arguments would make sense.

Anyone any thoughts? 

Thanks,

Peter



> -----Original Message-----
> From: Jeremias Maerki [mailto:[EMAIL PROTECTED]
> Sent: Wednesday, October 11, 2006 2:29 PM
> To: fop-dev@xmlgraphics.apache.org
> Subject: Re: Including an sRGB color profile?
> 
> 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