Fop
  1. Fop
  2. FOP-1134

[patch] from-table-column implementation

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Resolution: Fixed
    • Affects Version/s: trunk
    • Fix Version/s: None
    • Component/s: fo tree
    • Labels:
      None
    • Environment:
      Operating System: other
      Platform: Other
    • External issue ID:
      38282

      Description

      This patch should implement the from-table-function.

      Changes to TableFObj.java:
      1) Make the getTable method public to have a quick access.
      Alternatives:
      1a) Only make public in TableCell (and call there super.getTable)
      1b) Explicitly traverse back in the FromTableColumnFunction
      with getParent until we reach the Table object.

      Changes to TableColumn:
      2) Preserve PropertyList and add an accessor for this list
      Note:
      Preserved PropertyList could be cleared at the end of the table
      at fo tree parsing (for memory freeing) because it is not needed
      for the layout-managers.
      Should I do this in Table.endOfNode?

      Changes to FromTableColumnFunction.java:
      Details of implementation under discussion at fop-dev.
      Thread: "table-columns and number-columns-spanned"
      "last fallback" and eventually "second fallback" may be
      obsoleted by this discussion.

      1. from-table-column.followup
        0.8 kB
        gerhard oettl
      2. from-table-column.diff
        19 kB
        Andreas L. Delmelle
      3. from-table-column.diff
        19 kB
        Andreas L. Delmelle
      4. dorfchronik.fo
        3 kB
        gerhard oettl
      5. patch.from-table-column
        8 kB
        gerhard oettl

        Activity

        Hide
        gerhard oettl added a comment -

        Attachment patch.from-table-column has been added with description: initial patch

        Show
        gerhard oettl added a comment - Attachment patch.from-table-column has been added with description: initial patch
        Hide
        gerhard oettl added a comment -

        Attachment dorfchronik.fo has been added with description: an example fo-file

        Show
        gerhard oettl added a comment - Attachment dorfchronik.fo has been added with description: an example fo-file
        Hide
        Andreas L. Delmelle added a comment -

        Gerhard,

        I finally got around to taking a look at the column-numbers again. In the end,
        I think the solution was
        pretty straightforward (see attached patch).
        I managed to make the function applicable to table-cells as well... I only
        needed to make sure column-number and number-columns-spanned were treated the
        same way as font-size (see alterations to PropertyList). Maybe in the longer
        term, some rational ordering of the possibly specified properties needs to be
        looked at in a more global sense. (Apart from the above three, are there any
        other cases where the property parsing implies knowing the explicit values for
        other properties, so we need to make sure they are available first?)

        Roughly, I think my patch does more or less the same as yours. Differences are:

        • that I preferred to expose the isColumnNumberUsed() method, instead of using
          iterators over the columns List (a matter of taste? seemed to result in less
          objects being created...)
        • replaced the PropertyList reference in TableColumn with a StaticPropertyList
          (efficiency+)
        • did some cleanup: moved validation/overlapping colnrs to the PropertyMaker

        Let me know if this agrees with you...

        Cheers,

        Andreas

        Show
        Andreas L. Delmelle added a comment - Gerhard, I finally got around to taking a look at the column-numbers again. In the end, I think the solution was pretty straightforward (see attached patch). I managed to make the function applicable to table-cells as well... I only needed to make sure column-number and number-columns-spanned were treated the same way as font-size (see alterations to PropertyList). Maybe in the longer term, some rational ordering of the possibly specified properties needs to be looked at in a more global sense. (Apart from the above three, are there any other cases where the property parsing implies knowing the explicit values for other properties, so we need to make sure they are available first?) Roughly, I think my patch does more or less the same as yours. Differences are: that I preferred to expose the isColumnNumberUsed() method, instead of using iterators over the columns List (a matter of taste? seemed to result in less objects being created...) replaced the PropertyList reference in TableColumn with a StaticPropertyList (efficiency+) did some cleanup: moved validation/overlapping colnrs to the PropertyMaker Let me know if this agrees with you... Cheers, Andreas
        Hide
        Andreas L. Delmelle added a comment -

        Attachment from-table-column.diff has been added with description: alternative patch

        Show
        Andreas L. Delmelle added a comment - Attachment from-table-column.diff has been added with description: alternative patch
        Hide
        Andreas L. Delmelle added a comment -

        Slightly corrected and updated version of the proposed patch.

        Show
        Andreas L. Delmelle added a comment - Slightly corrected and updated version of the proposed patch.
        Hide
        Andreas L. Delmelle added a comment -

        Attachment from-table-column.diff has been added with description: correction...

        Show
        Andreas L. Delmelle added a comment - Attachment from-table-column.diff has been added with description: correction...
        Hide
        Andreas L. Delmelle added a comment -

        OK, after fumbling with it myself... Time for the questions/remarks

        ad 1) agreed. 1a) also sounds attractive. [As I was thinking about moving the column-numbering to the
        PropertyMaker -which turned out to be nearly impossible and quite unnecessary- I also considered
        creating a private inner class to the PropertyMaker, which subclassed TableFObj...]

        ad 2) Indeed! Seems I overlooked that. Table.endOfNode() seems like the right place to clear those
        references.

        ad 3) I think my proposed implementation offers the right approach (though not at all sure; feel free to
        object ), namely:

        • if there is an exact match in column-number, whatever the span, take that column
        • if there is no exact match, try to match any (colnr + s), where s is 1 .. (colspan_cell - 1)

        IMO, this is the way to read the definition in 5.10.4:
        "... from the fo:table-column whose column-number matches the column for which this expression is
        evaluated and whose number-columns-spanned also matches any span. If there is no match for the
        number-columns spanned, it is matched against a span of 1."
        This just seems like a way-too-complicated way of saying that, if there is an exact match, number-
        columns-spanned becomes irrelevant

        Regardless of the debate on how to interpret number-columns-spanned on fo:table-column, the
        column-indices used by column-spanning columns will be flagged as occupied –
        isColumnNumberUsed() will return true--, so the function will work properly in both interpretations.

        Show
        Andreas L. Delmelle added a comment - OK, after fumbling with it myself... Time for the questions/remarks ad 1) agreed. 1a) also sounds attractive. [As I was thinking about moving the column-numbering to the PropertyMaker - which turned out to be nearly impossible and quite unnecessary - I also considered creating a private inner class to the PropertyMaker, which subclassed TableFObj...] ad 2) Indeed! Seems I overlooked that. Table.endOfNode() seems like the right place to clear those references. ad 3) I think my proposed implementation offers the right approach (though not at all sure; feel free to object ), namely: if there is an exact match in column-number, whatever the span, take that column if there is no exact match, try to match any (colnr + s), where s is 1 .. (colspan_cell - 1) IMO, this is the way to read the definition in 5.10.4: "... from the fo:table-column whose column-number matches the column for which this expression is evaluated and whose number-columns-spanned also matches any span. If there is no match for the number-columns spanned, it is matched against a span of 1." This just seems like a way-too-complicated way of saying that, if there is an exact match, number- columns-spanned becomes irrelevant Regardless of the debate on how to interpret number-columns-spanned on fo:table-column, the column-indices used by column-spanning columns will be flagged as occupied – isColumnNumberUsed() will return true--, so the function will work properly in both interpretations.
        Hide
        gerhard oettl added a comment -

        No objections at all. I am very happy with your implemention.

        Show
        gerhard oettl added a comment - No objections at all. I am very happy with your implemention.
        Hide
        gerhard oettl added a comment -

        To apply after the other patch.

        Show
        gerhard oettl added a comment - To apply after the other patch.
        Hide
        gerhard oettl added a comment -

        Attachment from-table-column.followup has been added with description: Remove propertylist when not neccessary anymore

        Show
        gerhard oettl added a comment - Attachment from-table-column.followup has been added with description: Remove propertylist when not neccessary anymore
        Hide
        Manuel Mall added a comment -

        This is a bit of side issue but I noticed that the patch uses the same
        mechanism of early property evaluation as for font-size. However, there is an
        issue with the early evaluation of the font-size property in the context of
        markers. Could we introduce the same problem here for column-numbers?

        Show
        Manuel Mall added a comment - This is a bit of side issue but I noticed that the patch uses the same mechanism of early property evaluation as for font-size. However, there is an issue with the early evaluation of the font-size property in the context of markers. Could we introduce the same problem here for column-numbers?
        Hide
        Andreas L. Delmelle added a comment -

        Hi Manuel,

        I think it doesn't apply in this particular case... The problem with font-size, IIRC, concerns retrieved
        markers Should be no problem here. Column-number 3 will always remain column-number 3,
        regardless of whether the table is contained in a marker or not.

        I don't know precisely -will have to look it up- whether a marker that is in column-number 3 in a
        table descending from the flow, can be retrieved into a table in the static-content and thus end up in a
        different column...

        Cheers,

        Andreas

        Show
        Andreas L. Delmelle added a comment - Hi Manuel, I think it doesn't apply in this particular case... The problem with font-size, IIRC, concerns retrieved markers Should be no problem here. Column-number 3 will always remain column-number 3, regardless of whether the table is contained in a marker or not. I don't know precisely - will have to look it up - whether a marker that is in column-number 3 in a table descending from the flow, can be retrieved into a table in the static-content and thus end up in a different column... Cheers, Andreas
        Hide
        Andreas L. Delmelle added a comment -

        OK, just looked it up, and it seems like it is possible --although I wouldn't dream of it myself to do
        so, that's another issue.

        Then again: this problem will always pose itself, and it is not so much a matter of 'early property
        evaluation'. All properties are evaluated early, and some need to come first because of possible
        dependencies of other specified properties (i.e. relative font-sizes or core function calls). In itself, this
        isn't wrong, but the solution may be to postpone the whole property evaluation cycle for a marker and
        all of its descendants until the marker is retrieved (since we cannot possibly know how inherited
        properties specified on the retrieve-marker or its containing block will affect the properties of the
        markers and its descendants... Maybe this is another case where we absolutely need to keep a reference
        to the original PropertyLists instead of binding the individual properties to instance variables

        I'll apply the patch as is, with a few added comments indicating that markers still need to be treated
        with caution.

        Cheers,

        Andreas

        Show
        Andreas L. Delmelle added a comment - OK, just looked it up, and it seems like it is possible --although I wouldn't dream of it myself to do so, that's another issue. Then again: this problem will always pose itself, and it is not so much a matter of 'early property evaluation'. All properties are evaluated early, and some need to come first because of possible dependencies of other specified properties (i.e. relative font-sizes or core function calls). In itself, this isn't wrong, but the solution may be to postpone the whole property evaluation cycle for a marker and all of its descendants until the marker is retrieved (since we cannot possibly know how inherited properties specified on the retrieve-marker or its containing block will affect the properties of the markers and its descendants... Maybe this is another case where we absolutely need to keep a reference to the original PropertyLists instead of binding the individual properties to instance variables I'll apply the patch as is, with a few added comments indicating that markers still need to be treated with caution. Cheers, Andreas
        Hide
        Glenn Adams added a comment -

        batch transition pre-FOP1.0 resolved+fixed bugs to closed+fixed

        Show
        Glenn Adams added a comment - batch transition pre-FOP1.0 resolved+fixed bugs to closed+fixed

          People

          • Assignee:
            fop-dev
            Reporter:
            gerhard oettl
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development