Skip to content

Update page properly when changing custom cover image metadata (BL-16100) #7823

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
+74 −1
Open

Update page properly when changing custom cover image metadata (BL-16100) #7823

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
11 changes: 10 additions & 1 deletion src/BloomExe/Book/BookData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,15 @@ public void SuckInDataFromEditedDom(HtmlDom dom, BookInfo info = null)
UpdateVariablesAndDataDiv(dom.RawDom, info);
}

/// <summary>
/// Create or update the data div with all the data-book values in the specified page element.
/// </summary>
/// <param name="pageElement">This is a page div that we just edited and want to read from.</param>
public void SuckInDataFromEditedDom(SafeXmlElement pageElement, BookInfo info = null)
{
UpdateVariablesAndDataDiv(pageElement, info);
}

public void SynchronizeDataItemsThroughoutDOM()
{
var itemsToDelete = new HashSet<Tuple<string, string>>();
Expand Down Expand Up @@ -1457,7 +1466,7 @@ public void GatherDataItemsFromXElement(
try
{
string query =
$".//{elementName}[(@data-book or @data-library or @data-collection or @{kDataXmatterPage} or @data-custom-layout-id) and not(contains(@class,'bloom-writeOnly'))]";
$"self::{elementName}[(@data-book or @data-library or @data-collection or @{kDataXmatterPage} or @data-custom-layout-id) and not(contains(@class,'bloom-writeOnly'))] | .//{elementName}[(@data-book or @data-library or @data-collection or @{kDataXmatterPage} or @data-custom-layout-id) and not(contains(@class,'bloom-writeOnly'))]";

var nodesOfInterest = sourceElement.SafeSelectNodes(query);

Expand Down
18 changes: 18 additions & 0 deletions src/BloomExe/Edit/EditingModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1098,9 +1098,26 @@ public void UpdateMetaData(string url)
imgElt,
new NullProgress()
);
if (_nextSaveMustBeFull)
{
// We've changed the metadata on the current page, but a full save will
// try to sync everything using the data-div, which has not yet been updated.
// It comes before the page, so the out-of-date copy there will overwrite the
// changes we just made. The simplest way to prevent this is to update the
// data-div to match the current page before we do the full save.
UpdateDataDivFromCurrentPage();
}
Comment on lines +1101 to +1109
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

🚩 UpdateDataDivFromCurrentPage only runs when _nextSaveMustBeFull — metadata updates on non-full-save pages may not sync to data-div

The new guard at src/BloomExe/Edit/EditingModel.cs:1101 only calls UpdateDataDivFromCurrentPage when _nextSaveMustBeFull is true. When it's false (quick save), the image metadata updated by UpdateImgMetadataAttributesToMatchImage on the master DOM page won't be synced to the data-div. For non-custom-layout pages this is fine (image metadata isn't stored in the data-div). But for custom layout pages where the entire margin box is stored in the data-div, a quick save could leave the data-div with stale metadata. This may be acceptable if custom layout pages always trigger a full save, but it's worth confirming that NeedToDoFullSave / _nextSaveMustBeFull is always true for custom layout pages when metadata changes.

Open in Devin Review

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

RefreshDisplayOfCurrentPage();
}

private void UpdateDataDivFromCurrentPage()
{
CurrentBook.BookData.SuckInDataFromEditedDom(
_pageSelection.CurrentSelection.GetDivNodeForThisPage(),
CurrentBook.BookInfo
);
}
Comment on lines +1113 to +1119
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

🟡 Missing comment on private method UpdateDataDivFromCurrentPage

The new private method UpdateDataDivFromCurrentPage at src/BloomExe/Edit/EditingModel.cs:1113 has no summary comment. AGENTS.md states: "All public methods should have a comment. So should most private ones!" While "most" is a soft qualifier for private methods, this is a brand-new method being added in this PR and its purpose (syncing the data-div from the master DOM's current page to prevent stale data-div from overwriting metadata updates during a full save) is non-obvious without a comment.

Suggested change
private void UpdateDataDivFromCurrentPage()
{
CurrentBook.BookData.SuckInDataFromEditedDom(
_pageSelection.CurrentSelection.GetDivNodeForThisPage(),
CurrentBook.BookInfo
);
}
/// <summary>
/// Update the data div from the current page's content in the master DOM.
/// This ensures the data div reflects recent changes (e.g., image metadata updates)
/// before a full save that would otherwise overwrite them with stale data-div content.
/// </summary>
private void UpdateDataDivFromCurrentPage()
{
CurrentBook.BookData.SuckInDataFromEditedDom(
_pageSelection.CurrentSelection.GetDivNodeForThisPage(),
CurrentBook.BookInfo
);
}
Open in Devin Review

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


private DataSet _pageDataBeforeEdits;
private string _featureRequirementsBeforeEdits;

Expand Down Expand Up @@ -1349,6 +1366,7 @@ private static void OptimizeForLinux(HtmlDom pageListDom)
"img { image-rendering: optimizeSpeed; image-rendering: crisp-edges; }";
pageListDom.RawDom.GetElementsByTagName("head")[0].AppendChild(style);
}

internal void SaveToolboxSettings(string data)
{
// ref BL-9859, BL-9912, BL-9978
Expand Down
46 changes: 46 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 @@ -3491,6 +3491,52 @@ public void SuckInDataFromEditedDom_CustomLayoutPage_InactivatesNestedDataAndCla
);
}

[Test]
public void SuckInDataFromEditedDom_CustomLayoutPageWithImage_UpdatesSavedMarginBoxImageMetadata()
{
var bookDom = new HtmlDom(
@"<html><head></head><body>
<div id='bloomDataDiv'>
<div data-book='customOutsideFrontCover' lang='*'>
<div class='marginBox'>
<img src='cover.png' data-copyright='Old Copyright' data-creator='Old Creator' data-license='Old License'/>
</div>
</div>
</div>
<div class='bloom-page bloom-customLayout' data-custom-layout-id='customOutsideFrontCover' id='customCover1'>
<div class='marginBox'>
<img src='cover.png' data-copyright='Old Copyright' data-creator='Old Creator' data-license='Old License'/>
</div>
</div>
</body></html>"
);
var data = new BookData(bookDom, _collectionSettings, null);

var editedPageDom = new HtmlDom(
@"<html><head></head><body>
<div class='bloom-page bloom-customLayout' data-custom-layout-id='customOutsideFrontCover' id='customCover1'>
<div class='marginBox'>
<img src='cover.png' data-copyright='New Copyright' data-creator='New Creator' data-license='New License'/>
</div>
</div>
</body></html>"
);

var editedPage = (SafeXmlElement)
editedPageDom.RawDom.SelectSingleNode(
"//div[contains(@class,'bloom-page') and @data-custom-layout-id='customOutsideFrontCover']"
);

data.SuckInDataFromEditedDom(editedPage);

AssertThatXmlIn
.Dom(bookDom.RawDom)
.HasSpecifiedNumberOfMatchesForXpath(
"//div[@id='bloomDataDiv']/div[@data-book='customOutsideFrontCover']//img[@src='cover.png' and @data-copyright='New Copyright' and @data-creator='New Creator' and @data-license='New License']",
1
);
}

[Test]
public void GatherDataItemsFromXElement_CustomLayoutPageWithXmatterPage_GathersXmatterAttributes()
{
Expand Down