xmlgraphics-fop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bugzi...@apache.org
Subject DO NOT REPLY [Bug 46705] [PATCH] Enhancement: PDF Accessibility
Date Tue, 17 Feb 2009 11:00:29 GMT

--- Comment #7 from Vincent Hennebert <vhennebert@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

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


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

View raw message