Save the best possible img from custom layout to standard (BL-16086)#7806
Save the best possible img from custom layout to standard (BL-16086)#7806JohnThomson wants to merge 2 commits intomasterfrom
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
src/BloomExe/Book/Book.cs
Outdated
| coverImgElt = GetCoverImageElt(outsideFrontCover); | ||
| if (coverImgElt == null) | ||
| return null; | ||
|
|
||
| return GetImagePath(coverImgElt); | ||
| } |
There was a problem hiding this comment.
🚩 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.
Was this helpful? React with 👍 or 👎 to provide feedback.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 2 comments and resolved 1 discussion.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion.
src/BloomExe/Book/Book.cs
Outdated
| coverImgElt = GetCoverImageElt(outsideFrontCover); | ||
| if (coverImgElt == null) | ||
| return null; | ||
|
|
||
| return GetImagePath(coverImgElt); | ||
| } |
| coverImgElt = GetCoverImageElt(outsideFrontCover, StoragePageFolder); | ||
| if (coverImgElt == null) | ||
| return null; | ||
|
|
||
| return GetImagePath(coverImgElt); |
There was a problem hiding this comment.
🟡 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.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
This change is