Fop
  1. Fop
  2. FOP-1167

[PATCH] Handling of page-number-citation-last

    Details

    • Type: Bug Bug
    • Status: Open
    • Resolution: Unresolved
    • Affects Version/s: trunk
    • Fix Version/s: None
    • Component/s: page-master/layout
    • Labels:
      None
    • Environment:
      Operating System: other
      Platform: Other
    • External issue ID:
      39118

      Description

      This patch add the handling of the page-number-citation-last element defined in
      the XSL 1.1 spec

      1. patch_pncl.patch
        19 kB
        Pierre-Henri Kraus
      2. page-number-citation-last_basic.xml
        6 kB
        Pierre-Henri Kraus
      3. done.patch
        49 kB
        Pierre-Henri Kraus

        Activity

        Hide
        Glenn Adams added a comment -

        change status from ASSIGNED to NEW for consistency

        Show
        Glenn Adams added a comment - change status from ASSIGNED to NEW for consistency
        Hide
        Glenn Adams added a comment -

        increase priority for bugs with a patch

        Show
        Glenn Adams added a comment - increase priority for bugs with a patch
        Hide
        Glenn Adams added a comment -

        resetting P2 open bugs to P3 pending further review

        Show
        Glenn Adams added a comment - resetting P2 open bugs to P3 pending further review
        Hide
        Andreas L. Delmelle added a comment -

        Small update on this one:
        page-number-citation-last is currently correctly handled for fo:block, fo:inline and fo:list-block

        Does not work yet for:
        -> table-elements: ids not processed for table-header, table-footer, table-body and table-row; notifyEndOfLayout() is still called multiple times for table and table-cell.
        -> block-containers: AbstractLM.isLast(Position) does not work reliably in this case, as the BlockContainerLM also wraps the auxiliary positions for its children's borders. Result is that the 'last' position in the iterator that the block-container receives during addAreas() is not one of its own. It is given a position index, though, which is greater than that of the last content element, so...
        -> page-sequence: to be investigated

        Show
        Andreas L. Delmelle added a comment - Small update on this one: page-number-citation-last is currently correctly handled for fo:block, fo:inline and fo:list-block Does not work yet for: -> table-elements: ids not processed for table-header, table-footer, table-body and table-row; notifyEndOfLayout() is still called multiple times for table and table-cell. -> block-containers: AbstractLM.isLast(Position) does not work reliably in this case, as the BlockContainerLM also wraps the auxiliary positions for its children's borders. Result is that the 'last' position in the iterator that the block-container receives during addAreas() is not one of its own. It is given a position index, though, which is greater than that of the last content element, so... -> page-sequence: to be investigated
        Hide
        Jeremias Maerki added a comment -

        Pierre-Henri, I've applied your patch with modifications. See:
        http://svn.apache.org/viewcvs?rev=396243&view=rev

        Thanks a lot for that and sorry for the delay.

        I found a remaining problem with your patch. When a formatting object spans
        multiple pages, forward references are not properly resolved. You can see that
        in the *_basic.xml test case which I've modified and disabled. It looks like you
        didn't entirely get my hint last time. The problem is that addAreas() can be
        called multiple times. An example: If a block spans multiple pages, it is called
        once for each page it generates area for. Since you notify the AreaTreeHandler
        after each call to addAreas (and not only after the last) a forward reference
        gets the wrong page number (i.e. the one of the first area). Determining the
        last call to addAreas for a formatting objects might be a little tricky, however.

        Furthermore, you've only addressed block-level formatting objects and
        page-sequence for ID processing, but some of the inline-level formatting objects
        can similarly span multiple pages (like fo:inline for example). I don't think
        this is critical since you might rarely refer to an inline-level ID with
        page-number-citation-last. The most important use case is certainly referring to
        an ID from page-sequence to achieve reliable "page x of y" without the "empty
        block with ID" hack we've used until today.

        I'm leaving the issue open for now.

        Show
        Jeremias Maerki added a comment - Pierre-Henri, I've applied your patch with modifications. See: http://svn.apache.org/viewcvs?rev=396243&view=rev Thanks a lot for that and sorry for the delay. I found a remaining problem with your patch. When a formatting object spans multiple pages, forward references are not properly resolved. You can see that in the *_basic.xml test case which I've modified and disabled. It looks like you didn't entirely get my hint last time. The problem is that addAreas() can be called multiple times. An example: If a block spans multiple pages, it is called once for each page it generates area for. Since you notify the AreaTreeHandler after each call to addAreas (and not only after the last) a forward reference gets the wrong page number (i.e. the one of the first area). Determining the last call to addAreas for a formatting objects might be a little tricky, however. Furthermore, you've only addressed block-level formatting objects and page-sequence for ID processing, but some of the inline-level formatting objects can similarly span multiple pages (like fo:inline for example). I don't think this is critical since you might rarely refer to an inline-level ID with page-number-citation-last. The most important use case is certainly referring to an ID from page-sequence to achieve reliable "page x of y" without the "empty block with ID" hack we've used until today. I'm leaving the issue open for now.
        Hide
        Pierre-Henri Kraus added a comment -

        Attachment done.patch has been added with description: This file is the complete patch to handle page-number-citation-last in the different FO elements requested

        Show
        Pierre-Henri Kraus added a comment - Attachment done.patch has been added with description: This file is the complete patch to handle page-number-citation-last in the different FO elements requested
        Hide
        Pierre-Henri Kraus added a comment -

        Ignore the precedent patch, this one applies the same modifications + new ones

        Patch Explanation
        -----------------

        OK, this is a file to help you understand the changes applied
        to the source code to handle the page-number-citation-last
        element.

        New Data Structures
        -------------------

        2 java.util.HashSet in AreaTreeHandler.java :

        private Set unresolvedLayoutManagers = new HashSet();
        private Set alreadyResolvedIDs = new HashSet();

        The role of unresolvedLayoutManagers is to keep a list of
        "which formatting objects contributes IDs" as you proposed it.

        The role of alreadyResolvedIDs is to keep a list of IDs already
        resolved. That role might seems unnecessary, so here's a little
        example that i came across while doing the modification :

        ...
        <fo:block>page: <fo:page-number/>, bof2 is on page <fo:page-number-citation
        ref-id="bof2"/></fo:block>
        <fo:block linefeed-treatment="preserve" id="bof3">page: <fo:page-number/>
        test
        test
        test
        test
        test
        test
        </fo:block>
        <fo:block break-before="page">page: <fo:page-number/></fo:block>
        <fo:block>page: <fo:page-number/></fo:block>
        <fo:block>page: <fo:page-number/> of <fo:page-number-citation
        ref-id="eof1"/></fo:block>
        <fo:block>page(-last): <fo:page-number/> of <fo:page-number-citation-last
        ref-id="bof3"/></fo:block>
        ...

        Withouth the second Set, the problem that we come across is that the
        layout of the block with the id 'bof3' is finished before the creation
        of the page-number-citation-last layout manager. Due to the construction
        of the PNCL-LM, the first step it does is to add the UnresolvedPageNumber
        in the unresolvedLayoutManager list (there is no way to known in advance
        if the page number can be resolved or not at this point). Each layout manager
        is responsible of removing its ID from the unresolvedLayoutManager's list
        and, at the same time to call for the resolution of the IDs its possess.
        But as the ID of the block of the block layout manager is inexisting in the
        list as the PNCL-LM haven't addded yet, it is not resolved when "it should".
        The solution that i chose is to add each ID resolved "in advance" in a Set,
        that the PNCL-LM will check to see if it can skip the "wait for resolve"
        step, so it can immediately be resolved.

        I hope that this little explanation helped you to understand the changes.

        What have been done -> handling of PageSequence ids and Block ids + Block level

        ids (as page-number-citation)

        Known limits -> no handling of IDs contained in a root, flow, and the table
        related ids : table-header, table-footer, table-body ...
        And, i don't know if it's a problem, but when the actual UnresolvedPageNumber
        object is resolved, the font associated to it is null (this is due to the
        "transient" state of the object i guess...) and the IPD can't be updated (only
        the case with PageSequences).

        New Code
        --------
        As explained above.
        The source code changes are avail in the patch file.

        Show
        Pierre-Henri Kraus added a comment - Ignore the precedent patch, this one applies the same modifications + new ones Patch Explanation ----------------- OK, this is a file to help you understand the changes applied to the source code to handle the page-number-citation-last element. New Data Structures ------------------- 2 java.util.HashSet in AreaTreeHandler.java : private Set unresolvedLayoutManagers = new HashSet(); private Set alreadyResolvedIDs = new HashSet(); The role of unresolvedLayoutManagers is to keep a list of "which formatting objects contributes IDs" as you proposed it. The role of alreadyResolvedIDs is to keep a list of IDs already resolved. That role might seems unnecessary, so here's a little example that i came across while doing the modification : ... <fo:block>page: <fo:page-number/>, bof2 is on page <fo:page-number-citation ref-id="bof2"/></fo:block> <fo:block linefeed-treatment="preserve" id="bof3">page: <fo:page-number/> test test test test test test </fo:block> <fo:block break-before="page">page: <fo:page-number/></fo:block> <fo:block>page: <fo:page-number/></fo:block> <fo:block>page: <fo:page-number/> of <fo:page-number-citation ref-id="eof1"/></fo:block> <fo:block>page(-last): <fo:page-number/> of <fo:page-number-citation-last ref-id="bof3"/></fo:block> ... Withouth the second Set, the problem that we come across is that the layout of the block with the id 'bof3' is finished before the creation of the page-number-citation-last layout manager. Due to the construction of the PNCL-LM, the first step it does is to add the UnresolvedPageNumber in the unresolvedLayoutManager list (there is no way to known in advance if the page number can be resolved or not at this point). Each layout manager is responsible of removing its ID from the unresolvedLayoutManager's list and, at the same time to call for the resolution of the IDs its possess. But as the ID of the block of the block layout manager is inexisting in the list as the PNCL-LM haven't addded yet, it is not resolved when "it should". The solution that i chose is to add each ID resolved "in advance" in a Set, that the PNCL-LM will check to see if it can skip the "wait for resolve" step, so it can immediately be resolved. I hope that this little explanation helped you to understand the changes. What have been done -> handling of PageSequence ids and Block ids + Block level ids (as page-number-citation) Known limits -> no handling of IDs contained in a root, flow, and the table related ids : table-header, table-footer, table-body ... And, i don't know if it's a problem, but when the actual UnresolvedPageNumber object is resolved, the font associated to it is null (this is due to the "transient" state of the object i guess...) and the IPD can't be updated (only the case with PageSequences). New Code -------- As explained above. The source code changes are avail in the patch file.
        Hide
        Jeremias Maerki added a comment -

        (In reply to comment #2)
        > Created an attachment (id=18004) [edit]
        > Basic testcase to check the page-number-citation-last element
        >
        > OK, sorry for the delay but JUnit was acting strange
        > 2 questions:
        > - This test uses the "linefeed-treatment=preserve" property, is it ok ?

        Yes, but I don't see what its function is in this particular test case.

        > - Do you want more test for the element ?

        If you don't mind, I have an additional wish: Testing page-number-citation-last
        referencing an id on the page-sequence because that will be the major use case
        for it. The most interesting part is then to combine that with a
        page-position="last" where the last page is a blank page (see
        page-position_last_1.xml).

        > And to Jeremias, i'll try to fax the ICLA today or tommorow (asking someone as
        > i don't have a fax myself), and if i can't i'll send it by mail...

        Thank you! If possible, please try fax because we've had bad experiences with
        normal mail.

        Show
        Jeremias Maerki added a comment - (In reply to comment #2) > Created an attachment (id=18004) [edit] > Basic testcase to check the page-number-citation-last element > > OK, sorry for the delay but JUnit was acting strange > 2 questions: > - This test uses the "linefeed-treatment=preserve" property, is it ok ? Yes, but I don't see what its function is in this particular test case. > - Do you want more test for the element ? If you don't mind, I have an additional wish: Testing page-number-citation-last referencing an id on the page-sequence because that will be the major use case for it. The most interesting part is then to combine that with a page-position="last" where the last page is a blank page (see page-position_last_1.xml). > And to Jeremias, i'll try to fax the ICLA today or tommorow (asking someone as > i don't have a fax myself), and if i can't i'll send it by mail... Thank you! If possible, please try fax because we've had bad experiences with normal mail.
        Hide
        Pierre-Henri Kraus added a comment -

        Attachment page-number-citation-last_basic.xml has been added with description: Basic testcase to check the page-number-citation-last element

        Show
        Pierre-Henri Kraus added a comment - Attachment page-number-citation-last_basic.xml has been added with description: Basic testcase to check the page-number-citation-last element
        Hide
        Pierre-Henri Kraus added a comment -

        OK, sorry for the delay but JUnit was acting strange
        2 questions:

        • This test uses the "linefeed-treatment=preserve" property, is it ok ?
        • Do you want more test for the element ?

        And to Jeremias, i'll try to fax the ICLA today or tommorow (asking someone as
        i don't have a fax myself), and if i can't i'll send it by mail...

        Show
        Pierre-Henri Kraus added a comment - OK, sorry for the delay but JUnit was acting strange 2 questions: This test uses the "linefeed-treatment=preserve" property, is it ok ? Do you want more test for the element ? And to Jeremias, i'll try to fax the ICLA today or tommorow (asking someone as i don't have a fax myself), and if i can't i'll send it by mail...
        Hide
        Pierre-Henri Kraus added a comment -

        Attachment patch_pncl.patch has been added with description: The patch file

        Show
        Pierre-Henri Kraus added a comment - Attachment patch_pncl.patch has been added with description: The patch file

          People

          • Assignee:
            Unassigned
            Reporter:
            Pierre-Henri Kraus
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development