Hi Max,

you are right. It's always better to have small patches focused on one
thing. I don't get my MinOptMax patch focused only on the refactoring of
making MinOptMax immutable.

In the last half-an-hour I walked myself through all the diffs,
file-by-file. I must say - except from TextLayoutManager - it is
possible to understand all changes.

There are two other things done:

 - changing the signature from 
   InlineLevelLayoutManager#getWordChars from
   void getWordChars(StringBuffer sbChars, Position pos) to
   String getWordChars(Position pos)

 - moving the adjustment enum constants from BlockLevelLayoutManager
   into its own class.

All other things are renamings (okay mostly unrelated to MinOptMax) und
reformattings. The problem with the reformatting is, that I mechanically
type Ctl + Alt + L in Intellij after each crappy written peace of code.
I even tried to reformat only selected lines. But one unattended Ctl +
Alt + L is sufficient :| I mean, my code style options in Intellij
conform to the FOP coding styles. Mostly the reformatting corrects
things historically not conforming to the coding styles before.

Now, I could rewind all the not related refactorings from the patch. But
I fear that this would be much work.

So I have one suggestion: Max - maybe we could use Skype and walk
through the code together. If we both see the same diff and I can answer
your questions, I think it would be faster than as when I remove all the
unrelated stuff. Maybe if we both came to the conclusion that it would
be better to remove some aspect entirely - I would do this of course. I
nice side effect from this Skype session would be that we become more
familiar to one another. 

If I think about my OpenType patch or topics like refactoring the font
subsystem and advanced OpenType layout features in text processing, some
Skype sessions would be very useful.

This weekend, I'm a bit offside in Brandenburg without internet. So if
the Skype option is an option I'm happy to talk on Monday - Thursday

Best Regards

On Thu, 2009-10-29 at 14:45 +0100, Max Berger wrote:
> Hi Alex,
> Hi *,
> if you do not yet have FOP developer access, and you are working on a
> larger set of problems, please do not submit one large patch - current
> committers will not have the time to go through every single change.
> Instead, it is much nicer to have a series of small patches.
> One option is to use git. There is a current git clone of the FOP source
> tree available [1][2]. It also provides help to untangle tangled working
> copies [3]. Git lets you produce patches between different individual
> changesets [4], and detects if the patches where applied by someone else.
> References:
> [1] http://wiki.apache.org/general/GitAtApache
> [2] git://git.apache.org/fop.git
> [3] http://tomayko.com/writings/the-thing-about-git
> [4]
> http://www.kernel.org/pub/software/scm/git/docs/user-manual.html#sharing-your-changes
> hth
> Max
> Alexander Kiel schrieb:
> > Hi,
> > 
> > a issued a patch for MinOptMax:
> > https://issues.apache.org/bugzilla/show_bug.cgi?id=48071
> > 
> > Please read my first comment there and consider my patch :-)
> > 
> > Best Regards
> > Alex
> > 
> > On Sun, 2009-10-25 at 23:45 +0100, Alexander Kiel wrote:
> >> Hi,
> >>
> >> the class MinOptMax has some 800 usages in FOP. It holds a triple of
> >> values (min, opt, max) of length quantities. 
> >>
> >> It's heavily used during local computations and passing around. It's
> >> fields are public (whereas the class comment says they are only package
> >> visible). The public fields (and many methods) make MinOptMax mutable.
> >> This mutability is used in the computations for sheer performance
> >> reasons. But this mutability is a big bug attractor in passing around
> >> situations.
> >>
> >> I don't think that anyone would wonder that an immutable MinOptMax would
> >> help FOP.
> >>
> >> This refactoring wouldn't be rocket science if all usages of MinOptMax
> >> would be covered by tests. I just started and found many such uncovered
> >> sections. I'm very new here and so I simply can't write such tests. So I
> >> ask you to possible write such tests or remove uncovered code sections.
> >>
> >> As for performance. I would opt for just refactoring all stuff to
> >> immutable MinOptMax and only introduce an MinOptMaxBuffer if really
> >> needed.
> >>
> >> With an immutable MinOptMax we can easily remove all TODO's inside
> >> MinOptMax. The integrity tests (min <= opt <= max) and we can remove the
> >> clone method, because it wouldn't be needed anymore.
> >>
> >> I just started the refactoring. All what I need are unit tests.
> >>
> >> Best Regards
> >> Alex
> >>
> > 

Attachment: signature.asc
Description: This is a digitally signed message part

Reply via email to