Re: svn commit: r591587 - in /xmlgraphics/fop/trunk: ./ src/java/org/apache/fop/render/ps/ src/java/org/apache/fop/svg/
Hi Jeremias,
I’d have a couple of comments regarding your changes. I’ve only had
a quick look and don’t understand this part of the code well, but
following the principle of pair review, here they are:
- at several places there is code like the following:
private void checkInTextObject() {
if (!inTextObject) {
throw new IllegalStateException(Not in text object);
}
}
I understand and don’t criticise the need for safety checks,
especially if the code is new or complex. Only, since we now have
Java 1.4 as a minimal requirement, why not use assert statements? They
were created exactly for that.
- in PDFTextUtil:
- the setTextRenderingMode(int) method is only used within
PDFTextUtil; why then not making it private? The other
setTextRenderingMode method will still be available and seems to be
much safer to use anyway.
- it seems to me that a bit more of the internal state of
a PDFTextUtil object could be controlled by the object itself,
especially in the following piece of code (PDFTextPainter, l.220):
Font f = textUtil.selectFontForChar(ch);
if (f != textUtil.getCurrentFont()) {
textUtil.writeTJ();
textUtil.setCurrentFont(f);
textUtil.writeTf(f);
textUtil.writeTextMatrix(localTransform);
}
A signalFontUsed(f) method might make sense. Basically doing the
same test as above, but inside PDFTextUtil. The code would be better
encapsulated, so more maintainable. WDYT?
- in PDFTextPainter: now that the code is working, you probably want to
remove the DEBUG variable and all the statements displaying stuff to
System.out; or replace them with proper log.debug statements ;-)
For the rest I believe when you say it’s working... Congrats!
Vincent
Re: svn commit: r591587 - in /xmlgraphics/fop/trunk: ./ src/java/org/apache/fop/render/ps/ src/java/org/apache/fop/svg/
On 05.11.2007 13:05:29 Vincent Hennebert wrote:
Hi Jeremias,
I’d have a couple of comments regarding your changes. I’ve only had
a quick look and don’t understand this part of the code well, but
following the principle of pair review, here they are:
- at several places there is code like the following:
private void checkInTextObject() {
if (!inTextObject) {
throw new IllegalStateException(Not in text object);
}
}
I understand and don’t criticise the need for safety checks,
especially if the code is new or complex. Only, since we now have
Java 1.4 as a minimal requirement, why not use assert statements? They
were created exactly for that.
I could do a lot of stuff. I'm so used to working pre-Java-1.4 that it
didn't occur to me. Feel free to change. There's no code-ownership in
Apache projects.
- in PDFTextUtil:
- the setTextRenderingMode(int) method is only used within
PDFTextUtil; why then not making it private? The other
setTextRenderingMode method will still be available and seems to be
much safer to use anyway.
PDFTextUtil is designed to be useful for classes other than
PDFTextPainter. Someone might prefer working more closely to the PDF
spec and therefore prefer the int variant.
- it seems to me that a bit more of the internal state of
a PDFTextUtil object could be controlled by the object itself,
especially in the following piece of code (PDFTextPainter, l.220):
Font f = textUtil.selectFontForChar(ch);
if (f != textUtil.getCurrentFont()) {
textUtil.writeTJ();
textUtil.setCurrentFont(f);
textUtil.writeTf(f);
textUtil.writeTextMatrix(localTransform);
}
A signalFontUsed(f) method might make sense. Basically doing the
same test as above, but inside PDFTextUtil. The code would be better
encapsulated, so more maintainable. WDYT?
Again, feel free to change and improve.
- in PDFTextPainter: now that the code is working, you probably want to
remove the DEBUG variable and all the statements displaying stuff to
System.out; or replace them with proper log.debug statements ;-)
No, I don't. The debug code is most probably optimized away by the
compiler so it doesn't bother us at runtime. If any bugs arise, it's
convenient to switch on. Most importantly, though, the Batik guys have
indicated that they don't want to work with a logging framework. Since
this code is destined to go to Batik I try not to introduce more Commons
Logging in this area.
For the rest I believe when you say it’s working... Congrats!
Vincent
Jeremias Maerki
Re: svn commit: r591587 - in /xmlgraphics/fop/trunk: ./ src/java/org/apache/fop/render/ps/ src/java/org/apache/fop/svg/
Jeremias Maerki wrote:
snip/
- it seems to me that a bit more of the internal state of
a PDFTextUtil object could be controlled by the object itself,
especially in the following piece of code (PDFTextPainter, l.220):
Font f = textUtil.selectFontForChar(ch);
if (f != textUtil.getCurrentFont()) {
textUtil.writeTJ();
textUtil.setCurrentFont(f);
textUtil.writeTf(f);
textUtil.writeTextMatrix(localTransform);
}
A signalFontUsed(f) method might make sense. Basically doing the
same test as above, but inside PDFTextUtil. The code would be better
encapsulated, so more maintainable. WDYT?
Again, feel free to change and improve.
Ok. It’s just that I’m reluctant to modify code I’m not familiar with,
and from which I may have missed the logic.
- in PDFTextPainter: now that the code is working, you probably want to
remove the DEBUG variable and all the statements displaying stuff to
System.out; or replace them with proper log.debug statements ;-)
No, I don't. The debug code is most probably optimized away by the
compiler so it doesn't bother us at runtime. If any bugs arise, it's
Except if the DEBUG variable is left to true...
convenient to switch on. Most importantly, though, the Batik guys have
indicated that they don't want to work with a logging framework. Since
this code is destined to go to Batik I try not to introduce more Commons
Logging in this area.
Vincent
Re: svn commit: r591587 - in /xmlgraphics/fop/trunk: ./ src/java/org/apache/fop/render/ps/ src/java/org/apache/fop/svg/
On 05.11.2007 15:37:29 Vincent Hennebert wrote: snip/ - in PDFTextPainter: now that the code is working, you probably want to remove the DEBUG variable and all the statements displaying stuff to System.out; or replace them with proper log.debug statements ;-) No, I don't. The debug code is most probably optimized away by the compiler so it doesn't bother us at runtime. If any bugs arise, it's Except if the DEBUG variable is left to true... Oh, that was a mistake. I thought I switched it to false. Thanks for correcting it. snip/ Jeremias Maerki
