Conversation
JohnThomson
changed the title
![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
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 2 comments and resolved 2 discussions.
Reviewable status: 0 of 15 files reviewed, all discussions resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
StephenMcConnel
left a comment
StephenMcConnel
left a comment
There was a problem hiding this comment.
@StephenMcConnel reviewed 15 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on JohnThomson).
JohnThomson
force-pushed
the
customPageImprovements
branch
from
73bef05 to
23693b9
Compare
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 1 comment and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on JohnThomson).
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
JohnThomson
force-pushed
the
customPageImprovements
branch
from
23693b9 to
90b8313
Compare
| "coverIsImage", | ||
| out var legacyCoverSettingValue | ||
| ) && legacyCoverSettingValue; | ||
| BookInfo.AppearanceSettings.Properties.Remove("coverIsImage"); |
There was a problem hiding this comment.
🚩 Migration 14 doesn't write appearance.json to disk, unlike Migration 12
MigrateToLevel14CoverIsImageToCustomLayout removes the coverIsImage property from in-memory AppearanceSettings.Properties at line 4586, but unlike MigrateToLevel12PageNumberPosition (which explicitly calls WriteToFolder and WriteCssToFolder at BookStorage.cs:4561-4562), this migration does not persist the change to disk immediately. The property removal will only be written when the book is eventually saved via Save() → UpdateSupportFiles(). This is presumably safe because the DOM's maintenanceLevel is also only in memory until save, keeping both in sync. But it's worth noting the inconsistency with Level 12's approach.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const activateConvertedBackground = () => { | ||
| requestAnimationFrame(() => { | ||
| theOneCanvasElementManager.setActiveElement( | ||
| bgImageCe, | ||
| ); | ||
| }); | ||
| }; | ||
| activateConvertedBackground(); | ||
| if (!haveRealBgImage) { | ||
| const fallbackHandle = setTimeout(() => { | ||
| activateConvertedBackground(); | ||
| }, 120); | ||
| bgImg.addEventListener( | ||
| "load", | ||
| () => { | ||
| clearTimeout(fallbackHandle); | ||
| activateConvertedBackground(); | ||
| }, | ||
| { once: true }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🚩 activateConvertedBackground uses multiple activation strategies with potential for redundancy
The new code at lines 1788-1808 activates the converted background element using three strategies: (1) immediate requestAnimationFrame, (2) a 120ms fallback timeout, and (3) an image load event listener. When !haveRealBgImage, all three can fire, each calling setActiveElement. The timeout and load event race, and the load event cancels the timeout — but the immediate rAF call at line 1795 will always fire regardless. This means setActiveElement could be called 2-3 times. If setActiveElement is idempotent this is fine, but it's worth verifying there are no side effects from multiple rapid activations.
Was this helpful? React with 👍 or 👎 to provide feedback.
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 2 comments.
Reviewable status: 11 of 15 files reviewed, 2 unresolved discussions (waiting on StephenMcConnel).
3 participants
This change is