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

--- Comment #31 from Glenn Adams <gl...@skynav.com> 2011-03-22 10:59:03 EDT ---
inline

(In reply to comment #30)
> Glenn,
> 
> I had a brief look at your branch and have the following questions and
> comments:
> 
> You implemented the BIDI algorithm in a separate BidiUtil class. Why didn't 
> you
> integrate it into the layout engine? It seems that what is done there is a
> layout task, therefore should be put in the layout engine classes. FO tree
> objects are now manipulated by both the layout engine and that class, and I'm
> seriously concerned about the maintenance issues that that may create. 

BidiUtil is defined in o.a.f.layoutmgr, therefore it is "integrated into the
layout engine". I considered placing it in o.a.f.util or in o.a.f.text.bidi;
however, since it is only referenced by o.a.f.layoutmgr code, I decided that
was the best home for it at this time.

However, having said that, it is not performing layout and knows nothing about
areas or geometries whatsoever. It's functionality is invoked in two places:

(1) in PageSequenceLayoutManager.activateLayout(), in order to resolve bidi
levels of each delimited text range, and
(2) in LineLayoutManager.add{Inline,Block}Area, after completing line area
construction;

See XSL-FO Section 5.8 for more information. I'm open to any concrete
suggestions about a better point of integration, but I don't see any at present
that is consistent with 5.8.

> Also, AFAIU a whole data structure is created there that will exist in 
> parallel
> with the Knuth elements created by the layout engine. In addition to the 
> memory
> issues, isn't there a risk of discrepancy between the two? That may cause
> hard-to-find bugs.

There is no relationship between the bidi levels created by BidiUtil and Knuth
elements used for line breaking.

> 
> How feasible is it to run the BIDI algorithm on a subset of the FO tree? If 
> I'm
> right, ATM it is launched on a whole page-sequence. When we refactor the code
> so that layout can be started before a whole page-sequence has been parsed, in
> order to avoid the infamous memory issue that a lot of users are running into,
> will that code allow to do it?

I'm not sure what you mean by "ATM". The semantics of XSL-FO 5.8 as embodied by
the present implementation will have to be taken into account by such a
refactoring, as I'm sure will many other aspects of the current FOP
implementation.

> I have a concern with some metrics. That BidiUtil class is more than 1700
> lines. There are a few other new classes that are more than 1000 lines long,
> and TTFFile now is more than 5500 lines (!). What's your plan to break them
> down into more manageable chunks? Also, there are now 72 classes in the
> o.a.f.fonts package.

I have no such plan in the near to medium term. There is no technical reason to
refactor TTFFile, though I agree it is on the long side, and could probably use
a general refactoring in the long term. 

> Also, many variables have a two- or three-letter name, which makes it 
> difficult
> to understand what their purpose is, especially if a lot of them are involved
> at the same time. I realise there usually is a comment explaining what the
> variable does when it's defined, but it would be easier if the variable were
> carrying the comment with itself by having a longer name.

Symbol length is a trade off between readability and other factors, such as
line length and typing time. I prefer short symbols. Perhaps it is my training
in mathematics, electrical engineering, and having started programming with APL
that leads me in that direction. The meaning of short symbols should be readily
inferable given sufficient knowledge of the problem domain.

Best,
Glenn

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

Reply via email to