Just a few amendments:
- File.createTempFile(...) failed the unit tests and I didn't have time to
investigate why, this could be done in the future, though I'm not sure the
pros/cons of doing so.
- The method signature change getBaseURI -> getBaseDirectoryURI wasn't
done. After speaking to PH, it didn't seem appropriate to refer to it as a
directory since it could be anything. The javadoc clearly describes the
behaviour of the method, I think this is sufficient for now.
On 4 July 2012 08:38, mehdi houshmand <med1...@gmail.com> wrote:
>
>
> 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.
>> >>
>> >
>>
>>
>
