Fix BL-16819 "Opaque" setting is forgotten on cover - #8331
Merged
Conversation
Contributor
Author
|
[Claude Fable 5.1] Consulted Devin on 2026-09-08 15:56 UTC up to commit dc67acf. Devin reviewed both the first commit and the final one and reported no bugs, no Investigate flags, and no informational items either time. The one real finding on this PR (the data-div only ever adding classes, so a changed Transparency choice came back on reopen) was raised by the local pre-push review and fixed in 6711ac6. |
https://issues.bloomlibrary.org/youtrack/issue/BL-16819 The user's Transparency choice for an image (Auto / Transparent / Opaque in the image menu) is stored as a bloom-opaque or bloom-transparent class on the img. Every time a book is opened, Bloom regenerates the xmatter from the template and refills the cover image from the data-div in BookData.UpdateImageFromDataSet. That method copied the src back (and style, in special cases) but never the class attribute, so the cover reverted to Auto on every reopen. Content pages were unaffected because their images do not round-trip through the data-div. There was a second half to the bug: saving a page merges the img's classes into the data-div entry as a union, only ever adding (apart from a short remove-if-absent list). So even with the classes restored, changing Opaque back to Auto (or to Transparent) would have brought the old choice back on reopen. The fix: - UpdateImageFromDataSet now restores a short list of img classes that are user data (_imgClassesToRestoreFromDataDiv, currently the two transparency classes) from the saved class list, removing a listed class the data-div lacks so that Auto is restored too. No other classes are copied; bloom-imageLoadError in particular is still re-derived each time the book is opened (BL-14241). - The two transparency classes are added to _classesToRemoveIfAbsent so the data-div copy follows the img when the user changes the choice. Tests: new BookDataTests cover the save-then-reopen round trip for both override classes, changing the choice afterwards (to the other override and back to Auto) and reopening again, the removal of a stale override when the data-div says Auto, and that no other classes are copied. They failed before the fix. The full C# suite passes (3333 passed, 13 skipped). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
JohnThomson
force-pushed
the
BL-16819-cover-opaque
branch
from
September 8, 2026 16:02
dc67acf to
b9863a3
Compare
JohnThomson
marked this pull request as ready for review
September 8, 2026 16:03
StephenMcConnel
approved these changes
Sep 9, 2026
StephenMcConnel
left a comment
StephenMcConnel
left a comment
Contributor
There was a problem hiding this comment.
@StephenMcConnel reviewed 2 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on JohnThomson).
JohnThomson
added a commit
that referenced
this pull request
Sep 10, 2026
…mbnail (BL-16819) https://issues.bloomlibrary.org/youtrack/issue/BL-16819 The first fix (#8331) restores the bloom-opaque / bloom-transparent classes on the cover image from the data-div whenever the xmatter is regenerated. That survived reopening the book and switching books, but entering the Edit tab still reset the image to Auto, and the next save wrote the loss into the book file. The book's thumbnail in the collection also ignored the choice. Entering the Edit tab calls Book.SetMultilingualContentLanguages, which pushes the member _dataset back onto the pages with UpdateDomFromDataset(). That data set is the snapshot taken when the BookData was built; when a page is saved, UpdateSingleTextVariableInDataDiv recreates its entry for the cover image with the new value but no attribute list. RestoreImgClassesFromDataDiv read the missing list as an empty class list and removed the class. - RestoreImgClassesFromDataDiv leaves the image alone when there is no attribute list, matching MergeAttrsIntoElement, which already treats null as "no information". - BookThumbNailer.CreateThumbnailOfCoverImage decides transparency with HtmlDom.GetImageTransparencyMode, so the collection thumbnail honors Opaque and Transparent instead of always running the line-art check. Auto and Transparent both work for a JPEG cover: ImageUtils.MakeTransparentBackground re-saves a non-PNG source as PNG (only a PNG can carry the alpha channel), and MakeTransparentBackgroundIfNeeded reuses it after the line-art check, which accepts JPEGs just as the editor's display path does. The transparent copy is written to a randomly named .png temp file so thumbnails made at the same time cannot collide, and a failure in that step is logged and the thumbnail is made without transparency. RuntimeImageProcessor.MakePngBackgroundTransparentIfDesirable has no remaining callers and is removed. Tests: BookDataTests cover UpdateDomFromDataset() straight after construction and after a save that set the choice (the latter fails without the fix, with an empty class attribute). BookTests cover the BringBookUpToDate round trip including the page as prepared for editing. BookThumbnailerTests check a pixel of the thumbnail for each of the three choices on PNG and JPEG covers; the Opaque cases fail without the fix. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On the front cover, choosing Opaque (or Transparent) from the image's Transparency menu did not stick. Set it, quit Bloom, reopen the book, and the cover image is back on Auto, so a line-art picture on a colored cover is rendered with its background made transparent again. Images on content pages kept the setting; only the cover lost it.
Cause
The Transparency choice is stored as a
bloom-opaqueorbloom-transparentclass on the cover'simg. Every time a book is opened, Bloom rebuilds the front matter from the xmatter template and refills the cover image from the book's data div. The code that does the refilling (BookData.UpdateImageFromDataSet) copied the image'ssrcback, and itsstylein a couple of special cases, but never its classes, so the user's choice was thrown away with the old page. It was still saved correctly in the data div; it just never made it back onto the page.A second, hidden half: saving a page merges the image's classes into the data div as a union, only ever adding. So even with the classes restored, changing Opaque back to Auto (or to Transparent) would have brought the old choice back on the next open.
What the PR does
_imgClassesToRestoreFromDataDiv, currently the two transparency classes) is now restored from the saved class list. The data div is treated as authoritative, so a listed class it lacks is removed, which means switching back to Auto also survives a reopen.bloom-imageLoadErroris still deliberately re-derived each time the book opens (BL-14241).BookDataTestscover the save-then-reopen round trip for Opaque and Transparent, changing the choice afterwards (to the other override and back to Auto) and reopening again, the removal of a stale override when the data div says Auto, and that no unrelated classes are copied. They failed before the fix.Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16819
🤖 Generated with Claude Code
Devin review
This change is