Fix index link when term has subterms #3320; fix index-see-also #3314 #3373

robander · 2019-10-03T21:56:31Z

Signed-off-by: Robert D Anderson robander@us.ibm.com

name: Pull request
about: Propose changes to fix an issue or implement a new feature

Description

Current PDF output will drop all instances of an index term from the index, if even one instance of the term has a sub-term. This is done silently, so adding one secondary or tertiary term can erase any number of page links for the primary term.

Doing this ignores what the actual markup says, and might not even be the original intent (some of the code design is unclear).

Some indexing tools follow a practice that when there are child terms, the author should not index the primary; however, our code should respect the markup and leave that restriction to local rules.

The update here adds a variable $index.allow-link-with-subterm which defaults to true(); if set to false() it will ignore the links on the parent term as we do today. However, that should not be the default.

Motivation and Context

Fixes #3320
Fixes #3314

How Has This Been Tested?

Tested with the content in 3320; using the default true() setting keeps the expected links, while setting to false() removes them.

Type of Changes

Bug fix (non-breaking change which fixes an issue)

Signed-off-by: Robert D Anderson <robander@us.ibm.com>

robander · 2019-10-03T21:59:52Z

Not yet ready for review - still need to remove any page link for the primary term when it does specify a secondary.

Signed-off-by: Robert D Anderson <robander@us.ibm.com>

robander · 2019-10-04T20:49:15Z

Updated and added a better / more complete test case; tested for AH and FOP output.
3320.zip

To validate output when publishing 3320.ditamap:

EVERYWHERE is only a primary term, should have page links. (FOP doesn't remove duplicates so I get one page twice.)
Issue3320 should have one page link because used as primary once; it should have 2 sub-terms each with a page link
Only subterms should not have any page links, only subterms
PrimaryA should have two page links and one child with a page link
PrimaryB should have one page link and one child with a page link
SeeALSOTest should have a page link, with the See also
SeeTest should not have a page link

Signed-off-by: Robert D Anderson <robander@us.ibm.com>

robander · 2019-10-05T02:55:22Z

Adding to this pull request a fix for #3314.

If a term has more than one index-see-also, current processing ignores all but the first. Latest commit fixes that issue by preserving all see-also instances, separating them with a comma. If the two "see also" references are to IMPORTANT and MORE IMPORTANT, current processing will generate "See also IMPORTANT". With this fix, it will generate "See also IMPORTANT, MORE IMPORTANT"`. Each see-also target is linked correctly.
If a term has more than one index-see, current processing ignores all but the first. Latest commit fixes that issue by preserving all see instances, separating them with a comma. If the two "see" references are to IMPORTANT and MORE IMPORTANT, current processing will generate "See IMPORTANT" after the term. With this fix, it will generate "See IMPORTANT, MORE IMPORTANT". Each see-also target is linked correctly. A better long term fix would be to convert one or all of these to see-also but it didn't seem like it was worth going that far (particularly having to handle the case of a term with both index-seeandindex-see-also`.

Attaching test cases for #3314.
3314.zip

infotexture

Testing with FOP

EVERYWHERE is only a primary term, should have page links. (FOP doesn't remove duplicates so I get one page twice.) ✅

Issue3320 should have one page link because used as primary once; it should have 2 sub-terms each with a page link

❌ 3 page links (sub-term page links also appear in parent)

Only subterms should not have any page links, only subterms ✅

PrimaryA should have two page links and one child with a page link

❌ 3 page links (sub-term page link also appears in parent)

PrimaryB should have one page link and one child with a page link

❌ I get PrimaryC with 2 page links (sub-term page link also appears in parent)

SeeALSOTest should have a page link, with the See also ✅

SeeTest should not have a page link ✅

infotexture · 2019-10-05T15:22:43Z

Output files from FOP test above:

infotexture · 2019-10-05T15:29:10Z

The update here adds a variable $index.allow-link-with-subterm which defaults to true(); if set to false() it will ignore the links on the parent term as we do today. However, that should not be the default.

@robander Would users be able to adjust this default by passing a parameter on the CLI, or would changing this require an XSLT customization plug-in?

@lief-erickson With this change, the current docs index in dita-ot/docs#264 would show a large number of (redundant) page numbers for certain parent entries:

lief-erickson · 2019-10-07T03:28:53Z

@infotexture, that screen capture certainly exposes the duplicate page number issue in FOP, doesn't it?

robander · 2019-10-07T13:36:55Z

@infotexture not sure why you are getting page links on the primary term when the secondary is indexed. That's not what I see so I need to double check if something wasn't checked in.

As currently defined this is not a CLI option - to me, this feels way too specific an option to expose on the CLI. Especially for this case -- the only reason to suppress them is if you have a rule that primary terms cannot be indexed when secondary terms are present, but in that case, I think our default should still be "Do what the markup says and if you don't like it, you can update the markup per your local rules"

And the duplicate page numbers -- yeah, that has always been the case with FOP output, just wasn't exposed for us in this case because we ignored all of those entries.

jelovirt · 2019-10-08T15:24:01Z

src/main/plugins/org.dita.pdf2.xep/xsl/fo/index_xep.xsl

@@ -116,6 +116,9 @@ See the accompanying LICENSE file for applicable license.
                        </xsl:variable>
                        <xsl:if test="contains($isNormalChilds,'true ')">
                          <xsl:apply-templates select="." mode="make-index-ref">
+                            <xsl:with-param name="idxs" select="if ($index.allow-link-with-subterm and exists(key('index-leaves',@value)))
+                              then (opentopic-index:refID)


Suggested change

then (opentopic-index:refID)

then opentopic-index:refID

jelovirt · 2019-10-08T15:24:56Z

src/main/plugins/org.dita.pdf2/xsl/fo/index.xsl

+  <xsl:key name="index-leaves"
+    match="opentopic-index:index.entry
+              [empty(opentopic-index:index.entry|opentopic-index:see-childs)]
+              [not(ancestor::opentopic-index:index.group)]" 


Suggested change

[not(ancestor::opentopic-index:index.group)]"

[empty(ancestor::opentopic-index:index.group)]"

jelovirt · 2019-10-08T15:25:43Z

src/main/plugins/org.dita.pdf2/xsl/fo/index.xsl

@@ -220,16 +228,16 @@ See the accompanying LICENSE file for applicable license.

    <xsl:template match="opentopic-index:index.entry[not(opentopic-index:index.entry)]" mode="index-postprocess" priority="1">
        <xsl:variable name="page-setting" select=" (ancestor-or-self::opentopic-index:index.entry/@no-page | ancestor-or-self::opentopic-index:index.entry/@start-page)[last()]"/>
-    <xsl:variable name="isNoPage" select=" $page-setting = 'true' and name($page-setting) = 'no-page' "/>
+        <xsl:variable name="isNoPage" select=" $page-setting = 'true' and name($page-setting) = 'no-page' "/>


Suggested change

<xsl:variable name="isNoPage" select=" $page-setting = 'true' and name($page-setting) = 'no-page' "/>

<xsl:variable name="isNoPage" select="$page-setting = 'true' and name($page-setting) = 'no-page'"/>

Does this really work? Because

<xsl:variable name="page-setting" select="(ancestor-or-self::opentopic-index:index.entry/@no-page | ancestor-or-self::opentopic-index:index.entry/@start-page) [last()]"/>

type is (@no-page | @start-page)? and the name() function will never return no-page.

Honestly - it looks like no, for the reason you describe. I didn't touch that code and haven't tested it. (Didn't even examine it while working on the rest of this.) I'm guessing it's related to page ranges, which I think have other open issues.

I've fixed the spacing on the variable declaration but would rather leave this alone until I have a test case that would demonstrate whether it works or not - I think you're right that it shouldn't work, but ...

jelovirt · 2019-10-08T15:30:50Z

src/main/plugins/org.dita.pdf2/xsl/fo/index.xsl

-                    <xsl:apply-templates select="opentopic-index:index.entry[1]" mode="get-see-value"/>
-                </fo:basic-link>
+                <xsl:for-each select="opentopic-index:index.entry">
+                    <xsl:if test="not(position() eq 1)">


Suggested change

<xsl:if test="not(position() eq 1)">

<xsl:if test="position() ne 1">

jelovirt · 2019-10-08T15:31:19Z

src/main/plugins/org.dita.pdf2/xsl/fo/index.xsl

-                        <xsl:apply-templates select="opentopic-index:index.entry[1]" mode="get-see-value"/>
-                    </fo:basic-link>
+                    <xsl:for-each select="opentopic-index:index.entry">
+                        <xsl:if test="not(position() eq 1)">


Suggested change

<xsl:if test="not(position() eq 1)">

<xsl:if test="position() ne 1">

jelovirt · 2019-10-08T15:32:02Z

src/main/plugins/org.dita.pdf2/xsl/fo/index.xsl

@@ -298,8 +316,10 @@ See the accompanying LICENSE file for applicable license.
    <xsl:template match="opentopic-index:index.entry" mode="get-see-value">
        <fo:inline>
            <xsl:apply-templates select="opentopic-index:formatted-value/node()"/>
-            <xsl:text> </xsl:text>
-            <xsl:apply-templates select="opentopic-index:index.entry[1]" mode="get-see-value"/>
+            <xsl:if test="opentopic-index:index.entry[1]">


Suggested change

<xsl:if test="opentopic-index:index.entry[1]">

<xsl:if test="exists(opentopic-index:index.entry)">

IMO using exists() is more readable.

jelovirt · 2019-10-08T15:32:24Z

src/main/plugins/org.dita.pdf2/xsl/fo/index.xsl

-                <xsl:apply-templates select="opentopic-index:index.entry[1]" mode="get-see-value"/>
-            </fo:basic-link>
+            <xsl:for-each select="opentopic-index:index.entry">
+                <xsl:if test="not(position() eq 1)">


Suggested change

<xsl:if test="not(position() eq 1)">

<xsl:if test="position() ne 1">

jelovirt · 2019-10-08T15:32:39Z

src/main/plugins/org.dita.pdf2/xsl/fo/index.xsl

@@ -371,6 +396,9 @@ See the accompanying LICENSE file for applicable license.
                        </xsl:variable>
                        <xsl:if test="contains($isNormalChilds,'true ')">
                          <xsl:apply-templates select="." mode="make-index-ref">
+                            <xsl:with-param name="idxs" select="if ($index.allow-link-with-subterm and exists(key('index-leaves',@value))) 
+                              then (opentopic-index:refID)


Suggested change

then (opentopic-index:refID)

then opentopic-index:refID

robander · 2019-10-10T00:56:15Z

@infotexture testing again with code built from github and I reproduced the results you got with FOP -- investigating. [Edit: fixed with next commit.]

Signed-off-by: Robert D Anderson <robander@us.ibm.com>

lief-erickson · 2019-10-10T03:49:31Z

Tested with FOP:

Using the DITA-OT User Guide 1393f62b. The dropped see-also terms are present. That is fixed.

I am seeing primary entries that have sub-terms show page numbers. Several examples: Ant, dita command, DITAVAL, and many more. This is good.

In the case of DITAVAL, which is one example of several, the page numbers shown after the primary term are also shown after a sub-term. After some testing it appears as though the <index-see-also> markup is contributing a page number to the primary term. This doesn't seem correct to me.

Both of these entries below contribute a page locator to the primary DITAVAL entry (even the second one). I would expect no page locator and the term (clickable) in the "see also" list (for the relevant primary term).

<indexterm>DITAVAL<index-see-also>filters</index-see-also></indexterm>
<indexterm>profiling<index-see-also>DITAVAL</index-see-also></indexterm>

Using a private test document that showed results similar to the DITA-OT User Guide.

robander · 2019-10-10T04:09:31Z

@lief-erickson That was not my understanding of index-see-also, based on this bit from the spec: http://docs.oasis-open.org/dita/dita/v1.3/errata02/os/complete/part1-base/langRef/base/index-see-also.html

In addition to its "see also" redirection, an <index-see-also> functions as a pointwise index term, thereby typically generating a page reference as well as the "see also" indication.

As I've understood the specification and the implementation -- "see also" means "Index this point, and also put out a pointer to other terms that may be useful"; "index-see" does not put out a point, and is purely a redirect.

EDIT: wait I misunderstood part of your comment. With these two terms, I would expect the first to contribute a page link to DITAVAL, but not the second:
<indexterm>DITAVAL<index-see-also>filters</index-see-also></indexterm>
<indexterm>profiling<index-see-also>DITAVAL</index-see-also></indexterm>

lief-erickson · 2019-10-10T04:12:24Z

@robander, you're right. The spec does indeed say otherwise. It's not my expectation, but the output does match the spec.

Edit: it appears there's wiggle room because of the word "typically." I would typically expect no pager numbers, but apparently someone else might.

Yes, even the second entry adds a page locator, which I found definitely odd. The first one I could possibly understand, but not the second.

robander · 2019-10-10T12:38:52Z

@lief-erickson thanks for testing, and highlighting that issue. I've updated my test case to add this in the first topic:
<indexterm>ChildA</indexterm>
and these in the second topic:
<indexterm>ChildB<index-see-also>ChildA</index-see-also></indexterm>
<indexterm>ChildBSee<index-see>ChildA</index-see></indexterm>

As you noted -- both of those entries in the second topic incorrectly added page links to "ChildA", going to that second topic. I've got an update ready so that only the entry in the first topic creates a page link to "ChildA".

Signed-off-by: Robert D Anderson <robander@us.ibm.com>

Keep index links when term has subterms dita-ot#3320

0927009

Signed-off-by: Robert D Anderson <robander@us.ibm.com>

robander added bug plugin/pdf Issue related to PDF plug-in priority/medium Medium (or unknown) priority issue index Issue related to index sorting or rendering labels Oct 3, 2019

robander self-assigned this Oct 3, 2019

Hide idx link when subterm used dita-ot#3320

17ece80

Signed-off-by: Robert D Anderson <robander@us.ibm.com>

robander changed the title ~~Keep index links when term has subterms #3320~~ Fix index link when term has subterms #3320; fix index-see-also #3314 Oct 5, 2019

Restore dropped see/see-also entries dita-ot#3314

c4ecf0b

Signed-off-by: Robert D Anderson <robander@us.ibm.com>

robander mentioned this pull request Oct 5, 2019

PDF index <index-see-also> and <index-see> issues #3314

Closed

infotexture requested changes Oct 5, 2019

View reviewed changes

jelovirt requested changes Oct 8, 2019

View reviewed changes

Robert D Anderson added 2 commits October 9, 2019 21:11

Fix links for primary with subterms dita-ot#3373

1675602

Signed-off-by: Robert D Anderson <robander@us.ibm.com>

Optimize code, per dita-ot#3373 code review

532d322

Signed-off-by: Robert D Anderson <robander@us.ibm.com>

jelovirt previously approved these changes Oct 10, 2019

View reviewed changes

Remove extra page link from see, see-also dita-ot#3373

dc93c82

Signed-off-by: Robert D Anderson <robander@us.ibm.com>

robander dismissed jelovirt’s stale review via dc93c82 October 10, 2019 12:41

jelovirt approved these changes Oct 10, 2019

View reviewed changes

infotexture approved these changes Oct 10, 2019

View reviewed changes

robander merged commit d529c79 into dita-ot:develop Oct 10, 2019

robander deleted the hotfix/3320 branch October 10, 2019 14:10

robander added this to the 3.4 milestone Oct 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix index link when term has subterms #3320; fix index-see-also #3314 #3373

Fix index link when term has subterms #3320; fix index-see-also #3314 #3373

robander commented Oct 3, 2019 •

edited by jelovirt

Loading

robander commented Oct 3, 2019

robander commented Oct 4, 2019

robander commented Oct 5, 2019

infotexture left a comment

infotexture commented Oct 5, 2019

infotexture commented Oct 5, 2019

lief-erickson commented Oct 7, 2019

robander commented Oct 7, 2019

jelovirt Oct 8, 2019

jelovirt Oct 8, 2019

jelovirt Oct 8, 2019

jelovirt Oct 8, 2019

robander Oct 10, 2019

robander Oct 10, 2019

jelovirt Oct 8, 2019

jelovirt Oct 8, 2019

jelovirt Oct 8, 2019

jelovirt Oct 8, 2019

jelovirt Oct 8, 2019

robander commented Oct 10, 2019 •

edited

Loading

lief-erickson commented Oct 10, 2019

robander commented Oct 10, 2019 •

edited

Loading

lief-erickson commented Oct 10, 2019 •

edited

Loading

robander commented Oct 10, 2019

	[not(ancestor::opentopic-index:index.group)]"
	[empty(ancestor::opentopic-index:index.group)]"

	<xsl:variable name="isNoPage" select=" $page-setting = 'true' and name($page-setting) = 'no-page' "/>
	<xsl:variable name="isNoPage" select="$page-setting = 'true' and name($page-setting) = 'no-page'"/>

	<xsl:if test="not(position() eq 1)">
	<xsl:if test="position() ne 1">

	<xsl:if test="opentopic-index:index.entry[1]">
	<xsl:if test="exists(opentopic-index:index.entry)">

Fix index link when term has subterms #3320; fix index-see-also #3314 #3373

Fix index link when term has subterms #3320; fix index-see-also #3314 #3373

Conversation

robander commented Oct 3, 2019 • edited by jelovirt Loading

Description

Motivation and Context

How Has This Been Tested?

Type of Changes

robander commented Oct 3, 2019

robander commented Oct 4, 2019

robander commented Oct 5, 2019

infotexture left a comment

Choose a reason for hiding this comment

Testing with FOP

infotexture commented Oct 5, 2019

infotexture commented Oct 5, 2019

lief-erickson commented Oct 7, 2019

robander commented Oct 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robander commented Oct 10, 2019 • edited Loading

lief-erickson commented Oct 10, 2019

robander commented Oct 10, 2019 • edited Loading

lief-erickson commented Oct 10, 2019 • edited Loading

robander commented Oct 10, 2019

robander commented Oct 3, 2019 •

edited by jelovirt

Loading

robander commented Oct 10, 2019 •

edited

Loading

robander commented Oct 10, 2019 •

edited

Loading

lief-erickson commented Oct 10, 2019 •

edited

Loading