Re: svn commit: r1151195 - in /xmlgraphics/fop/trunk: src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java status.xml

2011-07-28 Thread Vincent Hennebert 
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=1151195view=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 SetLong 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=1151447view=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

2011-07-28 Thread Peter B. West 
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=1151447view=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

Stop this infighting [was: Re: svn commit: r1151195 - in /xmlgraphics/fop/trunk: src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java status.xml]

2011-07-28 Thread Simon Pepping 
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=1151195view=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

AW: svn commit: r1151195 - in /xmlgraphics/fop/trunk: src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java status.xml

2011-07-28 Thread Georg Datterl 
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=1151447view=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

2011-07-28 Thread Jeremias Maerki 
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=1151195view=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 SetLong 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?

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.

 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 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 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.

 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 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=1151447view=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. 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?