Skip to content

Prevent 6.3 from 'fixing' audio IDs in 6.4 custom covers (BL-15902) #7704

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.

Sign up for GitHub

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

Open
JohnThomson wants to merge 1 commit into
base: Version6.3
Choose a base branch
from
+32 −1
Open

Prevent 6.3 from 'fixing' audio IDs in 6.4 custom covers (BL-15902) #7704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/BloomExe/Book/Book.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1273,6 +1273,13 @@ private void FixDuplicateAudioIdsInDataDiv(HtmlDom bookDOM, HashSet<string> idSe
return; // shouldn't happen, but paranoia sometimes pays off, especially in running tests.
var nodes = dataDiv
.SafeSelectNodes("(.//div|.//span)[@id and contains(@class,'audio-sentence')]")
// We expect the customCover to have copies of other stuff in the data div.
.Where(x =>
!(
x is SafeXmlElement e
&& e.ParentWithAttributeValue("data-book", "customCover") != null
)
)
Comment on lines +1276 to +1282
Copy link

@devin-ai-integration devin-ai-integration bot Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 customCover string literal only appears in C# code — verify it matches what 6.4 actually emits

The filter added in this PR checks for data-book="customCover" via e.ParentWithAttributeValue("data-book", "customCover"). However, searching the entire repository for customCover only returns these two lines in Book.cs. There are no HTML templates, test fixtures, or other references that confirm this is the exact attribute value used by version 6.4's custom cover feature. The commit c3f8b0378 ("Adds the cover layout control and allows a 'custom' cover (BL-15902)") is in the git log but its contents aren't available locally. It would be worth verifying that customCover is the exact data-book value used in the 6.4 branch.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Member

@hatton hatton Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnThomson Can reply/resolve this one? Thx.

.ToList();
var duplicateAudioIdsFixed = 0;
foreach (var audioElement in nodes)
Comment on lines 1284 to 1285
Copy link

@devin-ai-integration devin-ai-integration bot Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 duplicateAudioIdsFixed is never incremented, so SynchronizeDataItemsThroughoutDOM() is dead code

In FixDuplicateAudioIdsInDataDiv, the local variable duplicateAudioIdsFixed is initialized to 0 at line 1284 and checked at line 1296, but it is never incremented inside the loop when a duplicate is actually found and fixed (lines 1289–1293). As a result, the condition if (duplicateAudioIdsFixed > 0) is always false, and _bookData.SynchronizeDataItemsThroughoutDOM() is never called.

Root Cause and Impact

The loop at src/BloomExe/Book/Book.cs:1285-1293 calls FixDuplicateAudioId when a duplicate is detected, but never does ++duplicateAudioIdsFixed. Compare with the analogous method FixMissingAudioIdsInDataDiv at src/BloomExe/Book/Book.cs:1343-1360, which correctly checks nodeList.Count > 0 before calling SynchronizeDataItemsThroughoutDOM().

Without the synchronization call, duplicate audio IDs that are fixed in the data div are not propagated to the rest of the book's DOM, leaving the book pages out of sync with the data div.

This is a pre-existing bug but is in the method directly modified by this PR.

(Refers to lines 1284-1297)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Member

@hatton hatton Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnThomson Can reply/resolve this one? Thx.

Expand Down
26 changes: 25 additions & 1 deletion src/BloomExe/SafeXml/SafeXmlElement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Xml;
Expand Down Expand Up @@ -201,6 +201,30 @@ public SafeXmlElement ParentOrSelfWithClass(string targetClass)
return current;
}

public SafeXmlElement ParentWithAttribute(string targetAttribute)
{
var current = ParentElement;
while (current != null && !current.HasAttribute(targetAttribute))
current = current.ParentNode as SafeXmlElement;
return current;
}

/// <summary>
/// Get a parent element if any that has the specified value for the specified attribute
/// Note: currently if attrVal is empty it will match both parents where the attribute
/// is present and empty, and parents that don't have the attribute at all. It is not
/// intended that it should be used this way.
/// </summary>
public SafeXmlElement ParentWithAttributeValue(string targetAttribute, string attrVal)
{
ArgumentException.ThrowIfNullOrEmpty(attrVal);

var current = ParentElement;
while (current != null && current.GetAttribute(targetAttribute) != attrVal)
current = current.ParentNode as SafeXmlElement;
return current;
}

public SafeXmlElement GetChildWithName(string name)
{
return ChildNodes.FirstOrDefault(n => n.Name.ToLowerInvariant() == name)
Expand Down