Skip to content

Save the best possible img from custom layout to standard (BL-16086) #7806

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: master
Choose a base branch
from
+167 −16
Open

Save the best possible img from custom layout to standard (BL-16086) #7806

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
50 changes: 34 additions & 16 deletions src/BloomExe/Book/Book.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5228,6 +5228,18 @@ public string GetCoverImagePathAndElt(out SafeXmlElement coverImgElt)
if (outsideFrontCover == null)
return null;

coverImgElt = GetCoverImageElt(outsideFrontCover);
if (coverImgElt == null)
return null;

return GetImagePath(coverImgElt);
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot Apr 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

P1: Behavioral regression: GetCoverImageElt selects elements based on whether the HTML source attribute is non-empty, but does not verify the referenced file exists on disk. The old inline code used GetImagePath() which checked file existence, so it would skip candidates with broken file references and fall through to other valid candidates. Now, if the best candidate's file is missing, GetCoverImagePathAndElt returns null instead of trying the next candidate.

Consider either (a) having GetCoverImageElt accept a file-existence predicate to skip broken references, or (b) adding a fallback loop in GetCoverImagePathAndElt that retries with the next-best candidate when GetImagePath returns null.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/BloomExe/Book/Book.cs, line 5235:

<comment>Behavioral regression: `GetCoverImageElt` selects elements based on whether the HTML source attribute is non-empty, but does not verify the referenced file exists on disk. The old inline code used `GetImagePath()` which checked file existence, so it would skip candidates with broken file references and fall through to other valid candidates. Now, if the best candidate's file is missing, `GetCoverImagePathAndElt` returns null instead of trying the next candidate.

Consider either (a) having `GetCoverImageElt` accept a file-existence predicate to skip broken references, or (b) adding a fallback loop in `GetCoverImagePathAndElt` that retries with the next-best candidate when `GetImagePath` returns null.</comment>

<file context>
@@ -5228,6 +5228,18 @@ public string GetCoverImagePathAndElt(out SafeXmlElement coverImgElt)
+            if (coverImgElt == null)
+                return null;
+
+            return GetImagePath(coverImgElt);
+        }
+
</file context>
Fix with Cubic

}
Comment on lines +5231 to +5236
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration bot Apr 2, 2026

Choose a reason for hiding this comment

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

🚩 Behavioral change: candidate selection no longer checks file existence on disk

The old GetCoverImagePathAndElt used GetImagePath() for candidate selection, which resolved paths against StoragePageFolder and checked RobustFile.Exists(). The new GetCoverImageElt uses GetImageSourceForCoverSelection() (Book.cs:5304-5311) which only reads DOM attributes. This means if the designated cover image has a valid-looking src but the file doesn't exist on disk, GetCoverImageElt will select that element, and then GetImagePath at Book.cs:5235 returns null — whereas the old code would have skipped it and tried the next candidate in the foreach loop. This affects all callers: CollectionApi.cs:710, BookMetadataApi.cs:56, BookCompressor.cs:48, PublishApi.cs:289, BookThumbNailer.cs:237, and Book.cs:5360. The scenario (designated image source is non-empty/non-placeholder but file missing, AND another valid candidate exists on the page) is very unlikely in practice, and DOM-based selection is more appropriate for the new BookData use case. Still, worth noting as a semantic change.

Open in Devin Review

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


internal static SafeXmlElement GetCoverImageElt(SafeXmlElement outsideFrontCover)
{
if (outsideFrontCover == null)
return null;

// Prefer the designated cover image if it is present and not placeholder.
var designatedCoverImage = outsideFrontCover
.SafeSelectNodes(
Expand All @@ -5237,21 +5249,20 @@ public string GetCoverImagePathAndElt(out SafeXmlElement coverImgElt)
.FirstOrDefault();

SafeXmlElement bestPlaceholderElt = null;
string bestPlaceholderPath = null;
string bestPlaceholderSource = null;

if (designatedCoverImage != null)
{
var designatedPath = GetImagePath(designatedCoverImage);
if (designatedPath != null)
var designatedSource = GetImageSourceForCoverSelection(designatedCoverImage);
if (!string.IsNullOrWhiteSpace(designatedSource))
{
if (!ImageUtils.IsPlaceholderImageFilename(designatedPath))
if (!ImageUtils.IsPlaceholderImageFilename(designatedSource))
{
coverImgElt = designatedCoverImage;
return designatedPath;
return designatedCoverImage;
}

bestPlaceholderElt = designatedCoverImage;
bestPlaceholderPath = designatedPath;
bestPlaceholderSource = designatedSource;
}
}

Expand All @@ -5271,25 +5282,32 @@ var candidate in outsideFrontCover
if (candidate == designatedCoverImage)
continue;

var candidatePath = GetImagePath(candidate);
if (candidatePath == null)
var candidateSource = GetImageSourceForCoverSelection(candidate);
if (string.IsNullOrWhiteSpace(candidateSource))
continue;

if (!ImageUtils.IsPlaceholderImageFilename(candidatePath))
if (!ImageUtils.IsPlaceholderImageFilename(candidateSource))
{
coverImgElt = candidate;
return candidatePath;
return candidate;
}

if (bestPlaceholderPath == null)
if (bestPlaceholderSource == null)
{
bestPlaceholderElt = candidate;
bestPlaceholderPath = candidatePath;
bestPlaceholderSource = candidateSource;
}
}

coverImgElt = bestPlaceholderElt;
return bestPlaceholderPath;
return bestPlaceholderElt;
}

private static string GetImageSourceForCoverSelection(SafeXmlElement imageElement)
{
var imageUrl = HtmlDom.GetImageElementUrl(imageElement);
if (!string.IsNullOrWhiteSpace(imageUrl.PathOnly.NotEncoded))
return imageUrl.PathOnly.NotEncoded;

return imageUrl.UrlEncoded;
}

private string GetImagePath(SafeXmlElement imageElement)
Expand Down
86 changes: 86 additions & 0 deletions src/BloomExe/Book/BookData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1658,6 +1658,8 @@ public void GatherDataItemsFromXElement(
);
}
}

AddFallbackCoverImageFromCustomOutsideFrontCover(data, sourceElement);
}
catch (Exception error)
{
Expand All @@ -1671,6 +1673,90 @@ public void GatherDataItemsFromXElement(
}
}

private void AddFallbackCoverImageFromCustomOutsideFrontCover(
DataSet data,
SafeXmlNode sourceElement
)
{
var customOutsideFrontCover = sourceElement
.SafeSelectNodes(
"self::div[contains(concat(' ', normalize-space(@class), ' '), ' outsideFrontCover ') and contains(concat(' ', normalize-space(@class), ' '), ' bloom-customLayout ')] | .//div[contains(concat(' ', normalize-space(@class), ' '), ' outsideFrontCover ') and contains(concat(' ', normalize-space(@class), ' '), ' bloom-customLayout ')]"
)
.Cast<SafeXmlElement>()
.FirstOrDefault();

if (customOutsideFrontCover == null)
return;

var fallbackImageNode = Book.GetCoverImageElt(customOutsideFrontCover);
if (fallbackImageNode == null)
return;

var imageUrl = HtmlDom.GetImageElementUrl(fallbackImageNode);
var encodedImagePath = imageUrl.UrlEncoded;
if (
string.IsNullOrWhiteSpace(encodedImagePath)
&& !string.IsNullOrWhiteSpace(imageUrl.PathOnly.NotEncoded)
)
{
encodedImagePath = UrlPathString
.CreateFromUnencodedString(imageUrl.PathOnly.NotEncoded)
.UrlEncoded;
}
if (string.IsNullOrWhiteSpace(encodedImagePath) && fallbackImageNode.Name == "img")
{
var src = fallbackImageNode.GetAttribute("src");
if (!string.IsNullOrWhiteSpace(src))
{
encodedImagePath = UrlPathString.CreateFromUrlEncodedString(src).UrlEncoded;
}
}
if (string.IsNullOrWhiteSpace(encodedImagePath))
return;

AddOrUpdateTextVariableFromNode(
data,
"coverImage",
"*",
encodedImagePath,
false,
true,
fallbackImageNode
);
}

private void AddOrUpdateTextVariableFromNode(
DataSet data,
string key,
string lang,
string value,
bool isCollectionValue,
bool isVariableUrlEncoded,
SafeXmlElement nodeForAttributes
)
{
if (!data.TextVariables.TryGetValue(key, out var dataSetValue))
{
var textAlternatives = new MultiTextBase();
textAlternatives.SetAlternative(lang, value);
dataSetValue = new DataSetElementValue(textAlternatives, isCollectionValue);
data.TextVariables.Add(key, dataSetValue);
}
else if (!dataSetValue.TextAlternatives.ContainsAlternative(lang))
{
dataSetValue.TextAlternatives.SetAlternative(lang, value);
}
else
{
return;
}

if (isVariableUrlEncoded)
KeysOfVariablesThatAreUrlEncoded.Add(key);

dataSetValue.SetAttributeList(lang, GetAttributesToSave(nodeForAttributes));
}

// Attributes not to copy when saving element attribute data in a DataSetElementValue.
static HashSet<string> _attributesNotToCopy = new HashSet<string>(
new[]
Expand Down
47 changes: 47 additions & 0 deletions src/BloomTests/Book/BookDataTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3457,6 +3457,53 @@ public void SuckInDataFromEditedDom_CustomLayoutPage_InactivatesNestedDataAndCla
);
}

[Test]
public void SuckInDataFromEditedDom_CustomOutsideFrontCoverWithoutCoverImageDataBook_UsesFallbackImageForCoverImage()
{
var bookDom = new HtmlDom(
@"<html><head></head><body>
<div id='bloomDataDiv'></div>
<div class='bloom-page outsideFrontCover'>
<div class='marginBox'>
<div class='bloom-canvas'>
<img data-book='coverImage' src='placeHolder.png'></img>
</div>
</div>
</div>
</body></html>"
);
var bookData = new BookData(bookDom, _collectionSettings, null);

var editedPageDom = new HtmlDom(
@"<html><head></head><body>
<div class='bloom-page outsideFrontCover bloom-customLayout' data-custom-layout-id='customOutsideFrontCover'>
<div class='marginBox'>
<div class='bloom-canvas'>
<img src='fallbackCoverImage.png'></img>
</div>
</div>
</div>
</body></html>"
);

bookData.SuckInDataFromEditedDom(editedPageDom);

var coverImageDataDivElement = bookDom.SelectSingleNodeHonoringDefaultNS(
"//div[@id='bloomDataDiv']/div[@data-book='coverImage']"
);
Assert.That(coverImageDataDivElement, Is.Not.Null);
Assert.That(coverImageDataDivElement.InnerText, Is.EqualTo("fallbackCoverImage.png"));

var standardCoverImageElement = (SafeXmlElement)
bookDom.SelectSingleNodeHonoringDefaultNS(
"//div[contains(@class,'outsideFrontCover')]//img[@data-book='coverImage']"
);
Assert.That(
standardCoverImageElement.GetAttribute("src"),
Is.EqualTo("fallbackCoverImage.png")
);
}

[Test]
public void GatherDataItemsFromXElement_CustomLayoutPageWithXmatterPage_GathersXmatterAttributes()
{
Expand Down