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 (MapString, 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
