Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A preserveOrder= of "false" does nothing #241

Closed
sydb opened this issue Mar 12, 2017 · 5 comments
Closed

A preserveOrder= of "false" does nothing #241

sydb opened this issue Mar 12, 2017 · 5 comments
Labels
conversion: odd conversion: relax resp: council status: needsDiscussion Council has not yet been able to agree on how to proceed. type: bug A bug report.

Comments

@sydb
Copy link
Member

sydb commented Mar 12, 2017

In an ODD, <sequence preserveOrder="false"> produces the same RELAX NG as <sequence> (or <sequence preserveOrder="true">).

For testing, try this tiny ODD that tries to constraint <choice> to contain 1 <expan> and 1 <abbr> in either order. Then try it changing "false" to "true". (Note that, to appease GitHub, I have added a “.txt” extension to the filename.)

@sydb sydb added the type: bug A bug report. label Mar 12, 2017
@sydb
Copy link
Member Author

sydb commented Feb 7, 2020

N.B. TEI issue #1970.

@martindholmes
Copy link
Contributor

Pinging this ticket -- surely we need to get this fixed? It's currently not possible (as far as I can see) to create a content model which may contain zero or one of each of a list of elements in any order.

@joeytakeda
Copy link
Contributor

I've recently been bitten by this bug and confirm it's still happening.

I've done a bit of digging in the Stylesheets and it looks like the root of the problem (or, at least, the initial problem) is in odd2odd.xsl. Running this command against the dev branch of the Stylesheets (where preserveOrder.odd is the ODD @sydb provided):

saxon -s:preserveOrder.odd -xsl:../Stylesheets/odds/odd2odd.xsl -o:preserveOrder_expanded.odd

Shows a <sequence> for content that has lost its @preserveOrder entirely

          <content>
            <sequence minOccurs="1" maxOccurs="1">
              <elementRef key="abbr"/>
              <elementRef key="expan"/>
            </sequence>
          </content>

I think the problem is in the odd2odd-simplifyODD template and, in particular, this condition:

Stylesheets/odds/odd2odd.xsl

Lines 1489 to 1493 in 9600186

<xsl:when test="$entCount gt 0 or $stripped='true'">
<xsl:element namespace="http://www.tei-c.org/ns/1.0" name="{$element}">
<xsl:attribute name="minOccurs" select="$min"/>
<xsl:attribute name="maxOccurs" select="if ($max eq -1) then 'unbounded' else $max"/>
<xsl:copy-of select="@*|*|text()|processing-instruction()"/>

Only the @minOccurs and @maxOccurs is retained from the context element and all other attributes are ignored; just did a quick test by adding @xml:id to the <sequence> and it's also absent from preserveOrder_expanded.odd.

I'm happy to work on this in a fork and file a PR, if that would be helpful!

joeytakeda added a commit to joeytakeda/Stylesheets that referenced this issue May 21, 2021
* Copy over the context attributes (except (min|max)Occurs) in odd2odd-simplifyODD in order to retain attributes (like preserveOrder) declared on the source element
joeytakeda added a commit to joeytakeda/Stylesheets that referenced this issue May 22, 2021
* In inputFiles/testPure1.odd: Constrain choice such that it must contain one of:
** abbr and expan
** expan and abbr
** corr and sic
** orig and reg

* Adding tests to both validInstance and invalidInstances for testPure1
* Updating the RNG and the RNG output files
@joeytakeda
Copy link
Contributor

The problem was indeed in odd2odd.xsl as far as I can tell. teiodds.xsl (imported byodd2relax.xsl) already has handling for preserveOrder:

Stylesheets/odds/teiodds.xsl

Lines 2078 to 2104 in 9600186

<xsl:template match="tei:sequence" mode="#default tangle">
<!-- "owe" = occurence wrapper element -->
<xsl:variable name="owe" select="tei:generateIndicators(@minOccurs,@maxOccurs)"/>
<!-- sequences of <dataRef> need use <list>, not <group> -->
<xsl:variable name="group_or_list" select="if ( *[ not( self::tei:dataRef ) ] ) then 'group' else 'list'"/>
<xsl:choose>
<xsl:when test="@preserveOrder eq 'false' and string-length($owe) eq 0">
<xsl:element name="{$group_or_list}" namespace="http://relaxng.org/ns/structure/1.0">
<rng:interleave>
<xsl:apply-templates mode="tangle"/>
</rng:interleave>
</xsl:element>
</xsl:when>
<xsl:when test="string-length($owe) eq 0">
<xsl:element name="{$group_or_list}" namespace="http://relaxng.org/ns/structure/1.0">
<xsl:apply-templates mode="tangle"/>
</xsl:element>
</xsl:when>
<xsl:otherwise>
<xsl:element name="{$owe}" namespace="http://relaxng.org/ns/structure/1.0">
<xsl:element name="{$group_or_list}" namespace="http://relaxng.org/ns/structure/1.0">
<xsl:apply-templates mode="tangle"/>
</xsl:element>
</xsl:element>
</xsl:otherwise>
</xsl:choose>
</xsl:template>

So copying over the attributes in odd2odd.xsl seems to fix the issue, preserving @preserveOrder so that it's recognized by the time the ODD hits the above template.

I've forked the repo and made the change to odd2odd.xsl (will do a PR once I post this comment) as well as added some tests to Test2. I wasn't sure if that was the proper way to add a test--I couldn't find any documentation on how to add tests to Test, but the documentation for Test2 was clear, so I just went ahead and did that; let me know if that's actually helpful and, if not, happy to remove the changes to Test2.

sydb pushed a commit that referenced this issue Jul 17, 2022
* Work on #241

* Copy over the context attributes (except (min|max)Occurs) in odd2odd-simplifyODD in order to retain attributes (like preserveOrder) declared on the source element

* Adding tests for preserveOrder (#241)

* In inputFiles/testPure1.odd: Constrain choice such that it must contain one of:
** abbr and expan
** expan and abbr
** corr and sic
** orig and reg

* Adding tests to both validInstance and invalidInstances for testPure1
* Updating the RNG and the RNG output files

* Fixes as requested by @sydb in PR #507

* Fixing prose (#507 (comment))
* Test nodes, not names (#507 (comment))
@sydb
Copy link
Member Author

sydb commented Jul 17, 2022

Merged in #507, and no errors on Test or Test2. Yay!
Thanks @joeytakeda!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conversion: odd conversion: relax resp: council status: needsDiscussion Council has not yet been able to agree on how to proceed. type: bug A bug report.
Projects
None yet
Development

No branches or pull requests

5 participants