https://issues.apache.org/bugzilla/show_bug.cgi?id=46705





--- Comment #7 from Vincent Hennebert <vhenneb...@gmail.com>  2009-02-17 
03:00:27 PST ---
That's great new functionality. I've only had time to look at a part of the
patch so far, so I'm posting my few comments below. More later if I have time.

In the o.a.f.accessibility package:
- why are there two classes? Only the sub-class seems to be externally used, so
it may as well be merged with the super-class. If some more general
functionality turns out to be necessary, the extraction of a common super-class
can always be done later.
- Likewise, only one constructor seems to be used ATM. If other constructors
are needed, they can also be added later on.
- AFAIU mTranHandler is set in all of the constructors. Why systematically test
it for null in the methods then?

There is an encapsulation problem in o.a.f.apps.Fop.getDefaultHandler(): the
lines of code dealing with accessibility should be moved to the accessibility
package.

In FOUserAgent:
- the "accessibility" string should be defined only once in a public final
static field (ideally somewhere inside the accessibility package)
- there's no need to explicitly put false for the accessibility option in the
constructor, as the value is tested for null in the accessibilityEnabled method

In FopFactoryConfigurator.configure():
- any reason why the accessibility option is not handled like the other
options? (using getValueAsBoolean)

And a nit about coding style: you seem to be using Hungarian notation to name
fields ('mTheClassField'). I don't have anything against this notation, but
it's not used inside the rest of FOP's codebase. For consistency we may want to
agree upon a notation. FWIW I tend to think that, thanks to modern development
environments that now highlight class members, this notation is no longer so
useful.

Thanks,
Vincent


-- 
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

Reply via email to