Update page properly when changing custom cover image metadata (BL-16100)#7823
Update page properly when changing custom cover image metadata (BL-16100)#7823JohnThomson wants to merge 1 commit intomasterfrom
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| private void UpdateDataDivFromCurrentPage() | ||
| { | ||
| CurrentBook.BookData.SuckInDataFromEditedDom( | ||
| _pageSelection.CurrentSelection.GetDivNodeForThisPage(), | ||
| CurrentBook.BookInfo | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 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.
| 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 | |
| ); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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(); | ||
| } |
There was a problem hiding this comment.
🚩 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.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
This change is