You are viewing a plain text version of this content. The canonical link for it is here.
Posted to fop-dev@xmlgraphics.apache.org by Max Berger <ma...@berger.name> on 2009/10/29 14:45:56 UTC
Patching with GIT/SVN (Re: Making MinOptMax Immutable)
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
>>
>
--
http://max.berger.name/
OpenPGP ID: C93C5700 Fpr: AB6638CE472A499B3959 ADA2F989A2E5C93C5700
Re: Patching with GIT/SVN (Re: Making MinOptMax Immutable)
Posted by Alexander Kiel <al...@gmx.net>.
Hi Max,
> One more thing I noticed: Alex, you are introducing deprecated methods
> and then using them. Please either
> - don't mark the methods as deprecated, if they are valid helper methods or
> - don't use the deprecated methods, but rather use whatever you inted to
> replace it with.
I marked the methods as deprecated, because they are deprecated in a
sense that I don't want them in MinOptMax and they are used in one code
region which is not covered by tests and so I don't know if this region
is used at all. I will discuss this with Vincent. Perhaps he knows
something of the code region which uses my deprecated methods.
Best Regards
Alex
Re: Patching with GIT/SVN (Re: Making MinOptMax Immutable)
Posted by Max Berger <ma...@berger.name>.
Hi *,
Vincent Hennebert schrieb:
> Hi,
>
> Looks like Max is busy with more urgent things :-)
Yes, work keeps me occupied most of the time. I was actually just
looking at the patch again, and decided that I am unable to apply it,
because I do cannot verify if the renames are correct, as it affects
some areas I am not familiar with.
> As this patch will affect my future work on the layout engine, I’d like
> to take over the patch review.
Vincent, feel free to do so. I have not assigned the patch to myself as
I could not promise to review it in a decent time.
>> 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.
One more thing I noticed: Alex, you are introducing deprecated methods
and then using them. Please either
- don't mark the methods as deprecated, if they are valid helper methods or
- don't use the deprecated methods, but rather use whatever you inted to
replace it with.
Max
--
http://max.berger.name/
OpenPGP ID: C93C5700 Fpr: AB6638CE472A499B3959 ADA2F989A2E5C93C5700
Re: Patching with GIT/SVN (Re: Making MinOptMax Immutable)
Posted by Vincent Hennebert <vh...@gmail.com>.
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
Re: Patching with GIT/SVN (Re: Making MinOptMax Immutable)
Posted by Alexander Kiel <al...@gmx.net>.
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
> >>
> >
>
>