Address issue #441: honor Sync Panes toggle mid-session#463
Merged
Conversation
Turning Sync Panes off while a document was open did not fully detach the panes: typing in the editor still scrolled the Preview toward the editor. The cause was a stale, editor-derived lastPreviewScrollTop (written while sync was on) that the full-reload completion handler restores unconditionally, plus lingering Editor scroll ownership. Detect the editorSyncScrolling transition in userDefaultsDidChange: and react immediately: - On disable: reset scroll ownership to Neither and recapture the preview's actual position so the panes become fully independent with no jump. Also keep lastPreviewScrollTop aligned with the preview's real position on the full-reload path whenever sync is off. - On enable: re-sync immediately, editor-authoritative. A cached lastKnownSyncScrolling (seeded at init from the live preference) drives edge detection only; gating still always reads the live preference. Adds Group O regression tests in MPScrollSyncTests.m covering the disable/enable transitions, ownership handling, nil-safety, and edge detection. Related to #441
Contributor
Code Coverage ReportCurrent Coverage: 61.18% Coverage Details (Summary) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the reported bug where, after turning Sync Panes off while a document is open, typing in the editor still auto-scrolled the Preview toward the editor. Restarting with sync already off avoided it — the tell-tale sign that this was a mid-session state problem, not an initialization one.
Root cause
While sync was ON, every forward sync wrote an editor-derived value into
lastPreviewScrollTop(syncScrollers). The full-reload completion handler (MPGetPreviewLoadingCompletionHandler) restores the preview tolastPreviewScrollTopunconditionally, regardless of the sync preference. After disabling sync, that stale editor-derived position kept getting restored, plus scroll ownership could linger asEditor— so the panes were never truly independent until the document was reopened.Fix
Detect the
editorSyncScrollingtransition inuserDefaultsDidChange:(the only native hook that fires for the XIB-bound checkbox) and react immediately:handleSyncScrollingDisabled): reset scroll ownership toNeitherand recapture the Preview's actual current position intolastPreviewScrollTop, so the panes become fully independent with no jump. The full-reload path also keepslastPreviewScrollTopaligned with the Preview's real position whenever sync is off, covering the "scroll the Preview independently, then type" case.handleSyncScrollingEnabled): re-sync immediately, editor-authoritative (Preview moves to match the editor), bracketed inEditorownership and returned toNeither— mirroring the existing editor-reveal sync insetSplitViewDividerLocation:.A cached
lastKnownSyncScrolling(seeded ininitfrom the live preference) drives edge detection only; gating still always reads the live preference, so no preference value is ever cached for decision-making.Audit
Confirmed every
syncScrollers/syncScrollersReversecall site reads the liveeditorSyncScrollingvalue; the only non-sync preview-moving write was the unconditionallastPreviewScrollToprestore, which this change neutralizes when sync is off.Files Changed
MacDown/Code/Document/MPDocument.m— transition detection +handleSyncScrollingEnabled/handleSyncScrollingDisabled; full-reload position capture when sync is off; cached value seeded ininit.MacDownTests/MPScrollSyncTests.m— new Group O regression tests.CHANGELOG.md— user-facing entry.Related Issue
Related to #441
Manual Testing Plan
Review Notes
Automated review found no critical issues. The unit tests lock the fix at the state level (ownership reset, position recapture, edge detection, nil-safety); the rendered end-to-end jump requires a live WebView and isn't headless-testable, which is consistent with the existing scroll-sync test suite's approach.