Problem in PageBreakingAlgorithm Constructor

[Alexander Kiel]

Hi,

the constructor of the class PageBreakingAlgorithm looks like this:

public PageBreakingAlgorithm(LayoutManager topLevelLM,
                                 PageProvider pageProvider,
                                 PageBreakingLayoutListener
layoutListener,
                                 int alignment, int alignmentLast,
                                 MinOptMax footnoteSeparatorLength,
                                 boolean partOverflowRecovery, boolean
autoHeight,
                                 boolean favorSinglePart) {
        super(alignment, alignmentLast, true, partOverflowRecovery, 0);
        this.topLevelLM = topLevelLM;
        this.pageProvider = pageProvider;
        this.layoutListener = layoutListener;
        best = new BestPageRecords();
        this.footnoteSeparatorLength = (MinOptMax)
footnoteSeparatorLength.clone();
        // add some stretch, to avoid a restart for every page
containing footnotes
        if (footnoteSeparatorLength.min == footnoteSeparatorLength.max)
{
            footnoteSeparatorLength.max += 10000;
        }
        this.autoHeight = autoHeight;
        this.favorSinglePart = favorSinglePart;
    }

The problem is the line:

footnoteSeparatorLength.max += 10000;

I think it should read rather:

this.footnoteSeparatorLength.max += 10000;

Clients calling the constructor shouldn't be happy about this situation.

I discovered this statement while refactoring the MinOptMax class into
an immutable one. I think this refactoring project should be another
mail. But this example shows how valuable a immutable MinOptMax would
be.

Can someone familiar with this part of FOP write a test which fails
against this current behavior? I could than use this test to verify that
my immutable MinOptMax works with this part.


Thanks
Alex

--
e-mail: alexanderkiel@...
web:    www.alexanderkiel.net

[Vincent Hennebert-2]

Hi Alexander,

That piece of code didn’t make sense, in addition to being wrong. It’s
up to the user to add stretching to the footnote separator if they ever
want to. Actually that code could even have led to unexpected results in
some cases.

Since it wasn’t breaking any layout test case, I removed it altogether.

Thanks,
Vincent


Alexander Kiel wrote:

> Hi,
>
> the constructor of the class PageBreakingAlgorithm looks like this:
>
> public PageBreakingAlgorithm(LayoutManager topLevelLM,
>                                  PageProvider pageProvider,
>                                  PageBreakingLayoutListener
> layoutListener,
>                                  int alignment, int alignmentLast,
>                                  MinOptMax footnoteSeparatorLength,
>                                  boolean partOverflowRecovery, boolean
> autoHeight,
>                                  boolean favorSinglePart) {
>         super(alignment, alignmentLast, true, partOverflowRecovery, 0);
>         this.topLevelLM = topLevelLM;
>         this.pageProvider = pageProvider;
>         this.layoutListener = layoutListener;
>         best = new BestPageRecords();
>         this.footnoteSeparatorLength = (MinOptMax)
> footnoteSeparatorLength.clone();
>         // add some stretch, to avoid a restart for every page
> containing footnotes
>         if (footnoteSeparatorLength.min == footnoteSeparatorLength.max)
> {
>             footnoteSeparatorLength.max += 10000;
>         }
>         this.autoHeight = autoHeight;
>         this.favorSinglePart = favorSinglePart;
>     }
>
> The problem is the line:
>
> footnoteSeparatorLength.max += 10000;
>
> I think it should read rather:
>
> this.footnoteSeparatorLength.max += 10000;
>
> Clients calling the constructor shouldn't be happy about this situation.
>
> I discovered this statement while refactoring the MinOptMax class into
> an immutable one. I think this refactoring project should be another
> mail. But this example shows how valuable a immutable MinOptMax would
> be.
>
> Can someone familiar with this part of FOP write a test which fails
> against this current behavior? I could than use this test to verify that
> my immutable MinOptMax works with this part.
>
>
> Thanks
> Alex
>

[Alexander Kiel]

Hi Vincent,

thanks for sorting this out.

Best Regards
Alexander

On Mon, 2009-10-26 at 12:18 +0000, Vincent Hennebert wrote:

> Hi Alexander,
>
> That piece of code didn’t make sense, in addition to being wrong. It’s
> up to the user to add stretching to the footnote separator if they ever
> want to. Actually that code could even have led to unexpected results in
> some cases.
>
> Since it wasn’t breaking any layout test case, I removed it altogether.
>
> Thanks,
> Vincent
>
>
> Alexander Kiel wrote:
> > Hi,
> >
> > the constructor of the class PageBreakingAlgorithm looks like this:
> >
> > public PageBreakingAlgorithm(LayoutManager topLevelLM,
> >                                  PageProvider pageProvider,
> >                                  PageBreakingLayoutListener
> > layoutListener,
> >                                  int alignment, int alignmentLast,
> >                                  MinOptMax footnoteSeparatorLength,
> >                                  boolean partOverflowRecovery, boolean
> > autoHeight,
> >                                  boolean favorSinglePart) {
> >         super(alignment, alignmentLast, true, partOverflowRecovery, 0);
> >         this.topLevelLM = topLevelLM;
> >         this.pageProvider = pageProvider;
> >         this.layoutListener = layoutListener;
> >         best = new BestPageRecords();
> >         this.footnoteSeparatorLength = (MinOptMax)
> > footnoteSeparatorLength.clone();
> >         // add some stretch, to avoid a restart for every page
> > containing footnotes
> >         if (footnoteSeparatorLength.min == footnoteSeparatorLength.max)
> > {
> >             footnoteSeparatorLength.max += 10000;
> >         }
> >         this.autoHeight = autoHeight;
> >         this.favorSinglePart = favorSinglePart;
> >     }
> >
> > The problem is the line:
> >
> > footnoteSeparatorLength.max += 10000;
> >
> > I think it should read rather:
> >
> > this.footnoteSeparatorLength.max += 10000;
> >
> > Clients calling the constructor shouldn't be happy about this situation.
> >
> > I discovered this statement while refactoring the MinOptMax class into
> > an immutable one. I think this refactoring project should be another
> > mail. But this example shows how valuable a immutable MinOptMax would
> > be.
> >
> > Can someone familiar with this part of FOP write a test which fails
> > against this current behavior? I could than use this test to verify that
> > my immutable MinOptMax works with this part.
> >
> >
> > Thanks
> > Alex
> >
>