Small change that should be easy to apply. Thought I would provide this is a smaller more manageable patch prior to more postscript renderer functionality being added in a forthcoming patch. Here I have simply factored out the color map cache and fo/uri resolution from FopFactory. These objects can now be used independently without reference to the FopFactory. Adrian Cumiskey.
Created attachment 20061 [details] small patch file
Created attachment 20062 [details] small patch file Missed a comment header on ColorSpaceCache.
Thanks for yet another patch, Adrian. Sorry it takes so long to process them. I've got three comments on this one: 1. The HyphenationTreeResolver seems to me like a foreign body in the FOPResolver class. IMO, it's abuses that class as carrier. Can't you hold that reference in a different place for your PostScript stuff and keep the reference in the FopFactory? 2. You're naming the class with the resolution code from FopFactory "FOPResolver". Now, with the existing "FOURIResolver" in the same package, I get the impression that there will be questions like: What is what? What's the difference between FOURIResolver and FOPResolver? What do you think about merging FOPResolver into FOURIResolver? I think this would be a safe thing to do. 3. I'm very much against introducing singletons for FopResolver, ElementMappingRegistry and ColorSpaceCache. Getting rid of all the singletons was one of my design goals for the new FOP API. The FopFactory should be reused whenever possible by the user (you could have a singleton for FopFactory in your app, though), but I want to create FopFactories which are completely separate from each other. The ColorSpaceCache is probably a border case since it doesn't make much sense to keep multiple instance of this class, but I wouldn't want singletons for the other two because of unwanted side-effects (configuring one FOP environment (i.e. FopFactory) shouldn't modify the other).
Hi Jeremias, (In reply to comment #3) > Thanks for yet another patch, Adrian. Sorry it takes so long to process them. Yes, the patch queue is getting pretty long these days. Its a real shame that all the good work that people are contributing on the patch queue is not making its way into the trunk more quickly. > > I've got three comments on this one: > 1. The HyphenationTreeResolver seems to me like a foreign body in the > FOPResolver class. IMO, it's abuses that class as carrier. Can't you hold that > reference in a different place for your PostScript stuff and keep the reference > in the FopFactory? The idea behind this FOPResolver refactoring work was to delegate all resolving actions away from FopFactory. It seemed to me that creating a FOPResolver class seemed like a good holding class for grouping all resolving related functionality together. The postscript changes would only make use of any color map cache change so there is no problem with the reference remaining rooted in the FopFactory. > 2. You're naming the class with the resolution code from FopFactory > "FOPResolver". Now, with the existing "FOURIResolver" in the same package, I get > the impression that there will be questions like: What is what? What's the > difference between FOURIResolver and FOPResolver? What do you think about > merging FOPResolver into FOURIResolver? I think this would be a safe thing to do. I think it is best to wait until PATCH http://issues.apache.org/bugzilla/show_bug.cgi?id=41831 has been applied before looking at merging FOPResolver and FOURIResolver. Patch 41831 contains a rewrite of the FOURIResolver resolve method. > 3. I'm very much against introducing singletons for FopResolver, > ElementMappingRegistry and ColorSpaceCache. Getting rid of all the singletons > was one of my design goals for the new FOP API. The FopFactory should be reused > whenever possible by the user (you could have a singleton for FopFactory in your > app, though), but I want to create FopFactories which are completely separate > from each other. The ColorSpaceCache is probably a border case since it doesn't > make much sense to keep multiple instance of this class, but I wouldn't want > singletons for the other two because of unwanted side-effects (configuring one > FOP environment (i.e. FopFactory) shouldn't modify the other). Thats a fair point on FopResolver and ElementMappingRegistry. I think its best to revisit this patch once 41831 has been committed. I really wish Andreas would return to look at it because he had spent quite a lot of time looking at this patch and was ready to commit it when he started having problems with his ISP. If another developer takes care of committing this rather large patch I can imagine quite a lot of effort being duplicated. All the best, Adrian.
Created attachment 20288 [details] small patch file Updated following feedback. * Removed singletons. * Merged uri resolution code into FOURIResolver.
Patch applied with modifications (see commit message): http://svn.apache.org/viewvc?view=rev&rev=551874 Thanks for the patch and your patience.
batch transition pre-FOP1.0 resolved+fixed bugs to closed+fixed