Preserve Xmatter music when book is updated (BL-16036)#7777
Preserve Xmatter music when book is updated (BL-16036)#7777JohnThomson wants to merge 1 commit intomasterfrom
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
JohnThomson
force-pushed
the
keepXmatterMusic
branch
from
f05bacb to
8ea86cb
Compare
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion.
| var reallyNeedFullSave = book.UpdateDomFromEditedPage( | ||
| editedPageDom, | ||
| out SafeXmlElement pageToSaveToDisk, | ||
| false | ||
| ); | ||
|
|
||
| Assert.That(pageToSaveToDisk, Is.Not.Null); | ||
| Assert.That(reallyNeedFullSave, Is.True); | ||
| AssertThatXmlIn | ||
| .Dom(book.RawDom) | ||
| .HasSpecifiedNumberOfMatchesForXpath( | ||
| "//div[@id='bloomDataDiv']/div[@data-xmatter-page='frontCover' and @data-backgroundaudio='SoundTrack0.mp3' and @data-backgroundaudiovolume='0.21']", | ||
| 1 | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔴 Test asserts behavior that UpdateDomFromEditedPage does not implement
The test UpdateDomFromEditedPage_XmatterFrontCoverHasBackgroundAudio_ForcesFullSave passes needToDoFullSave: false to UpdateDomFromEditedPage and then asserts reallyNeedFullSave is true. However, UpdateDomFromEditedPage at src/BloomExe/Book/Book.cs:4262 returns needToDoFullSave || stylesChanged, which will be false || false = false since no styles changed. The method has no logic to detect xmatter page data attributes and force a full save. Additionally, the test asserts a div with data-xmatter-page and audio attributes exists inside bloomDataDiv, but _bookData.SuckInDataFromEditedDom (which would populate the data div) is skipped when needToDoFullSave is false (src/BloomExe/Book/Book.cs:4243-4244). This test will fail at both assertions. It appears the PR is missing the corresponding changes to UpdateDomFromEditedPage that would detect xmatter page attributes on edited pages and trigger a full save.
Prompt for agents
The test UpdateDomFromEditedPage_XmatterFrontCoverHasBackgroundAudio_ForcesFullSave expects UpdateDomFromEditedPage (in src/BloomExe/Book/Book.cs around line 4221-4262) to detect that an xmatter page has background audio attributes and force a full save (returning true and populating the bloomDataDiv). However, UpdateDomFromEditedPage was not modified in this PR to add that logic. You need to add logic in UpdateDomFromEditedPage that, when the edited page is an xmatter page (has data-xmatter-page attribute) with data-backgroundaudio attributes, forces needToDoFullSave to true so that SuckInDataFromEditedDom is called and the method returns true. This would involve checking whether pageFromEditedDom has a data-xmatter-page attribute and relevant data-backgroundaudio/data-backgroundaudiovolume attributes, and if so, overriding needToDoFullSave to true before the conditional call to _bookData.SuckInDataFromEditedDom.
Was this helpful? React with 👍 or 👎 to provide feedback.
| private static readonly HashSet<string> kXmatterPageAttributesToPreserve = | ||
| new HashSet<string>(StringComparer.OrdinalIgnoreCase) | ||
| { | ||
| HtmlDom.musicAttrName, | ||
| HtmlDom.musicVolumeName, | ||
| }; |
There was a problem hiding this comment.
🚩 CaptureXmatterPageAttributesToPreserve only preserves 2 specific attributes while GatherXmatterPageDataAttributeSetIntoDataSet preserves all data- attributes*
The new kXmatterPageAttributesToPreserve set at src/BloomExe/Book/Book.cs:2590-2595 only includes data-backgroundaudio and data-backgroundaudiovolume. In contrast, the existing GatherXmatterPageDataAttributeSetIntoDataSet method at src/BloomExe/Book/BookData.cs:1654-1682 captures ALL data-* attributes from xmatter pages into the DataSet. This means the capture/restore in BringXmatterHtmlUpToDate won't preserve other data attributes that may have been added to xmatter pages (e.g., data-correct-sound, data-wrong-sound, game-related attributes listed in HtmlDom.cs:1934-1940). This is likely intentional since the existing BookData synchronization mechanism handles those through the data div, but it could be a gap if any other attributes need direct preservation during xmatter re-injection and aren't flowing through the data div path.
Was this helpful? React with 👍 or 👎 to provide feedback.
| RestoreXmatterPageAttributesToPreserve(bookDOM, xmatterAttributesByPage); | ||
|
|
||
| var dataBookLangs = bookDOM.GatherDataBookLanguages(); | ||
| TranslationGroupManager.PrepareDataBookTranslationGroups(bookDOM.RawDom, dataBookLangs); |
There was a problem hiding this comment.
🚩 RestoreXmatterPageAttributesToPreserve placement may conflict with subsequent data div synchronization
In BringXmatterHtmlUpToDate, RestoreXmatterPageAttributesToPreserve is called at line 2582, followed by PrepareDataBookTranslationGroups at line 2585. Notably, the data div synchronization that normally pushes xmatter page attributes from the data div to pages (via UpdateXmatterPageDataAttributeSets in BookData) happens separately during SynchronizeDataItemsThroughoutDOM or UpdateDomFromDataset. The new restore method ensures attributes survive the xmatter re-injection, but they may later be overwritten or need to be re-synced to the data div. This ordering appears correct for the intended purpose—the attributes are restored to pages so that the next data synchronization can pick them up—but it's worth verifying the full lifecycle.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
This change is