Hi Alexios, Thanks for taking a look at this.
On Thu, Jul 5, 2012 at 1:41 PM, Alexios Giotis <alex.gio...@gmail.com> wrote: > Hi, > > I had a quick look on the work you are doing and I think that the newly > introduced methods > FopFactory.getRendererConfig(getRendererConfig(FOUserAgent userAgent, > Configuration cfg, > RendererConfigParser configCreator)) > and the similar ones on FOUserAgent should all be replaced with a single > > FopFactory.getRendererConfig(String mimeType). > > 1. Design reason: This is a public API that uses too many internal objects. > RendererConfigParser is not part of the public API [1] and I find no reason > to add. If this does not change, then it becomes automatically part of the > public API (it's a public method of the public FopFactory). > > [1] http://xmlgraphics.apache.org/fop/trunk/embedding.html#API > Good point - a quick fix is to make the method can be made package private. With a bit more work I agree that the method signature could be simplified: The Configuration object is resolvable from the mime type, as you suggest, however calls to getRendererConfig(String mimeType, RendererConfigParser configCreator) can be made whereby configCreator.getMimeType() differs from mimeType (see BitmapRendererConfigurator). Ideally we should fix the configuration hierarchy however this was out of scope for our initial implementation. Removing the FOUserAgent parameter and calling RendererConfigParser.build() with the FopFactory makes sense, however the issue of the Event Broadcaster crops up - see below... > 2. Buggy: FopFactory is supposed to be reusable and a new FOUserAgent should > be created for every execution since it holds some rendering specific > information. But the existing FopFactory.getRendererConfig() uses the > rendering specific FOUserAgent to cache in (Map<String, RendererConfig> > rendererConfig) a RendererConfig. The cached object will be used by a future > FOUserAgent with possibly different user properties. Sounds scary to me. What > if the configuration of a renderer uses a FOUserAgent specific property ? > (the properties of the first FOUserAgent will be used). > > Looking a little deeper, the renderer config implementations are currently > using firstly the FOUserAgent to get access to FopFactory (e.g. to check the > strict validation flag), & secondly the event broadcaster. The first tells me > that they actually need to get the "immutable" FopFactory instead of the > FOUserAgent. The second (usage of the event broadcaster) makes my fears come > true. Scenario: > > During the first rendering, a default FOUserAgent is used. Assuming the > configuration is problematic, the default event handlers will log a message > and continue. During the next rendering, an event broadcaster that aborts > processing on any error is registered. The user would expect to stop on any > error, but the process will simply continue. > I see a few options. I am in favor of the simpler one which is to throw an > exception instead of broadcasting an event when an error in found in the > renderer config. A second one is to add yet another flag on FopFactory that > will determine this behavior. Another one is to register event handlers > related to renderer config on FopFactory. It's flexible but a thread safe > implementation of the handler is required. Well spotted! We considered this problem and wondered whether we should store the events and republish them upon subsequent calls to FopFactory.getRendererConfig. I think this is something to consider, along with your suggestions, if it turns out to be a practical requirement. > 3. Buggy: FopFactory getRendererConfig() is not thread safe. Wrap the method > body in a synchronized (rendererConfig) {...} Well spotted! Thanks, Peter
