On 3 July 2012 15:48, Vincent Hennebert <vhenneb...@gmail.com> wrote:
> I had started to build up a list of questions and comments but didn’t
> get round to finishing it in time. Anyway, here it is, hopefully it will
> still provide valuable feedback. The most important items to address are
> the possible regression regarding strict FO validation and the public
> API.
>
> The new classes FopConfParser, FopFactoryBuilder and FopFactoryConfig
> should be renamed into FOPConfParser, FOPFactoryBuilder and
> FOPFactoryConfig. The fact that some existing public classes have Fop in
> lower case in their names was a mistake that was discovered after the
> API was frozen. Let’s not do that mistake again.
>
I don't agree, I think this makes the classes more inconsistent rather than
less so. I don't particularly have an opinion whether they should be Fop*
or FOP* but I think either way, there should be consistency
>
> There are quite a few typos in the javadocs of the new class, it would
> be good to fix them.
>
> src/java/org/apache/fop/apps/EnvironmentProfile.java
> • class javadoc: “The environment profile represents the restrictions
> and allowances that FOP is”... what?
>
Done.
>
> src/java/org/apache/fop/apps/EnvironmentalProfileFactory.java
> • should be renamed into EnvironmentProfileFactory
> • in the inner Profile class: there are checks that constructor
> parameters are not null, except for FontManager. So may that be null
> then? Doesn’t look like.
>
Done.
>
> src/java/org/apache/fop/apps/FOUserAgent.java
> • left-over TODO in resolveURI: should that be left as-is for now?
>
Yes. There are XGC classes that require this, there's already a TODO there
suggesting that we intend on removing this.
• getRendererConfig(String mimeType, RendererConfigParser configCreator)
> isn’t there a risk of mismatch between the given mimeType and
> configCreator? Can’t the mimeType be taken from the
> RendererConfigParser.getMimeType method?
>
No. The same parser can be used for more than one MIME type and we don't
want the config data to be retrieved from the wrong MIME type.
> • getRendererConfiguration(String mimeType)
> This comment:
> // silently pass over configurations without mime type
> Really? Don’t we want to at least display a warning?
>
This code is copy/pasted from elsewhere, we could have fixed this, but we'd
also have to test it. Time constraints didn't allow such luxuries.
> • public ColorSpaceCache getColorSpaceCache() {
> /** TODO: javadoc*/
>
Done
> • getHyphPatNames()
> this is hard to pronounce; also ‘Pat’ can easily be mistaken for
> ‘Path’
> s/getHyphPatNames/getHyphenationPatternNames/?
>
Luckily it's written so you don't have to pronounce it ;). No seriously,
done.
>
> src/java/org/apache/fop/apps/FopConfParser.java
> • there’s a mistake in the handling of ‘strict-validation’: this is
> related to checking the conformance of XSL-FO documents against the
> spec. The setting to strictly check the validity of config files is
> ‘strict-configuration’
>
Again, there's a TODO, I don't think this is a regression but just
something that was confused previously, though again, time constraints...
>
> src/java/org/apache/fop/apps/FopFactoryBuilder.java
> • the buildConfig method is deprecated but is used by the build method
> underneath, which is itself not deprecated. But then it shouldn’t rely
> on a deprecated method?
>
The javadoc makes it clear that the only reason this is there is to
maintain backwards compatibility, we'll remove it at some point in the
future when users are more comfortable with the new API design.
>
> src/java/org/apache/fop/apps/FopFactoryConfig.java
> • This is an interface whose methods’ javadocs refer to the FopFactory
> concrete class. So an abstraction depends on a concrete
> implementation. Surely that should be the other way around?
>
Again, this is to make the transition easier for users. Users are used to
accessing these members via the FopFactory and the javadocs help lubricate
the transition.
>
> src/java/org/apache/fop/apps/io/InternalResourceResolver
> • what does ‘Internal’ stand for?
>
Stand for? It doesn't stand for anything. This class is FOPs internal
resource resolver that does some primitive URI validation when strings are
given and holds the base URI when relative URIs are given.
> • getBaseURI: I don’t think adding a slash at the end of the URI if it’s
> not there is desirable, because that effectively changes the base URI.
> That may lead to very confusing errors.
>
As the javadoc suggests this is for giving you a directory base URI, I've
changed the method signature to {getBaseURI, getBaseDirectoryURI}.
>
> src/java/org/apache/fop/apps/io/TempResourceResolver
> • unless I’m mistaken there’s no way of releasing the temporary resource
> once it’s no longer needed. The DefaultTempResourceResolver
> implementation deleteOnExits the file but this may not be enough if
> FOP is run on a server that is meant to (almost) never shut down. Or
> did I miss something?
>
You missed something; this is the default implementation. This is pretty
much only for the CLI use-case, if users wish to define their own
implementation, all they have to do is implement the interface.
> • in DefaultTempResourceResolver.getTempFile: the File.createTempFile
> method should be used instead of querying the ‘java.io.tmpdir’
> property.
>
Done.
>
> src/java/org/apache/fop/fonts/FontConfig.java
> • all this interface declares is an inner FontConfigParser interface.
> Why not simply promote FontConfigParser as a top-level interface and
> remove FontConfig?
>
The intention was to provide an interface that covers both use cases, AFP
and everything else, so that we could make the font configuration process a
bit more OOP and a bit less "if (something) do something". Time constraints
howerver...
>
> src/java/org/apache/fop/fonts/FontConfigurator.java
> • some documentation about the T generic parameter would be good. Also,
> shouldn’t this parameter be bounded by some interface?
>
See the reply above. Again, there's a TODO suggesting just that, again,
time constraints...
>
> Thanks,
> Vincent
>
>
> On 03/07/12 10:35, mehdi houshmand wrote:
> > Thanks guys, that's 5 x +1 and no one in opposition. I'll merge in the
> > branch imminently and update the docs as appropriate
> >
> > On 2 July 2012 14:40, Glenn Adams <gl...@skynav.com> wrote:
> >
> >>
> >> On Mon, Jul 2, 2012 at 7:25 AM, mehdi houshmand <med1...@gmail.com>
> wrote:
> >>
> >>> Ahh I see, ok, thanks Pascal. I think we got our wires crossed a little
> >>> there ;)
> >>>
> >>> Glenn, my plan was to merge the URI-resolution stuff in tomorrow. If
> this
> >>> burdens you in some way, could you let me know how and maybe we can
> come to
> >>> some resolution? As far as I can see, it shouldn't affect the 1.1rc1
> branch
> >>> at all.
> >>>
> >>
> >> Go ahead. I'm going to rename the fop_1.1rc1 branch to fop_1.1 and then
> >> create a fop_1.1rc1 tag on that branch that corresponds with the current
> >> rc1 rev.
> >>
> >
>
>
