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
