In order to produce change bars along the columns for changed contents, FO supports the fo:change-bar-begin and fo:change-bar-end elements. These are not supported by FOP. Support for change bars should be added. Effort needed - add parsing and validation of fo: elements - determine fo elements under influence of one or more change bars - when creating areas from such affected fo elements, create additional areas representing the change bars as defined in the FO standard.
Status update: I have been working on the change bar implementation (on and off) and reached a state where I see first results. Summary of what has been implemented and how: - the change-bar-begin and change-bar-end element and attribute recognition was added - the semantics of change bars is that every element between the change-bar-begin and -end is "under the influence" of the change bar and every area it produces should be decorated by a change bar with the form/placement given with the change-bar-begin element Thus, for every FO object, one has to keep track, which change bars (can be more than one) influence it. I added that to FONode and as the vector of "active" change bars has to be kept globally added that to FOUserAgent. Thus, after .fo parsing, at each FObj we have the vector of change bars influencing the FObj. - For each area generated by an object ("influenced area") influenced by change bars, a change bar area has to added (for each such change bar). I choose to implement the rendering of change bars in the Render (in AbstractRenderer.java), as the placement of change bar areas is not decideable locally to each "influenced area" but rather relative to column start/end/left/right edges, which seemed easier to implement when the renderer iterates through all areas anyhow. Thus, the "influenced areas" only record the same vector of change bars that their generating layout manager (via the FObj associated to the LayoutManager) had, when they are generated. I modified the code under oa.fop.layoutmgr.* to copy over the vector of change bars to the Area, which was modified to have an additional vector element. - In the AbstractRenderer, the code that produces output for Block or other Areas was modified to draw the change bars, if any is attached to the area (i.e. the Area was an "influenced area"). Also, the computation of the relevant places along columns, etc. is computed there. This last change needs information about the reference orientation and the writing mode, as change bar placement can depend on the binding edge being coincident with start or stop edge and on can be explicitly be placed on "left" and "right" edges. This amounts to quite some data to record at the ReferenceRegion and handled in the various constructors. From a memory footprint point of view, the additional effort needed by this approach is a vector of change bars (the change bar objects itself are shared), and one change bar object for each begin-change-bar element in the input. In addition, a reference to this vector is attached at every Area (but the vector itself is not copied). A first implementation seems to work but has only been tested on "normal" change bar placement (along the inner edge of a one-column layout). No writing mode or orientation variations have been tried yet:-) A diff with my set of changes will follow in another attachment to this PR soon.
Created attachment 25028 [details] WIP patch for review. Attached is a first patch against trunk that implements change bars. Some things are still not perfect or missing altogether, namely - things mentioned in the previous attachment to this PR - test cases - The default values for the attributes of the fo:change-bar-begin element are not correct - No attributes are currently written out for the area tree XML output that would allow to recreate the change bar info when reading it back in - Some style issues in the code
(In reply to comment #2) Hi Stephan, Thanks for you patch. I've only had a quick look so far, and processing it will take some time. Creating a branch will probably be useful to allow for incremental updates. I'll try and give more feedback in the next days. Meanwhile, I noticed a few mistakes in the property handling: - AFAIU the value of change-bar-class is not required to be unique. The nesting level must also be taken into account, which allows to clear any ambiguity in case the same name is used several times. Also, there is no default value for this property. - the default value of change-bar-color is the current value of the color property, not black. Thanks, Vincent
(In reply to comment #3) > I'll try and give more feedback in the next days. Meanwhile, I noticed a few > mistakes in the property handling: > - AFAIU the value of change-bar-class is not required to be unique. The nesting > level must also be taken into account, which allows to clear any ambiguity in > case the same name is used several times. Also, there is no default value for > this property. yes, and the code in ChangeBarBegin.java only checks, if the same class has already been used in the _actual_ stack of change bars. In ChangeBarEnd.java the class name is removed from the stack of change bars by the pop(), implemented in ChangeBar.java and via the FOUserAgent.java popChangeBar(). That is, the FOUserAgent holds the current stack of nested change bars and there the class must be unique for each one. When the stack is copied to any FObj, a copy of the whole Vector is made and attached. And yes, the class has no default, so it shouldn't have one set in FOPropertyMapping.java > - the default value of change-bar-color is the current value of the color > property, not black. Yes, I was just to lazy to find out, how to get it before I commited the patch:-)
Discussion started on the fop-dev mailing list, see here: http://markmail.org/thread/7hhtzgndvbsgakec
(In reply to comment #4) > > - AFAIU the value of change-bar-class is not required to be unique. The nesting > > level must also be taken into account, which allows to clear any ambiguity in > > case the same name is used several times. Also, there is no default value for > > this property. > > yes, and the code in ChangeBarBegin.java only checks, if the same class > has already been used in the _actual_ stack of change bars. That is still too restrictive. The following construct is perfectly legal: <fo:change-bar-begin change-bar-class="my-change-bar"/> <fo:block>blah... blah... <fo:change-bar-begin change-bar-class="my-change-bar"/> blah... blah... </fo:block> ... <fo:change-bar-end change-bar-class="my-change-bar"/> <fo:change-bar-end change-bar-class="my-change-bar"/> There are two matching pairs of the same class "my-change-bar" but different nesting levels. > > - the default value of change-bar-color is the current value of the color > > property, not black. > > Yes, I was just to lazy to find out, how to get it before I commited the > patch:-) That's ok, then just put a TODO comment to indicate that this part still needs some work (might be of help for yourself as well, actually). Vincent
Hi Stephan, There are Checkstyle issues in your patch (mainly forbidden tab characters). For your next patches, may I ask you to run Checkstyle with the checkstyle-4.0.xml file available at the root of project. No need to re-submit the current one. Thanks, Vincent
Created attachment 25098 [details] WIP patch March 6th 2010 Attached please find a new patch for the current change bar status. Changes from the last patch: - checkstyle issue should be resolved - Default values for change-bar-class and -color changed - If change bar is within fo:marker, make sure it gets processed correctly (property handling of elements inside of fo:marker has to be different for change-bar-begin/end) - Correct handling of check for change-bar-class property; it need not be unique - property for change-bar-offset added (was missing before) - Add writing of "referenceOrientation" and "writingMode" in the XMLRenderer What needs work: - change bars for table elements are not placed correctly (e.g. entry in second column of table) - test cases
Created attachment 25114 [details] Simple document to test change bars I created a simple document to test the change-bar feature and it doesn't seem to work. The change bar starts and finishes too early.
I've just created a temporary branch for the implementation of change bars: http://svn.eu.apache.org/repos/asf/xmlgraphics/fop/branches/Temp_ChangeBars Stephan, please create your patches against that branch from now on. You don't need to check out the branch, you can just switch your local copy by running the following at the root: svn switch http://svn.eu.apache.org/repos/asf/xmlgraphics/fop/branches/Temp_ChangeBars I'm going to apply the patch bits by bits, as issues are being resolved. I've concentrated on the FO part so far. More later. Vincent
(In reply to comment #9) > Created an attachment (id=25114) [details] > Simple document to test change bars > > I created a simple document to test the change-bar feature and it doesn't seem > to work. The change bar starts and finishes too early. yes, the logic for which elements are under change-bar influence were wrong. A fop element should only be displayed with a a change bar, if it is _totally_ between a change-bar-begin and change-bar-end. I have a new implementation and will try to update the patch against trunk some time this or next week. With that new version, the example is displayed OK.
(In reply to comment #11) > (In reply to comment #9) > > Created an attachment (id=25114) [details] [details] > > Simple document to test change bars > > > > I created a simple document to test the change-bar feature and it doesn't seem > > to work. The change bar starts and finishes too early. > > yes, the logic for which elements are under change-bar influence were wrong. > > A fop element should only be displayed with a a change bar, if it is _totally_ > between a change-bar-begin and change-bar-end. > > I have a new implementation and will try to update the patch against trunk > some time this or next week. > > With that new version, the example is displayed OK. Oh, forgot to mention: the issue with wrong placement of change bars in e.g. tables is also resolved with the new patch. Reason was, that for area that are reference-areas, the (absolute) position of the margins (where cb are to be placed) has to be tracked too, as the reference system is shifted inside the area.
Created attachment 26741 [details] New change bar patch against /trunk Attached is a new patch that adds change bar output and change-bar-begin/end processing. The output is done in AbstractRenderer.java by drawing change bar areas if an area to be drawn is affected by change bars. As such, it is not very efficient, i.e., it is not checked, if change bars areas overlap. Also, when drawing change bars for consecutive lines, the change bars are drawn with the same height as the line area, causing gaps between the change bars (as lines are offset). This has been run through checkstyle. The remaining functionality is very much the same as the first patch presented one year (sic) ago. Although 46 files are touched, most changes are quite trivial (adding recording of change bars at areas or layout managers). The test case attached to this PR is now handled correctly. Missing: Unit tests extensive testing.
Created attachment 26745 [details] Patch against /trunk 20110308 An updated patch, last patch did not include changes to AbstractPathOrientedRenderer.java
Created attachment 26991 [details] Updated patch for trunk as of 12.5.2011 Patch for change bar generation updated for trunk as of 12.5.2011
resetting P2 open bugs to P3 pending further review
increase priority for bugs with a patch
Created attachment 28737 [details] Patch against FOP from trunk on 6.5.2012 An update for the support for change bars for current fop
(In reply to comment #18) > Created attachment 28737 [details] > Patch against FOP from trunk on 6.5.2012 > > An update for the support for change bars for current fop quick review of the patch: (1) very important, needs to provide multiple test files to be included under test/layoutengine/standard-testcases (2) please change all code that uses if ( CONSTANT == variable ) to read if ( variable == CONSTANT ) - that is a MSFTism that we don't wish to use (though I admit it appears in various places and needs to be rooted out) (3) note that XSL-FO 1.1 6.4.14 states "The reference-orientation and writing-mode of the region-viewport-area are determined by the formatting object that generates the area (see 6.4.5 fo:page-sequence). The reference-orientation of the region-reference-area is set to "0" and is, therefore, the same as the orientation established by the region-viewport-area. The writing-mode of the region-reference-area is set to the same value as that of the region-viewport-area." and further 6.4.5 states "The reference-orientation and writing-mode of the region-viewport-areas are determined by the values of the "reference-orientation" and "writing-mode" properties of the fo:page-sequence." this means that your changes to BodyRegion [and RegionReference] to derive the writing-mode trait of the body region reference area from the RegionBody (fo:region-body) are not quite correct; at present, i am working on a fix that processes writing modes and reference orientation for regions correctly (including support for the 'from-page-master-region()' property value function); i would suggest that, for the time being you remove all of the writing mode and reference orientation code you added, and go ahead assuming a TB_LR mode for the present; when I have my fix in place, we can coordinate the additional changes needed to handle proper drawing of change bars with other writing modes; (4) using FOUserAgent to store the change bar stack is incorrect, and violates various abstraction barriers (a clue to this is that your additional import from o.a.f.flow is the first reference to that package from FOUserAgent); you should use o.a.f.fo.pagination.PageSequence instead, since it is page sequence which is responsible for generating change bar areas; each (5) please convert the line-ending convention of the new files you have added to use the UNIX (\n) convention prior to creating a patch file; (6) since Vincent has already created a Temp_ChangeBars branch; i suggest you coordinate with him to (1) have him update that branch to the current trunk HEAD, (2) recreate your patch against that updated branch after taking into account any changes required by the above comments;
awaiting updated patch to resolve issues in comment 19
(In reply to comment #20) > awaiting updated patch to resolve issues in comment 19 Update patch that solves issues in comment 19, except for the test cases which have to wait until I find some time to create & add them. As for storing the changebars at the PageSequence instead of the FOUserAgent: I didn't find any existing way to obtain the current PageSequence during parsing. So I added code to Root that allows to obtain the last (current) page sequence and also adds page sequences when they are created (getSucceedingPageSequence(..) was broken in Root, as PageSequence s were never added to the list before...) If there is a more direct way of doing this, please let me know. Otherwise, I removed the code for reference orientation and writing mode and hard-wired if to TB_LR and reference orientation to 0 in the AbstractRender for now. Style issues should also be resolved with this patch.
Created attachment 28738 [details] New patch taking most of issues in comment #19 into account Takes most issues from comment #19 into account, except test cases.