Without seeing the code, no objections here. It's good to be on the safe
side by using a synchronized Map. I'll look into the "base" parameter
for getColorSpace() when I see the code.

When you submit the first patch, please don't expect a fast turn-around,
since right now I can't tell when I will be able to process it. I'm
currently struggling to juggle my items on my task list and all the
other FOP stuff (like this) in the little time I have available for FOP
nowadays.

On 11.10.2006 17:35:34 Peter wrote:
> 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



Jeremias Maerki

Reply via email to