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.
src/BloomExe/Book/Book.cs
Outdated
| private static readonly HashSet<string> kXmatterPageAttributesToPreserve = | ||
| new HashSet<string>(StringComparer.OrdinalIgnoreCase) | ||
| { | ||
| HtmlDom.musicAttrName, | ||
| HtmlDom.musicVolumeName, | ||
| }; |
There was a problem hiding this comment.
📝 Info: Behavioral narrowing of GatherXmatterPageDataAttributeSetIntoDataSet from catch-all to whitelist
The old code at BookData.cs:1665-1676 gathered ALL data-* attributes (except data-xmatter-page) from xmatter page elements into the dataset. The new code only gathers attributes in _xmatterPageAttributesToPreserveSet (i.e., data-backgroundaudio and data-backgroundaudiovolume). This means any previously-stored non-music data-* attributes in the data-div will be silently dropped on the next update cycle, since PushXmatterPageAttributesIntoDataDiv (BookData.cs:589-609) removes the old data-div element and recreates it with only the filtered attributes. This is almost certainly intentional cleanup, but if any xmatter template introduces a new user-editable data-* attribute in the future, it will need to be added to PageAttributesToSaveAndPreserveInXmatter at HtmlDom.cs:1903 to survive xmatter replacement.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
I've attempted to refactor so there is a single source of truth for what page attributes need to be saved and what ones additionally need to survive xmatter update and therefore get copied in BookData
src/BloomExe/Book/Book.cs
Outdated
| RestoreXmatterPageAttributesToPreserve(bookDOM, xmatterAttributesByPage); | ||
|
|
||
| var dataBookLangs = bookDOM.GatherDataBookLanguages(); | ||
| TranslationGroupManager.PrepareDataBookTranslationGroups(bookDOM.RawDom, dataBookLangs); |
There was a problem hiding this comment.
🚩 Data-div priority could mask freshly-restored page attributes in edge cases
In SynchronizeDataItemsThroughoutDOM, the data-div is processed before actual pages (BookData.cs:1389: if (!data.XmatterPageDataAttributeSets.ContainsKey(key))). If the data-div has an xmatter page entry without music attributes (e.g., from a prior save where no music was set), and RestoreXmatterPageAttributesToPreserve just restored music attributes to the actual page, the dataset will have the data-div's empty set and skip the page. However, this doesn't cause data loss because UpdateXmatterPageDataAttributeSets (BookData.cs:2070-2077) only sets attributes from the dataset — it never removes attributes already on the page. So the restored music attributes survive. But on the next save cycle, PushXmatterPageAttributesIntoDataDiv will write an empty set to the data-div, meaning the data-div won't persist the music attributes until the user triggers another edit cycle that reads them from the page first. The new CaptureXmatterPageAttributesToPreserve in Book.cs compensates for this by reading directly from pages rather than relying on the data-div.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
I don't clearly understand what it's getting at. The code has changed considerably in response to other comments. It seems to be working.
There was a problem hiding this comment.
This and another comment from devin indicate it thinks we now have two overlapping ways of dealing with saving such attributes. The other place calls it a belt-and-suspenders. But given that you thought this couldn't be a regression but Colin says it is, my intuition is that there is more going on here to understand.
In other words, devin seems to think that there is already code which is attempting to do the thing but we now added code to do the thing instead of fixing that code.
JohnThomson
force-pushed
the
keepXmatterMusic
branch
from
8ea86cb to
f72ba0b
Compare
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 3 comments.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on hatton).
src/BloomExe/Book/Book.cs
Outdated
| RestoreXmatterPageAttributesToPreserve(bookDOM, xmatterAttributesByPage); | ||
|
|
||
| var dataBookLangs = bookDOM.GatherDataBookLanguages(); | ||
| TranslationGroupManager.PrepareDataBookTranslationGroups(bookDOM.RawDom, dataBookLangs); |
There was a problem hiding this comment.
I don't clearly understand what it's getting at. The code has changed considerably in response to other comments. It seems to be working.
src/BloomExe/Book/Book.cs
Outdated
| private static readonly HashSet<string> kXmatterPageAttributesToPreserve = | ||
| new HashSet<string>(StringComparer.OrdinalIgnoreCase) | ||
| { | ||
| HtmlDom.musicAttrName, | ||
| HtmlDom.musicVolumeName, | ||
| }; |
There was a problem hiding this comment.
I've attempted to refactor so there is a single source of truth for what page attributes need to be saved and what ones additionally need to survive xmatter update and therefore get copied in BookData
3 participants
This change is