Hi,

Looks like Max is busy with more urgent things :-)

As this patch will affect my future work on the layout engine, I’d like
to take over the patch review.

Your suggestion to use Skype sounds good. That will ease the job a bit.
I’ll contact you off-line to exchange details and arrange a time.

More below:

Alexander Kiel wrote:
> 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)

What’s the reason for that change?


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

I’m strongly against reformatting a whole file in one go. At least, as
long as code formatters don’t do a better job at formatting multi-line
statements. They break the line as near to the length limit (100 in FOP)
as possible, instead of breaking as high as possible in the statement’s
hierarchy. For example, in o.a.f.layoutmgr.table.ActiveCell.java:
        elementList.add(iter.nextIndex() - 1,
                new FillerPenalty(minBPD - cumulateLength));

is automatically formatted into:
        elementList.add(iter.nextIndex() - 1, new FillerPenalty(minBPD
                - cumulateLength));

which I find is less readable. Also, sometimes you break a line where
it’s most logical (e.g., keeping variables of similar semantics on one
line), and a code formatter will never be able to do that.

So, please try and ban that Ctl-Alt-L shortcut :-)


Also, from the quick look I had at your patch, many of your
reformattings affect Javadoc comments. I don’t think we have any
enforced convention about Javadoc. Agreeing upon one would probably ease
everyone’s lives.


> 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
>>
<snip/>

Thanks,
Vincent

Reply via email to