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

The first topicref to a ditabase file always pulls in the first topic #1904

Closed
mnordseth opened this Issue Apr 30, 2015 · 8 comments

Comments

Projects
None yet
5 participants
@mnordseth

mnordseth commented Apr 30, 2015

I have a ditabase file with several topics:

<!DOCTYPE dita PUBLIC "-//OASIS//DTD DITA Composite//EN" "ditabase.dtd" >
<dita>
  <reference id="t1">
    <title>Topic 1</title>
  </reference>

  <reference id="t2">
    <title>Topic 2</title>
  </reference>

  <reference id="t3">
    <title>Topic 3</title>
  </reference>
</dita>

And a map that pulls in those topics:

<?xml version='1.0' encoding='UTF-8'?>
<!DOCTYPE map PUBLIC "-//OASIS//DTD DITA Map//EN" "map.dtd">
<map>
  <topicref href="test.dita#t2"/>
  <topicref href="test.dita#t1"/>
  <topicref href="test.dita#t3"/>
</map>

The output will always put topic t1 first, while the remaining topics are in the expected order.

@jelovirt

This comment has been minimized.

Show comment
Hide comment
@jelovirt

jelovirt Apr 30, 2015

Member

Which DITA-OT version, which transtype?

Member

jelovirt commented Apr 30, 2015

Which DITA-OT version, which transtype?

@mnordseth

This comment has been minimized.

Show comment
Hide comment
@mnordseth

mnordseth Apr 30, 2015

DITA-OT 1.8.5, transtype pdf, pdf2 and pdf5 (AntennaHouse).

mnordseth commented Apr 30, 2015

DITA-OT 1.8.5, transtype pdf, pdf2 and pdf5 (AntennaHouse).

@robander

This comment has been minimized.

Show comment
Hide comment
@robander

robander Apr 30, 2015

Member

PDF has always had problems with the composite topic. Per the spec, a reference to the file (like href="all.dita" should bring in all of the topics, but has only ever pulled in the first - think we have a very old issue open for that. I'm pretty sure this comes from a problem in how the merged file is created.

Member

robander commented Apr 30, 2015

PDF has always had problems with the composite topic. Per the spec, a reference to the file (like href="all.dita" should bring in all of the topics, but has only ever pulled in the first - think we have a very old issue open for that. I'm pretty sure this comes from a problem in how the merged file is created.

@eerohele eerohele added bug pdf labels May 27, 2015

@eerohele

This comment has been minimized.

Show comment
Hide comment
@eerohele

eerohele May 30, 2015

Member

Fixtures.

I believe the issue is that when determining the ID of the first topic in the topicref target file, MergeMapParser.java always gets the ID of the first topic in the target file, rather than using the ID defined in the @href attribute of the topicref itself.

So, to use my files as an example, MergeMapParser.java reads root.ditamap and when it comes across <topicref href="topic1.dita#topic2"/>, it opens topic1.dita, looks inside and sees that #topic1 is the first topic in that file.

It then decides #topic1 is the ID of the first topic in that file, rather than using #topic2, which is defined in the @href attribute of the <topicref> element itself.

One possible fix is something like this:

diff --git a/src/main/java/org/dita/dost/reader/MergeMapParser.java b/src/main/java/org/dita/dost/reader/MergeMapParser.java
index cf5df80..f09af06 100644
--- a/src/main/java/org/dita/dost/reader/MergeMapParser.java
+++ b/src/main/java/org/dita/dost/reader/MergeMapParser.java
@@ -56,6 +56,8 @@ public final class MergeMapParser extends XMLFilterImpl {
     private File dirPath = null;
     private File tempdir = null;

+    private boolean isFirstTopicRef = true;
+
     private final Stack<String> processStack;
     private int processLevel;
     private final ByteArrayOutputStream topicBuffer;
@@ -207,7 +209,20 @@ public final class MergeMapParser extends XMLFilterImpl {
                         final File f = new File(dirPath, toFile(p).getPath());
                         if (f.exists()) {
                             topicParser.parse(toFile(p).getPath(), dirPath);
-                            final String fileId = topicParser.getFirstTopicId();
+                            final String fileId;
+
+                            // If this is the first topicref and its @href
+                            // attribute has an ID, use that ID as the first
+                            // topic ID.
+                            //
+                            // Otherwise, use the ID of the first topic in
+                            // the topicref target.
+                            if (isFirstTopicRef && attValue.getFragment() != null) {
+                                fileId = util.getIdValue(attValue);
+                            } else {
+                                fileId = topicParser.getFirstTopicId();
+                            }
+
                             util.addId(attValue, fileId);
                             if (attValue.getFragment() != null) {
                                 util.addId(stripFragment(attValue), fileId);
@@ -227,6 +242,8 @@ public final class MergeMapParser extends XMLFilterImpl {
                     }
                 XMLUtils.addOrSetAttribute(atts, ATTRIBUTE_NAME_HREF, attValue.toString());
             }
+
+            isFirstTopicRef = false;
         }
         getContentHandler().startElement(uri, localName, qName, atts != null ? atts : attributes);
     }

@jelovirt: Let me know if you'd like a PR.

Member

eerohele commented May 30, 2015

Fixtures.

I believe the issue is that when determining the ID of the first topic in the topicref target file, MergeMapParser.java always gets the ID of the first topic in the target file, rather than using the ID defined in the @href attribute of the topicref itself.

So, to use my files as an example, MergeMapParser.java reads root.ditamap and when it comes across <topicref href="topic1.dita#topic2"/>, it opens topic1.dita, looks inside and sees that #topic1 is the first topic in that file.

It then decides #topic1 is the ID of the first topic in that file, rather than using #topic2, which is defined in the @href attribute of the <topicref> element itself.

One possible fix is something like this:

diff --git a/src/main/java/org/dita/dost/reader/MergeMapParser.java b/src/main/java/org/dita/dost/reader/MergeMapParser.java
index cf5df80..f09af06 100644
--- a/src/main/java/org/dita/dost/reader/MergeMapParser.java
+++ b/src/main/java/org/dita/dost/reader/MergeMapParser.java
@@ -56,6 +56,8 @@ public final class MergeMapParser extends XMLFilterImpl {
     private File dirPath = null;
     private File tempdir = null;

+    private boolean isFirstTopicRef = true;
+
     private final Stack<String> processStack;
     private int processLevel;
     private final ByteArrayOutputStream topicBuffer;
@@ -207,7 +209,20 @@ public final class MergeMapParser extends XMLFilterImpl {
                         final File f = new File(dirPath, toFile(p).getPath());
                         if (f.exists()) {
                             topicParser.parse(toFile(p).getPath(), dirPath);
-                            final String fileId = topicParser.getFirstTopicId();
+                            final String fileId;
+
+                            // If this is the first topicref and its @href
+                            // attribute has an ID, use that ID as the first
+                            // topic ID.
+                            //
+                            // Otherwise, use the ID of the first topic in
+                            // the topicref target.
+                            if (isFirstTopicRef && attValue.getFragment() != null) {
+                                fileId = util.getIdValue(attValue);
+                            } else {
+                                fileId = topicParser.getFirstTopicId();
+                            }
+
                             util.addId(attValue, fileId);
                             if (attValue.getFragment() != null) {
                                 util.addId(stripFragment(attValue), fileId);
@@ -227,6 +242,8 @@ public final class MergeMapParser extends XMLFilterImpl {
                     }
                 XMLUtils.addOrSetAttribute(atts, ATTRIBUTE_NAME_HREF, attValue.toString());
             }
+
+            isFirstTopicRef = false;
         }
         getContentHandler().startElement(uri, localName, qName, atts != null ? atts : attributes);
     }

@jelovirt: Let me know if you'd like a PR.

@jelovirt

This comment has been minimized.

Show comment
Hide comment
@jelovirt

jelovirt Jun 7, 2015

Member

This will not work when the map contains multiple topicrefs to ditabase files. The implementation cannot rely on first occurrence for all topicrefs.

Member

jelovirt commented Jun 7, 2015

This will not work when the map contains multiple topicrefs to ditabase files. The implementation cannot rely on first occurrence for all topicrefs.

@robander

This comment has been minimized.

Show comment
Hide comment
@robander

robander May 25, 2016

Member

I ran into this behavior again when trying to work on #1077 last week (currently our oldest open issue). Both issues are related, though they address different bugs.

Member

robander commented May 25, 2016

I ran into this behavior again when trying to work on #1077 last week (currently our oldest open issue). Both issues are related, though they address different bugs.

@markgif

This comment has been minimized.

Show comment
Hide comment
@markgif

markgif Sep 20, 2016

I think I ran into this with a different setup than the one @mnordseth shows. Here is my ditabase file with two embedded topics:

<dita>
  <topic id="t1">
    <title>Topic title BAD</title>
    <body>
      <p>Here is topic text.</p>
    </body>
  </topic>
  <concept id="c1">
    <title>Concept 1 title</title>
    <conbody>
      <p>Concept text.</p>
    </conbody>
  </concept>
</dita>

Here is my map, which references the ditabase file itself, not a topic inside the ditabase:

<map>
 <title>map</title>
  <topicref href="ditabase-test.dita"/>
</map>

With the OT 1.8.5 pdf2 transtype, only the first topic appears in the PDF. But if I embed the second topic inside the first one, then it works in the PDF:

<dita>
  <topic id="t1">
    <title>Topic title BAD</title>
    <body>
      <p>Here is topic text.</p>
    </body>
    <concept id="c1">
      <title>Concept 1 title</title>
      <conbody>
        <p>Concept text.</p>
      </conbody>
    </concept>
  </topic>
</dita>

markgif commented Sep 20, 2016

I think I ran into this with a different setup than the one @mnordseth shows. Here is my ditabase file with two embedded topics:

<dita>
  <topic id="t1">
    <title>Topic title BAD</title>
    <body>
      <p>Here is topic text.</p>
    </body>
  </topic>
  <concept id="c1">
    <title>Concept 1 title</title>
    <conbody>
      <p>Concept text.</p>
    </conbody>
  </concept>
</dita>

Here is my map, which references the ditabase file itself, not a topic inside the ditabase:

<map>
 <title>map</title>
  <topicref href="ditabase-test.dita"/>
</map>

With the OT 1.8.5 pdf2 transtype, only the first topic appears in the PDF. But if I embed the second topic inside the first one, then it works in the PDF:

<dita>
  <topic id="t1">
    <title>Topic title BAD</title>
    <body>
      <p>Here is topic text.</p>
    </body>
    <concept id="c1">
      <title>Concept 1 title</title>
      <conbody>
        <p>Concept text.</p>
      </conbody>
    </concept>
  </topic>
</dita>

@robander robander added this to the 2.5 milestone Jun 30, 2017

@robander

This comment has been minimized.

Show comment
Hide comment
@robander

robander Jun 30, 2017

Member

The issue here was the one Eero described (grabbing the first root topic's ID for all cases), and was resolved by #2679. Tagging this with 2.5 and closing.

Member

robander commented Jun 30, 2017

The issue here was the one Eero described (grabbing the first root topic's ID for all cases), and was resolved by #2679. Tagging this with 2.5 and closing.

@robander robander closed this Jun 30, 2017

infotexture added a commit to dita-ot/docs that referenced this issue Jun 30, 2017

Add link to dita-ot/dita-ot#1904
Signed-off-by: Roger Sheen <roger@infotexture.net>

infotexture added a commit to dita-ot/docs that referenced this issue Jun 30, 2017

Merge branch 'hotfix/2.5.2' into develop
* hotfix/2.5.2:
  Add link to dita-ot/dita-ot#1904
  Remove unbundled plugins from output format list

infotexture added a commit to dita-ot/docs that referenced this issue Aug 1, 2017

Merge branch 'hotfix/2.5.2'
* hotfix/2.5.2:
  Minor copy editing for 2.5.2 Release Notes
  Draft notes for 2 fixes in 2.5.2
  Clean up resources listings
  Link to DITA-OT Slack team (Fixes #146)
  Prepare 2.5.2 Release Notes stub
  Bump 'maintenance-version' key to “2.5.2”
  Use latest DITA-OT version (2.5.1) for CI builds
  Fix typo in issue link
  Add link to dita-ot/dita-ot#1904
  Remove unbundled plugins from output format list
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment