On 27.07.2011 12:09:58 Vincent Hennebert wrote:
> There is basic housekeeping that ought to be done IMO:
>
> On 26/07/11 19:28, jeremias wrote:
> > Author: jeremias
> > Date: Tue Jul 26 18:28:07 2011
> > New Revision: 1151195
> >
> > URL: http://svn.apache.org/viewvc?rev=1151195&view=rev
> > Log:
> > Fixed a bug in TTF subsetting where a composite glyph could get remapped
> > more than once resulting in garbled character.
> >
> > Modified:
> >
> > xmlgraphics/fop/trunk/src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java
> > xmlgraphics/fop/trunk/status.xml
> >
> <snip/>
> > /**
> > + * We need to remember which composites were already remapped because
> > the value to be
> > + * remapped is being read from the TTF file and being replaced right
> > there. Doing this
> > + * twice would create a bad map the second time.
> > + */
> > + private Set<Long> remappedComposites = null;
>
> This should be put at the beginning of the file, along with all field
> declarations. (Also, theres no need to initialize it to null as its
> already done by default.)
So we're back to nitpicking. I've done that intentionally to indicate
that the variable is only just used by the following method. And the
null is only there to emphasize that the variable is lazily assigned
because the thing is often not even needed.
>
> > +
> > + /**
> > * Rewrite all compositepointers in glyphindex glyphIdx
> > - *
> > */
> > - private void remapComposite(FontFileReader in, Map glyphs,
> > - int glyphOffset,
> > + private void remapComposite(FontFileReader in, Map<Integer, Integer>
> > glyphs,
> > + long glyphOffset,
> > Integer glyphIdx) throws IOException {
> > - int offset = glyphOffset +
> > (int)mtxTab[glyphIdx.intValue()].getOffset()
> > - + 10;
> > + if (remappedComposites == null) {
> > + remappedComposites = new java.util.HashSet<Long>();
> > + }
> > + TTFMtxEntry mtxEntry = mtxTab[glyphIdx.intValue()];
> > + long offset = glyphOffset + mtxEntry.getOffset() + 10;
> > + if (remappedComposites.contains(offset)) {
> > + return;
>
> This return introduces another exit point that is hidden at the
> beginning of the method. This is something that one wouldnt expect and
> makes the method hard to understand and maintain. This method should
> most probably be split into smaller methods.
I'll swallow my comment to this and just do the split:
http://svn.apache.org/viewvc?rev=1151447&view=rev
> But more importantly, there is no unit test that comes with this commit.
> So there is no reason to believe that the problem is fixed and, most of
> all, will not happen again in the future. Can you please add a unit test
> for this?
No, I cannot. For licensing reasons. I can't upload the font that's
causing this into the Apache SVN repository. I'd have to artificially
construct a font that emulates this and I certainly won't try to do that.
Jeremias Maerki
