Summary: | [PATCH] Factoring out of color map cache and uri/fo resolution from FopFactory | ||
---|---|---|---|
Product: | Fop - Now in Jira | Reporter: | Adrian Cumiskey <fop-dev> |
Component: | general | Assignee: | fop-dev |
Status: | CLOSED FIXED | ||
Severity: | normal | ||
Priority: | P4 | ||
Version: | trunk | ||
Target Milestone: | --- | ||
Hardware: | Other | ||
OS: | other | ||
Attachments: |
small patch file
small patch file small patch file |
Description
Adrian Cumiskey
2007-04-27 07:50:31 UTC
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 |