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
