Unify editor and preview zoom - #518
Closed
leevi2010-cursor wants to merge 9 commits into
Closed
Conversation
Provide Actual Size (Cmd+0), Zoom In (Cmd++), and Zoom Out (Cmd+-) menu items plus a toolbar dropdown so users can rescale the rendered preview for readability, presentations, or larger displays. Zoom level persists across launches and snaps to preset increments: 50, 75, 90, 100, 110, 125, 150, and 200 percent.
Implements transient per-document zoom as requested in schuyler#335: - Cmd+ zooms in (10% increments, max 300%) - Cmd- zooms out (10% decrements, min 50%) - Cmd+0 resets to actual size - Zoom applies to both editor and preview when preference is enabled - Zoom is transient (not saved to preferences) - Menu items validate at zoom limits - Does not mutate base font preference Fixes schuyler#335
- Move kMinZoom/kMaxZoom to file-scope constants - Fix preview zoom when previewZoomRelativeToBaseFontSize is off - Fix floating-point comparison in resetZoom: validation
Adds MPZoomTests.m with 22 tests covering zoom multiplier basics, menu validation, preference observer behavior, and tab stop calculation. Two tests are expected to FAIL against current code as TDD regression guards: - testSetupEditorPreservesZoomedFontSize: guards against setupEditor: resetting the editor font to its unzoomed base size, clobbering any active zoom. - testTabStopsReflectZoomedFontSize: guards against tab stops being computed from the base font instead of the zoomed font. Related to schuyler#335
Bug 1: Cmd+0 conflict. "Actual Size" was bound to Cmd+0, which collides with Format > "Paragraph". Remove the shortcut from "Actual Size" (menu only) and change "Zoom In" from "+" to "=" so Cmd+= works without Shift on US keyboards (standard macOS zoom behavior). Bug 2: Editor font reset on preference changes. setupEditor: applied the raw base font via self.editor.font = font, clobbering any active zoom when the font, style, or line-spacing preference changed. Bug 3: Tab stops computed from base font. setupEditor: computed tab stops from the unzoomed base font's space width, so tabs appeared at the wrong width after zoom. Add a zoomedEditorFont helper that returns base font x zoomMultiplier. setupEditor: now uses it for both the tab stop calculation and the editor font assignment, fixing bugs 2 and 3 in one change. Simplify applyCurrentZoom to delegate to setupEditor: (which already calls scaleWebview), so zoom actions also refresh tab stops and the syntax highlighter's font cache. Related to schuyler#335
XIB: The previous fix moved Cmd+0 off "Actual Size" but introduced two new collisions - Cmd+= with Format > "Highlight" and Cmd+- with Format > "Strikethrough". Change "Zoom In" to Cmd+Plus (keyEquivalent "+") and add Shift+Cmd to "Zoom Out" so both use the Shift modifier and neither collides with the Format menu shortcuts. Performance: applyCurrentZoom previously routed through setupEditor:, which does far more than the zoom path needs - it deactivates and reactivates the PEG Markdown highlighter, re-reads and re-applies the stylesheet from disk, and replaces the editor's CALayer. Extract the font and paragraph-style logic into a dedicated applyEditorFontAndParagraphStyle helper that both setupEditor: and applyCurrentZoom call, so zoom keystrokes skip the highlighter re-parse and CALayer rebuild. Tests: remove outdated "expected to FAIL" comments now that the bugs are fixed, and drop testSetupEditorDoesNotResetZoomMultiplier, which only checked that a CGFloat property stayed unchanged across a method that never touched it - the real Bug 2 regression is covered by testSetupEditorPreservesZoomedFontSize. Related to schuyler#335
Extract previewScale from scaleWebview so the scale computation is unit-testable without mocking WebView. The PR's scaleWebview change removed the early-return when previewZoomRelativeToBaseFontSize is OFF, so add four tests that pin both branches of the calculation: - preference OFF + zoom 1.0 -> 1.0 (no-op equivalence) - preference OFF + non-default zoom -> tracks zoomMultiplier - preference ON + non-default zoom -> (fontSize/14) * zoomMultiplier - preference ON + zoom 1.0 -> matches legacy fontSize/14 ratio Each test saves and restores the preference values it touches. Related to schuyler#335
schuyler
reviewed
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
- Toolbar zoom dropdown does nothing: the popup menu items are assigned
target = self.documentinsidesetupToolbarItems(called from-init), where thedocumentoutlet is still nil, so clicking a preset is a silent no-op — the deferred-dispatch proxy pattern already used elsewhere in this file avoids exactly this. - Format shortcuts lost, not relocated: MainMenu.xib strips the key equivalents from Highlight (⌘=), Strikethrough (⌘−), and Paragraph (⌘0) rather than reassigning them per #470, so those commands lose their shortcuts entirely.
- Two contradictory zoom test suites: MPZoomTests.m asserts flat ±0.1 stepping on a per-document
zoomMultiplier, while MPPreviewZoomTests.m asserts preset-snapping on a shared preference — only the latter matches the implementation, and the stale suite would benefit from reconciliation or removal. - Headline behavior untested: nothing verifies that zoom is actually shared across open document windows (the PR's core claim), and such a test would also have surfaced the nil-target bug above.
Suggestions
resetZoom:/selectDocumentZoom:writedocumentZoomLeveldirectly, bypassing the clamp insetZoomMultiplier:, so routing through the setter would keep bounds enforcement in one place.- The zoom preset array is duplicated between MPToolbarController.m and MPDocument.m with a hand-sync comment, and a single shared source would avoid drift.
- The new
toggleEditorPane:→editorStartInPreviewModechange appears to be PR #519's feature bundled in here, so it's worth confirming that's intentional or splitting it out. - A dealloc round-trip test for the toolbar controller's new KVO observer would guard the add/remove lifecycle.
- help.md has no zoom section and doesn't note the moved Format shortcuts, leaving users no in-app reference for either.
- Several tests early-
returnwhen headless, silently passing with zero assertions, where XCTSkip would make those skips visible in CI.
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>
Owner
|
Superseded by #552, which rebased this branch onto current Generated by Claude Code |
schuyler
added
the
rc-pending
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
5 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
Unifies editor and preview zoom into a single document-level control, as proposed in issue #470.
This builds on the implementations and retained commit history from #442 and #395.
Testing
MPZoomTests,MPPreviewZoomTests, andMPToolbarControllerTests)MainMenu.xibRelated to #470 and #335.