DO NOT REPLY [Bug 48071] New: [PATCH] MinOptMax immutable/layoutmgr refactoring

https://issues.apache.org/bugzilla/show_bug.cgi?id=48071

           Summary: [PATCH] MinOptMax immutable/layoutmgr refactoring
           Product: Fop
           Version: 1.0dev
          Platform: PC
        OS/Version: Linux
            Status: NEW
          Severity: normal
          Priority: P2
         Component: page-master/layout
        AssignedTo: fop-dev@...
        ReportedBy: alexanderkiel@...


Created an attachment (id=24429)
 --> (https://issues.apache.org/bugzilla/attachment.cgi?id=24429)
patch

This is the patch which I annonced in
http://markmail.org/thread/2ka3sejiecymn4te.

It's all about refactoring. No new functionality.

The main point was to make MinOptMax immutable. MinOptMax has around 800 usages
in FOP. It's central usage is in the package layoutmgr. It's passed around a
lot. This passing around highly benefits from the newly immutability. It's also
used in computations which may be a bit slower now but not by a far margin.

The big point is that the invariant min <= opt <= max can now be really
guarantied (except at malicious serialization - but I could handle this). There
is now no way to break this invariant (please validate this).

I have a MinOptMaxTest which covers all method except for the deprecated ones.
MinOptMax conforms to Checkstyle and looks nice. :-)  

It's really an all new class.

Apart from MinOptMax, I refactored some of the code around it's usage. Most of
the layoutmanager code will improve from refactoring. Most of it isn't up to
todays standards. Sadly I'm not able to refactor much, because I don't really
in detail what is happening. I saw many Knuth things and as a LaTex user, I
have an idea about this algorithm but not in detail.

I'm really interested in TextLayoutManager so I refactored there a lot in the
process to understand the code. I created the TextAreaBuilder in the process of
refactoring the method createTextArea. This method is so complex that I though
it is really an class of its own.

I'm interested in TextLayoutManager because I like to implement some advanced
OpenType layout features to the text processing. As you properly know, I
currently write new code to read TrueType/OpenType files. I'm at a point where
I can read and preprocess all the OpenType advanced typography features such as
glyph substitution and glyph positioning (much more than kerning). So at the
end it would be nice to actually see this features in action. So I came to
TextLayoutManager.

Back to my patch. Most of the changes I made are covered by tests. Of course it
runs all tests. But there are some uncovered places as I already noted in my
first mail of http://markmail.org/thread/2ka3sejiecymn4te.I don't know of the
uncovered places really used in FOP. So it would be necessary for the committer
to look at such uncovered places before applying may patch.

I hope my patch will help the layoutmgr code.

Best Regards
Alex

--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/bugzilla/show_bug.cgi?id=48071

--- Comment #1 from Max Berger <max@...> 2009-10-29 06:20:57 UTC ---
Alex,

just looked at your patch:

- I like the basic idea of replacing public field with the corresponding
accessors, and making the class immutable. This is much cleaner design.

- A speed comparison would be nice - is the new implementation slower or
faster? Do you have "proof" for your claim (e.g. time render a document with
and without the patch - any significant difference?)

- As another enhancement, the flyweight pattern could be applied to minoptmax
(of course, only after your patch is applied)

BUT:
- This patch has MUCH more content than just the MinOptMax. It contains many
reformattings (probably automatically done by your IDE) AND some other renames,
cleanups, etc. Although I do believe that what you did makes sense in most
cases, this makes this patch very difficult to review.

- I also see some diffs in $Id$ lines, which should not happen (looks like
we're missing svn props here, will check that)

I know it is difficult, but could you please refafctor the patch to address
only one issue at a time? (I'll send an email about this to the fop-dev list)

--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.