You are viewing a plain text version of this content. The canonical link for it is here.
Posted to fop-dev@xmlgraphics.apache.org by Vincent Hennebert <vh...@gmail.com> on 2011/07/27 12:09:58 UTC
Re: svn commit: r1151195 - in /xmlgraphics/fop/trunk: src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java
status.xml
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
Re: svn commit: r1151195 - in /xmlgraphics/fop/trunk: src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java status.xml
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
Please note that we're not talking about the ASF license policy or even
open source. We're talking about commercial, closed source fonts for
which there are usually no redistribution rights (at least in the
standalone form)! Uploading them to a publicly accessible code
repository would mean illegal reproduction.
As for looking for a free font that shows the same problem: there's a
big chance that you're going to search for days and the search might
still turn out empty. The constellation is very peculiar:
There must be more than one character mapped to the the same glyph
description and that glyph description must be a composite glyph.
On 29.07.2011 17:54:14 Benson Margulies wrote:
> apache-extras is there for non-AL associated stuff.
>
> On Fri, Jul 29, 2011 at 6:45 AM, Vincent Hennebert <vh...@gmail.com> wrote:
<snip/>
> > The problem is, as explained above, we need to be able to test the font
> > library, and have the tests in a public place. Surely, among all the
> > commonly available free fonts, there must be one that shows the problem?
> >
> > And if the fonts licenses are incompatible with ASL2.0, maybe we can
> > set up a project on SourceForge, like for the hyphenation patterns?
> > Maybe OFFO itself would be a proper host for that?
Jeremias Maerki
Re: svn commit: r1151195 - in /xmlgraphics/fop/trunk: src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java
status.xml
Posted by Vincent Hennebert <vh...@gmail.com>.
On 29/07/11 16:54, Benson Margulies wrote:
> apache-extras is there for non-AL associated stuff.
Indeed, now I remember this announcement that was recently made about
Apache Extras. Seems like the perfect place. Thanks for the reminder,
Vincent
> On Fri, Jul 29, 2011 at 6:45 AM, Vincent Hennebert <vh...@gmail.com> wrote:
>> On 28/07/11 13:52, Jeremias Maerki wrote:
>>> On 28.07.2011 13:59:52 Vincent Hennebert wrote:
>>>> On 27/07/11 13:39, Jeremias Maerki wrote:
>>>>> On 27.07.2011 12:09:58 Vincent Hennebert wrote:
>>
>> <snip/>
>>
>>> I've done extensive test with various fonts.
>>
>> This is good to hear. The problem is that those tests are not publicly
>> available, so if anyone else makes any changes to the font library, they
>> won’t have the possibility to test them and avoid regressions. For this
>> reason I think it’s very important to have tests available to everyone.
>>
>>
>>>>> I've done that intentionally to indicate
>>>>> that the variable is only just used by the following method.
>>>>
>>>> By putting it at a non-expected place you’re making it difficult to find
>>>> the variable and understand in a quick glance what the class is made of.
>>>> This hampers the readability and maintainability of the code. Given that
>>>> it’s what we spend most of our time on, I find this worrying.
>>>
>>> See? And I did it that way exactly because I wanted to make this more
>>> readable and understandable. It's just hopeless to even try around you.
>>
>> This is great to know that you were trying to make it more readable. In
>> this case though, I think putting the variable declaration in the middle
>> of methods would do more harm than good, because of the very strongly
>> established convention of putting all variables at the beginning of the
>> class.
>>
>>
>>>> Your needing to put the variable near to the methods that use it is
>>>> a clear sign that this class is too big and needs to be split into
>>>> smaller entities.
>>>
>>> That's becoming a standard statement of yours.
>>
>> I’m puzzled when I read this because this is actually what is being
>> recommended in every book, and by every experienced OO developer. So why
>> not apply it?
>>
>> <snip/>
>>
>>>>> I'll swallow my comment to this and just do the split:
>>>>> http://svn.apache.org/viewvc?rev=1151447&view=rev
>>>>
>>>> When I read this and the sarcastic message associated to the commit, I’m
>>>> concerned about the unwelcoming atmosphere that is being created on this
>>>> mailing list. Can we try and remain civil to each other?
>>>
>>> It was meant to be sarcastic and an expression of my anger.
>>
>> I appreciate that you may be angry, how does that justify an aggressive
>> tone though?
>>
>>> We two got
>>> along in the last few months because we apparently went out of each
>>> other's way. But that only hides the underlying problem. I cannot turn
>>> myself magically into the person that can always forsee how you want
>>> something done. And I'm getting really tired of having the same
>>> arguments over and over. The only way I can react to this is to retreat
>>> again. Which is probably what I'll be doing after finishing some of the
>>> things I promised to a number of people.
>>>
>>>>
>>>>>> 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.
>>>>
>>>> We have the DejaVuLGCSerif font in our tests/resources/fonts directory.
>>>> Surely it must be possible to reproduce the issue with that font. Did
>>>> you have a look at it?
>>>
>>> I'm afraid, both DejaVuLGCSerif and glb12 don't have that particular
>>> constellation.
>>
>> The problem is, as explained above, we need to be able to test the font
>> library, and have the tests in a public place. Surely, among all the
>> commonly available free fonts, there must be one that shows the problem?
>>
>> And if the fonts’ licenses are incompatible with ASL2.0, maybe we can
>> set up a project on SourceForge, like for the hyphenation patterns?
>> Maybe OFFO itself would be a proper host for that?
>>
>>
>> Thanks,
>> Vincent
>>
Re: svn commit: r1151195 - in /xmlgraphics/fop/trunk:
src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java status.xml
Posted by Benson Margulies <bi...@gmail.com>.
apache-extras is there for non-AL associated stuff.
On Fri, Jul 29, 2011 at 6:45 AM, Vincent Hennebert <vh...@gmail.com> wrote:
> On 28/07/11 13:52, Jeremias Maerki wrote:
>> On 28.07.2011 13:59:52 Vincent Hennebert wrote:
>>> On 27/07/11 13:39, Jeremias Maerki wrote:
>>>> On 27.07.2011 12:09:58 Vincent Hennebert wrote:
>
> <snip/>
>
>> I've done extensive test with various fonts.
>
> This is good to hear. The problem is that those tests are not publicly
> available, so if anyone else makes any changes to the font library, they
> won’t have the possibility to test them and avoid regressions. For this
> reason I think it’s very important to have tests available to everyone.
>
>
>>>> I've done that intentionally to indicate
>>>> that the variable is only just used by the following method.
>>>
>>> By putting it at a non-expected place you’re making it difficult to find
>>> the variable and understand in a quick glance what the class is made of.
>>> This hampers the readability and maintainability of the code. Given that
>>> it’s what we spend most of our time on, I find this worrying.
>>
>> See? And I did it that way exactly because I wanted to make this more
>> readable and understandable. It's just hopeless to even try around you.
>
> This is great to know that you were trying to make it more readable. In
> this case though, I think putting the variable declaration in the middle
> of methods would do more harm than good, because of the very strongly
> established convention of putting all variables at the beginning of the
> class.
>
>
>>> Your needing to put the variable near to the methods that use it is
>>> a clear sign that this class is too big and needs to be split into
>>> smaller entities.
>>
>> That's becoming a standard statement of yours.
>
> I’m puzzled when I read this because this is actually what is being
> recommended in every book, and by every experienced OO developer. So why
> not apply it?
>
> <snip/>
>
>>>> I'll swallow my comment to this and just do the split:
>>>> http://svn.apache.org/viewvc?rev=1151447&view=rev
>>>
>>> When I read this and the sarcastic message associated to the commit, I’m
>>> concerned about the unwelcoming atmosphere that is being created on this
>>> mailing list. Can we try and remain civil to each other?
>>
>> It was meant to be sarcastic and an expression of my anger.
>
> I appreciate that you may be angry, how does that justify an aggressive
> tone though?
>
>> We two got
>> along in the last few months because we apparently went out of each
>> other's way. But that only hides the underlying problem. I cannot turn
>> myself magically into the person that can always forsee how you want
>> something done. And I'm getting really tired of having the same
>> arguments over and over. The only way I can react to this is to retreat
>> again. Which is probably what I'll be doing after finishing some of the
>> things I promised to a number of people.
>>
>>>
>>>>> 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.
>>>
>>> We have the DejaVuLGCSerif font in our tests/resources/fonts directory.
>>> Surely it must be possible to reproduce the issue with that font. Did
>>> you have a look at it?
>>
>> I'm afraid, both DejaVuLGCSerif and glb12 don't have that particular
>> constellation.
>
> The problem is, as explained above, we need to be able to test the font
> library, and have the tests in a public place. Surely, among all the
> commonly available free fonts, there must be one that shows the problem?
>
> And if the fonts’ licenses are incompatible with ASL2.0, maybe we can
> set up a project on SourceForge, like for the hyphenation patterns?
> Maybe OFFO itself would be a proper host for that?
>
>
> Thanks,
> Vincent
>
Re: svn commit: r1151195 - in /xmlgraphics/fop/trunk: src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java
status.xml
Posted by Vincent Hennebert <vh...@gmail.com>.
On 28/07/11 13:52, Jeremias Maerki wrote:
> On 28.07.2011 13:59:52 Vincent Hennebert wrote:
>> On 27/07/11 13:39, Jeremias Maerki wrote:
>>> On 27.07.2011 12:09:58 Vincent Hennebert wrote:
<snip/>
> I've done extensive test with various fonts.
This is good to hear. The problem is that those tests are not publicly
available, so if anyone else makes any changes to the font library, they
won’t have the possibility to test them and avoid regressions. For this
reason I think it’s very important to have tests available to everyone.
>>> I've done that intentionally to indicate
>>> that the variable is only just used by the following method.
>>
>> By putting it at a non-expected place you’re making it difficult to find
>> the variable and understand in a quick glance what the class is made of.
>> This hampers the readability and maintainability of the code. Given that
>> it’s what we spend most of our time on, I find this worrying.
>
> See? And I did it that way exactly because I wanted to make this more
> readable and understandable. It's just hopeless to even try around you.
This is great to know that you were trying to make it more readable. In
this case though, I think putting the variable declaration in the middle
of methods would do more harm than good, because of the very strongly
established convention of putting all variables at the beginning of the
class.
>> Your needing to put the variable near to the methods that use it is
>> a clear sign that this class is too big and needs to be split into
>> smaller entities.
>
> That's becoming a standard statement of yours.
I’m puzzled when I read this because this is actually what is being
recommended in every book, and by every experienced OO developer. So why
not apply it?
<snip/>
>>> I'll swallow my comment to this and just do the split:
>>> http://svn.apache.org/viewvc?rev=1151447&view=rev
>>
>> When I read this and the sarcastic message associated to the commit, I’m
>> concerned about the unwelcoming atmosphere that is being created on this
>> mailing list. Can we try and remain civil to each other?
>
> It was meant to be sarcastic and an expression of my anger.
I appreciate that you may be angry, how does that justify an aggressive
tone though?
> We two got
> along in the last few months because we apparently went out of each
> other's way. But that only hides the underlying problem. I cannot turn
> myself magically into the person that can always forsee how you want
> something done. And I'm getting really tired of having the same
> arguments over and over. The only way I can react to this is to retreat
> again. Which is probably what I'll be doing after finishing some of the
> things I promised to a number of people.
>
>>
>>>> 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.
>>
>> We have the DejaVuLGCSerif font in our tests/resources/fonts directory.
>> Surely it must be possible to reproduce the issue with that font. Did
>> you have a look at it?
>
> I'm afraid, both DejaVuLGCSerif and glb12 don't have that particular
> constellation.
The problem is, as explained above, we need to be able to test the font
library, and have the tests in a public place. Surely, among all the
commonly available free fonts, there must be one that shows the problem?
And if the fonts’ licenses are incompatible with ASL2.0, maybe we can
set up a project on SourceForge, like for the hyphenation patterns?
Maybe OFFO itself would be a proper host for that?
Thanks,
Vincent
Re: svn commit: r1151195 - in /xmlgraphics/fop/trunk: src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java status.xml
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
On 28.07.2011 13:59:52 Vincent Hennebert wrote:
> On 27/07/11 13:39, Jeremias Maerki wrote:
> > 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 actually find it very worrying that you consider this to be
> nitpicking, when any decent book about software programming will
> emphasize the importance of producing code that is as clear and readable
> as possible. I think its urgent to improve the readability of our code
> base if we want to attract more contributors. Could we commit to that?
You know, I've tried VERY hard to do the change in a way so I hope you
would agree with it. It is clear to me by now that it is extremely hard
for anyone to please your expectations. Try to do it one way, you want
it another.
> Its great that you java 1.5-ified parts of the code in that commit (has
> it been tested though?), and it would be good if the other changes were
> bringing the same improved clarity.
I've done extensive test with various fonts.
>
> > I've done that intentionally to indicate
> > that the variable is only just used by the following method.
>
> By putting it at a non-expected place youre making it difficult to find
> the variable and understand in a quick glance what the class is made of.
> This hampers the readability and maintainability of the code. Given that
> its what we spend most of our time on, I find this worrying.
See? And I did it that way exactly because I wanted to make this more
readable and understandable. It's just hopeless to even try around you.
> Your needing to put the variable near to the methods that use it is
> a clear sign that this class is too big and needs to be split into
> smaller entities.
That's becoming a standard statement of yours. Not that this approach always
accomplishes the desired result.
>
> > And the
> > null is only there to emphasize that the variable is lazily assigned
> > because the thing is often not even needed.
>
> This is an interesting convention, although I believe it is cancelled out
> by the fact that in a vast majority of cases, the initialization is
> there just out of ignorance of Javas default initialization. But that
> doesnt matter too much.
>
>
> <snip/>
> >>> + 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
>
> When I read this and the sarcastic message associated to the commit, Im
> concerned about the unwelcoming atmosphere that is being created on this
> mailing list. Can we try and remain civil to each other?
It was meant to be sarcastic and an expression of my anger. We two got
along in the last few months because we apparently went out of each
other's way. But that only hides the underlying problem. I cannot turn
myself magically into the person that can always forsee how you want
something done. And I'm getting really tired of having the same
arguments over and over. The only way I can react to this is to retreat
again. Which is probably what I'll be doing after finishing some of the
things I promised to a number of people.
>
> >> 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.
>
> We have the DejaVuLGCSerif font in our tests/resources/fonts directory.
> Surely it must be possible to reproduce the issue with that font. Did
> you have a look at it?
I'm afraid, both DejaVuLGCSerif and glb12 don't have that particular
constellation.
>
> Thanks,
> Vincent
Jeremias Maerki
AW: svn commit: r1151195 - in /xmlgraphics/fop/trunk:
src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java status.xml
Posted by Georg Datterl <ge...@geneon.de>.
Popcorn. Coke. Larks' tongues. Wrens' livers. Chaffinch brains. Jaguars' earlobes. Wolf nipple chips. Get 'em while they're hot. They're lovely. Dromedary pretzels, only half a denar. Tuscany fried bats...
Come back later for fresh stones!
Regards, Georg
------------------------------------
See below
Peter West
"How can these things be?"
On 28/07/2011, at 9:59 PM, Vincent Hennebert wrote:
> On 27/07/11 13:39, Jeremias Maerki wrote:
>> On 27.07.2011 12:09:58 Vincent Hennebert wrote:
>>
>> So we're back to nitpicking.
>
Oh, absolutely not!
> I actually find it very worrying that you consider this to be
> nitpicking, when any decent book about software programming will
... etc. etc.
>
>
>> I've done that intentionally to indicate
>> that the variable is only just used by the following method.
>
> By putting it at a non-expected place you're making it difficult to find
> the variable and understand in a quick glance what the class is made of.
> This hampers the readability and maintainability of the code. Given that
> it's what we spend most of our time on, I find this worrying.
That is, by assisting the understanding of the use of this variable, you have made it harder to "understand in a quick glance." But in any case...
>
> Your needing to put the variable near to the methods that use it is
> a clear sign that this class is too big and needs to be split into
> smaller entities.
>
>
>> And the
>> null is only there to emphasize that the variable is lazily assigned
>> because the thing is often not even needed.
>
> This is an interesting convention, although I believe it is cancelled out
> by the fact that in a vast majority of cases, the initialization is
> there just out of ignorance of Java's default initialization. But that
> doesn't matter too much.
That is, you're a dope who doesn't understand Java, unlike some. Come clean, Jeremias.
>>> 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
>
> When I read this and the sarcastic message associated to the commit, I'm
> concerned about the unwelcoming atmosphere that is being created on this
> mailing list. Can we try and remain civil to each other?
>
Vincent is concerned that YOU are creating an unwelcome atmosphere on this list. Jeremias, when will you learn?
>
>>> 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.
>
> We have the DejaVuLGCSerif font in our tests/resources/fonts directory.
> Surely it must be possible to reproduce the issue with that font. Did
> you have a look at it?
Well, DID you? Eh? Eh?
>
>
> Thanks,
> Vincent
Re: svn commit: r1151195 - in /xmlgraphics/fop/trunk:
src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java status.xml
Posted by mehdi houshmand <me...@gmail.com>.
Hi Guys,
I was tasked with fixing this issue, and spent some time cleaning up
and testing this area of code. I really don't want to get in the
middle of this but if any one has any questions or wishes to make
amendments, feel free to do so, or suggest improvements and I'll amend
the patch.
https://issues.apache.org/bugzilla/show_bug.cgi?id=51596
Mehdi
On 31 July 2011 10:23, Andreas L. Delmelle <an...@telenet.be> wrote:
> On 28 Jul 2011, at 14:33, Peter B. West wrote:
>
>> On 28/07/2011, at 9:59 PM, Vincent Hennebert wrote:
>>
>>> By putting it at a non-expected place you’re making it difficult to find
>>> the variable and understand in a quick glance what the class is made of.
>>> This hampers the readability and maintainability of the code. Given that
>>> it’s what we spend most of our time on, I find this worrying.
>>
>> That is, by assisting the understanding of the use of this variable, you have made it harder to "understand in a quick glance." But in any case...
>
> That is: "By putting it at place where I did not expect it, you are making it difficult for normal people, like me, to find it --because, well, it is then located near to where it is actually used-- and understand in a quick glance [...] I find this worrying --and did not care much for _your_ intentions either way."
>
> And then, a bit later:
>
> On 29 Jul 2011, at 12:57, Vincent Hennebert wrote:
>
>> I don’t think I was attacking anyone. I realise that there is bad past
>> experience about this, but I don’t believe anything similar happened
>> here. I’m sorry if my message sounded confrontational.
>
> "I did not do anything wrong! Sorry if that was too confrontational. Can we play nice again? Can we? Please?"
>
>> I suppose I could write a whole chapter about how I am affected by
>> unreadable and unmaintainable code in FOP almost on a daily basis; About
>> the anger, the frustration, and sometimes the total despair that result
>> from this. But I’ll refrain to this paragraph.
>
> "I suppose I could write a whole chapter about how I am affected by the awful mess you old-timers have left me with. The voices, the nightmares, the hair-ripping... but I will remain civil, because I am perfect and never do anything wrong. No, really. It's true. Everyone who agrees with me, says so."
>
>
>
> Regards,
>
> Andreas
> ---
Re: svn commit: r1151195 - in /xmlgraphics/fop/trunk: src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java status.xml
Posted by "Andreas L. Delmelle" <an...@telenet.be>.
On 28 Jul 2011, at 14:33, Peter B. West wrote:
> On 28/07/2011, at 9:59 PM, Vincent Hennebert wrote:
>
>> By putting it at a non-expected place you’re making it difficult to find
>> the variable and understand in a quick glance what the class is made of.
>> This hampers the readability and maintainability of the code. Given that
>> it’s what we spend most of our time on, I find this worrying.
>
> That is, by assisting the understanding of the use of this variable, you have made it harder to "understand in a quick glance." But in any case...
That is: "By putting it at place where I did not expect it, you are making it difficult for normal people, like me, to find it --because, well, it is then located near to where it is actually used-- and understand in a quick glance [...] I find this worrying --and did not care much for _your_ intentions either way."
And then, a bit later:
On 29 Jul 2011, at 12:57, Vincent Hennebert wrote:
> I don’t think I was attacking anyone. I realise that there is bad past
> experience about this, but I don’t believe anything similar happened
> here. I’m sorry if my message sounded confrontational.
"I did not do anything wrong! Sorry if that was too confrontational. Can we play nice again? Can we? Please?"
> I suppose I could write a whole chapter about how I am affected by
> unreadable and unmaintainable code in FOP almost on a daily basis; About
> the anger, the frustration, and sometimes the total despair that result
> from this. But I’ll refrain to this paragraph.
"I suppose I could write a whole chapter about how I am affected by the awful mess you old-timers have left me with. The voices, the nightmares, the hair-ripping... but I will remain civil, because I am perfect and never do anything wrong. No, really. It's true. Everyone who agrees with me, says so."
Regards,
Andreas
---
Re: svn commit: r1151195 - in /xmlgraphics/fop/trunk: src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java status.xml
Posted by "Peter B. West" <pb...@pbw.id.au>.
See below
Peter West
"How can these things be?"
On 28/07/2011, at 9:59 PM, Vincent Hennebert wrote:
> On 27/07/11 13:39, Jeremias Maerki wrote:
>> On 27.07.2011 12:09:58 Vincent Hennebert wrote:
>>
>> So we're back to nitpicking.
>
Oh, absolutely not!
> I actually find it very worrying that you consider this to be
> nitpicking, when any decent book about software programming will
... etc. etc.
>
>
>> I've done that intentionally to indicate
>> that the variable is only just used by the following method.
>
> By putting it at a non-expected place you’re making it difficult to find
> the variable and understand in a quick glance what the class is made of.
> This hampers the readability and maintainability of the code. Given that
> it’s what we spend most of our time on, I find this worrying.
That is, by assisting the understanding of the use of this variable, you have made it harder to "understand in a quick glance." But in any case...
>
> Your needing to put the variable near to the methods that use it is
> a clear sign that this class is too big and needs to be split into
> smaller entities.
>
>
>> And the
>> null is only there to emphasize that the variable is lazily assigned
>> because the thing is often not even needed.
>
> This is an interesting convention, although I believe it is cancelled out
> by the fact that in a vast majority of cases, the initialization is
> there just out of ignorance of Java’s default initialization. But that
> doesn’t matter too much.
That is, you're a dope who doesn't understand Java, unlike some. Come clean, Jeremias.
>>> 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
>
> When I read this and the sarcastic message associated to the commit, I’m
> concerned about the unwelcoming atmosphere that is being created on this
> mailing list. Can we try and remain civil to each other?
>
Vincent is concerned that YOU are creating an unwelcome atmosphere on this list. Jeremias, when will you learn?
>
>>> 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.
>
> We have the DejaVuLGCSerif font in our tests/resources/fonts directory.
> Surely it must be possible to reproduce the issue with that font. Did
> you have a look at it?
Well, DID you? Eh? Eh?
>
>
> Thanks,
> Vincent
Re: Stop this infighting [was: Re: svn commit: r1151195 - in /xmlgraphics/fop/trunk:
src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java status.xml]
Posted by Vincent Hennebert <vh...@gmail.com>.
On 28/07/11 13:47, Simon Pepping wrote:
> Stop this infighting, please. We all have our strong and weak points.
> Stop attacking each other on real or perceived weak points. Cooperate
> with each other and complement each other in a positive atmosphere.
I don’t think I was attacking anyone. I realise that there is bad past
experience about this, but I don’t believe anything similar happened
here. I’m sorry if my message sounded confrontational.
If we were sitting next to each other and reviewing each other code
before committing, we would have this very kind of discussion and it
would be totally mundane. I am actually doing that almost every day with
my colleagues and it proves to be very productive and greatly enhances
the quality of the code.
The fact that we are working remotely unfortunately doesn’t allow to do
that, so we have to resort to a less-than-ideal medium which is email.
Shall we try and make the best out of it?
As a project member, one of my responsibilities is to ensure the good
health of the project. And when I see the lack of readability of its
codebase, I am extremely concerned about its viability. The experience
I have accumulated in the past few years has only confirmed my thoughts
and concerns. So I’m trying to do something to change that.
I suppose I could write a whole chapter about how I am affected by
unreadable and unmaintainable code in FOP almost on a daily basis; About
the anger, the frustration, and sometimes the total despair that result
from this. But I’ll refrain to this paragraph.
Thanks,
Vincent
> Simon
>
> On Thu, Jul 28, 2011 at 12:59:52PM +0100, Vincent Hennebert wrote:
>> On 27/07/11 13:39, Jeremias Maerki wrote:
>>> 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
>>>>>
Stop this infighting [was: Re: svn commit: r1151195 - in
/xmlgraphics/fop/trunk:
src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java status.xml]
Posted by Simon Pepping <sp...@leverkruid.eu>.
Stop this infighting, please. We all have our strong and weak points.
Stop attacking each other on real or perceived weak points. Cooperate
with each other and complement each other in a positive atmosphere.
Simon
On Thu, Jul 28, 2011 at 12:59:52PM +0100, Vincent Hennebert wrote:
> On 27/07/11 13:39, Jeremias Maerki wrote:
> > 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
> >>>
Re: svn commit: r1151195 - in /xmlgraphics/fop/trunk: src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java
status.xml
Posted by Vincent Hennebert <vh...@gmail.com>.
On 27/07/11 13:39, Jeremias Maerki wrote:
> 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, there’s no need to initialize it to null as it’s
>> already done by default.)
>
> So we're back to nitpicking.
I actually find it very worrying that you consider this to be
nitpicking, when any decent book about software programming will
emphasize the importance of producing code that is as clear and readable
as possible. I think it’s urgent to improve the readability of our code
base if we want to attract more contributors. Could we commit to that?
It’s great that you java 1.5-ified parts of the code in that commit (has
it been tested though?), and it would be good if the other changes were
bringing the same improved clarity.
> I've done that intentionally to indicate
> that the variable is only just used by the following method.
By putting it at a non-expected place you’re making it difficult to find
the variable and understand in a quick glance what the class is made of.
This hampers the readability and maintainability of the code. Given that
it’s what we spend most of our time on, I find this worrying.
Your needing to put the variable near to the methods that use it is
a clear sign that this class is too big and needs to be split into
smaller entities.
> And the
> null is only there to emphasize that the variable is lazily assigned
> because the thing is often not even needed.
This is an interesting convention, although I believe it is cancelled out
by the fact that in a vast majority of cases, the initialization is
there just out of ignorance of Java’s default initialization. But that
doesn’t matter too much.
<snip/>
>>> + 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.
>
> I'll swallow my comment to this and just do the split:
> http://svn.apache.org/viewvc?rev=1151447&view=rev
When I read this and the sarcastic message associated to the commit, I’m
concerned about the unwelcoming atmosphere that is being created on this
mailing list. Can we try and remain civil to each other?
>> 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.
We have the DejaVuLGCSerif font in our tests/resources/fonts directory.
Surely it must be possible to reproduce the issue with that font. Did
you have a look at it?
Thanks,
Vincent
Re: svn commit: r1151195 - in /xmlgraphics/fop/trunk: src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java status.xml
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
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