Remember editor pane visibility - #519
Draft
leevi2010-cursor wants to merge 1 commit into
Draft
Conversation
schuyler
requested changes
Jul 11, 2026
schuyler
left a comment
schuyler
left a comment
Owner
There was a problem hiding this comment.
Thanks for the PR, @leevi2010-cursor!
Issues
- Overloads a user-facing preference: hiding or restoring the editor pane silently writes
editorStartInPreviewMode— the explicit "Start in preview mode" Preferences checkbox — so a user's deliberate opt-in gets cleared (and the checkbox visibly flips) as a side effect of a View-menu action, where a dedicated key or window state restoration would keep the two concepts separate. - Untested no-op interaction: no test covers the case where the last-visible-pane guard blocks the toggle, which is the interaction most likely to silently corrupt the remembered preference.
Suggestions
- A one-line comment at the call site explaining the "write only on actual change" gate and its relationship to the last-visible-pane guard would make the coupling legible.
- The asymmetry between
toggleEditorPane:(writes the preference) andtogglePreviewPane:(doesn't) is unexplained, so a note on the intent would help. - A test asserting
togglePreviewPane:never toucheseditorStartInPreviewModewould lock in that intentional asymmetry.
14 tasks
schuyler
added a commit
that referenced
this pull request
Aug 3, 2026
## Summary Takes over draft PR #518 ("Unify editor and preview zoom") and finishes it maintainer-side, per the execution plan in #529. Unified document zoom becomes a single document-level control: ⌘+ / ⌘− / ⌘0 and a toolbar preset dropdown drive one shared zoom level that applies to every open document window and persists across launches. This branch is #518's 8 zoom commits rebased onto current `main`, plus five reviewed fix commits addressing the wiring bug, keybinding collisions, test hygiene, and testability called out in #529. Every change was developed under the project's Rule of Two — each commit (including the rebase conflict resolutions) was reviewed by an independent reviewer before landing. ## What's included **Rebased from #518 (unchanged behavior):** the zoom controls, toolbar preset menu, shared-preference persistence, editor-font/tab-stop scaling, preview re-zoom after WebView reloads, and the v6 preference migration (`documentZoomLevel = 1.0`, additive). **Fixes made on top (one commit each):** 1. **Toolbar zoom dropdown no-op** (`MPToolbarController.m`) — items set `target = self.document` at construction, when the outlet is still nil, so clicking a preset did nothing. Now routed through the file's deferred-dispatch idiom: `target = self`, document resolved lazily at click time, forwarding to `selectDocumentZoom:`. 2. **Format-shortcut relocation** (`MainMenu.xib`) — zoom claims ⌘+/⌘−/⌘0. Rather than dropping the three colliding Format bindings, relocate them: Strikethrough ⌘− → **⌘⇧X**, Paragraph ⌘0 → **⌘⌥0**, Highlight ⌘= → **dropped**. Verified no new collisions. 3. **Zoom test reconciliation** (`MPZoomTests.m`) — removed three tests whose docstrings asserted a flat ±0.1 stepping model and replaced them with tests of the real behavior; `MPPreviewZoomTests.m` remains authoritative. Net test count preserved (25). 4. **Cross-window zoom sharing test** (`MPDocument.m`, `MPZoomTests.m`) — extracted the shared-preference KVO registration out of the nib-load path so a headless test can drive it, and added a test proving a zoom change in one document propagates to another. 5. **Clamp centralization** (`MPDocument.m`) — `resetZoom:` / `selectDocumentZoom:` wrote `documentZoomLevel` directly, bypassing the bounds clamp; both now route through `setZoomMultiplier:`. ## Judgment calls - **Branch de-entanglement.** Rebased #518's 8 zoom commits onto current `main`, dropping the duplicated tip commit (`106aff0`, byte-identical to #519's sole commit) so #519's preference-overloading question stays isolated on its own PR. The conflict surface was slightly larger than #529 predicted: besides the expected mechanical `project.pbxproj` conflict (both #504 and this branch add a test entry — kept both), `main` having advanced past #518's base produced one additional trivial source conflict in `MPDocument.m` (kept both #504's anchor-model constant and the zoom bounds constants; independent adjacent additions). - **Fix #3 model correction.** #529 described the deleted tests as encoding a "false flat ±0.1 stepping" model. In fact zoom snaps among a fixed **non-uniform** preset list (`0.5, 0.75, 0.9, 1.0, 1.1, 1.25, 1.5, 2.0, 3.0`) via `stepDocumentZoomDirection:`; the old tests were coincidentally right at their data points but their asserted *general* model was false. Replacements assert the real snap-to-nearest-preset behavior from off-grid values, with expectations derived from the implementation. - **Fix #4 approach.** Chose the recommended refactor over the fallback. Two refinements to #529's sketch, both verified against the code: (a) the test spies on `applyCurrentZoom` firing rather than asserting on `zoomMultiplier` — the latter is a passthrough over the shared pref and would read the new value even if the observer never fired (tautology); (b) the test explicitly unregisters Doc B's observers before release, because `-close`'s teardown is gated on a flag only set in the nib path, so a headless observer would otherwise crash on dealloc. - **Fix #5 scope.** Routed the two setters named in #529 through the clamp. `stepDocumentZoomDirection:` still writes the pref directly — it only ever writes bounded preset values, so it's safe as-is and outside the issue's named scope (noted as a follow-up below). ## Testing - Unit tests added/reconciled: the cross-window propagation test (fix #4) and the corrected preset-snap tests (fix #3); model-independent rendering/validation/`previewScale` tests retained. - **Build and full-suite verification run on macOS CI** — this branch was prepared on a Linux host where Xcode isn't available, so the debug build and the complete test suite (test count ≥ baseline) are confirmed by CI on this PR, not locally. Please treat the CI result as the build/test gate. ## Follow-ups (not this PR) - Rebind **Highlight** to a sensible shortcut (its shortcut was dropped here) — already tracked in #529. - Route `stepDocumentZoomDirection:` through `setZoomMultiplier:` for a single write path (safe as-is; consistency only). - Pre-existing (predates this PR): a few tests call `makeWindowControllers` without a matching `-close`/unregister, a potential KVO-dealloc risk on a GUI runner. Not introduced here; worth its own ticket. ## Acceptance criteria (#529) - [x] Toolbar zoom dropdown re-zooms the document when a preset is clicked. - [x] ⌘+/⌘−/⌘0 drive zoom; Strikethrough (⌘⇧X) and Paragraph (⌘⌥0) rebound; Highlight's drop is intentional and documented. - [x] Zoom shared across windows (headless test) and persisted across launches (v6 migration). - [x] Migration v6 additive. - [x] Rule-of-Two review on every change, including conflict resolutions. - [ ] Debug build + full suite green, test count ≥ baseline — **pending CI on this PR** (macOS-only). Related to #529 (execution plan), #518 (takeover), #470 / #335 (design), #504 (pbxproj overlap), #519 (kept separate). --- _Generated by [Claude Code](https://claude.ai/code/session_01XYByAV9anAn55kwy3dsF4U)_ --------- Co-authored-by: dpankros <dpankros@gmail.com> Co-authored-by: Steve Stonebraker <github@brakertech.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: leevi2010-cursor <leevi2010@gmail.com>
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
Remember the user's editor pane choice when they use View > Hide Editor Pane or Restore Editor Pane.
This makes the next document window honor the most recent explicit editor-pane choice without adding another preference.
Testing
MPPaneToggleTests: 32 tests pass