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, there’s no need to initialize it to null as it’s
already done by default.)
> +
> + /**
> * 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 wouldn’t expect and
makes the method hard to understand and maintain. This method should
most probably be split into smaller methods.
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?
Thanks,
Vincent
