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 evening. Best Regards Alex 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 > >> > > > >
signature.asc
Description: This is a digitally signed message part
