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 2 commits into
base: master
Choose a base branch
from
+193 −19
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
2 commits
Select commit Hold shift + click to select a range
48eb8f2
Save the best possible img from custom layout to standard (BL-16086)
JohnThomson Apr 2, 2026
c6de1bf
Post-review changes
JohnThomson Apr 6, 2026
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
79 changes: 60 additions & 19 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,21 @@ public string GetCoverImagePathAndElt(out SafeXmlElement coverImgElt)
if (outsideFrontCover == null)
return null;

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

return GetImagePath(coverImgElt);
Comment on lines +5231 to +5235
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

🟡 GetCoverImagePathAndElt sets coverImgElt to non-null out value even when return path is null

The refactored GetCoverImagePathAndElt sets the coverImgElt out parameter from GetCoverImageElt before calling GetImagePath. When GetCoverImageElt returns bestNonExistingNonPlaceholderElt (an element whose image file doesn't exist on disk), GetImagePath returns null but coverImgElt remains set to that element. This breaks the invariant from the old code where coverImgElt and the return path were always both null or both non-null.

This also changes observable behavior: when the designated cover image references a non-existent file and a placeholder element is also present on the cover page, the old code would return the placeholder path (via bestPlaceholderPath), but the new code returns null (because bestNonExistingNonPlaceholderElt ?? bestPlaceholderElt prefers the non-existing element, and then GetImagePath fails to find the file). Callers like BookThumbNailer.cs:237-252 that specifically handle placeholder paths to generate a thumbnail from a substitute placeholder file will now get null and skip thumbnail generation entirely.

Prompt for agents 
In GetCoverImagePathAndElt (Book.cs around line 5231), the coverImgElt out parameter is set from GetCoverImageElt before GetImagePath is called. If GetImagePath returns null (file doesn't exist on disk), coverImgElt is still non-null, breaking the invariant that both are null or both non-null.

Additionally, GetCoverImageElt returns bestNonExistingNonPlaceholderElt in preference to bestPlaceholderElt at line 5319. When called from GetCoverImagePathAndElt with a bookFolderPath, this means a non-existing image element is preferred over a placeholder element. Then GetImagePath returns null for the non-existing file, losing the placeholder path that the old code would have returned.

To fix: either (a) check the return of GetImagePath and reset coverImgElt to null if the path is null, or (b) adjust the fallback in GetCoverImageElt so that when bookFolderPath is provided, bestPlaceholderElt is preferred over bestNonExistingNonPlaceholderElt (or at least equal). The bestNonExistingNonPlaceholderElt preference makes sense for the AddFallbackCoverImageFromCustomOutsideFrontCover use case (where bookFolderPath is null), but not for GetCoverImagePathAndElt.
Open in Devin Review

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

}

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

// Prefer the designated cover image if it is present and not placeholder.
var designatedCoverImage = outsideFrontCover
.SafeSelectNodes(
Expand All @@ -5236,22 +5251,30 @@ public string GetCoverImagePathAndElt(out SafeXmlElement coverImgElt)
.Cast<SafeXmlElement>()
.FirstOrDefault();

SafeXmlElement bestNonExistingNonPlaceholderElt = null;
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;
if (
bookFolderPath == null
|| ImageExistsInFolder(designatedSource, bookFolderPath)
)
return designatedCoverImage;
// File doesn't exist on disk; remember it but keep looking for one that does.
bestNonExistingNonPlaceholderElt = designatedCoverImage;
}
else
{
bestPlaceholderElt = designatedCoverImage;
bestPlaceholderSource = designatedSource;
}

bestPlaceholderElt = designatedCoverImage;
bestPlaceholderPath = designatedPath;
}
}

Expand All @@ -5271,25 +5294,43 @@ 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;
}
if (
bookFolderPath == null
|| ImageExistsInFolder(candidateSource, bookFolderPath)
)
return candidate;

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

coverImgElt = bestPlaceholderElt;
return bestPlaceholderPath;
return bestNonExistingNonPlaceholderElt ?? bestPlaceholderElt;
}

private static bool ImageExistsInFolder(string source, string bookFolderPath)
{
return RobustFile.Exists(Path.Combine(bookFolderPath, Uri.UnescapeDataString(source)));
}

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