Add zoom keyboard shortcuts (Cmd+, Cmd-, Cmd+0)#337
Add zoom keyboard shortcuts (Cmd+, Cmd-, Cmd+0)#337ssstonebraker wants to merge 2 commits intoschuyler:mainfrom
Conversation
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
schuyler
left a comment
There was a problem hiding this comment.
Thanks for tackling #335 — the overall design is clean. Transient per-document zoom via a multiplier (not mutating preferences) is the right call. A few things to address before this is ready to merge:
Bug: Preview zoom broken when preference is off
This is the main issue. scaleWebview has an early return when previewZoomRelativeToBaseFontSize is NO:
if (!self.preferences.previewZoomRelativeToBaseFontSize)
return;Because the zoomMultiplier is applied inside that method, Cmd+/Cmd- won't affect the preview at all when the preference is off. The zoom shortcuts should work on the preview regardless of that setting.
Duplicated zoom constants
kMaxZoom (3.0) and kMinZoom (0.5) are each defined as static const in three separate places — validateUserInterfaceItem:, zoomIn:, and zoomOut:. These should be file-level constants defined once to avoid the risk of them getting out of sync.
Floating-point comparison in resetZoom: validation
return self.zoomMultiplier != 1.0;After several +0.1 / -0.1 round trips, IEEE 754 can drift — 1.0 + 0.1 - 0.1 may not exactly equal 1.0. This could leave the "Actual Size" menu item enabled at 100%. Something like fabs(self.zoomMultiplier - 1.0) > 0.001 would be safer.
- Move kMinZoom/kMaxZoom to file-scope constants - Fix preview zoom when previewZoomRelativeToBaseFontSize is off - Fix floating-point comparison in resetZoom: validation
|
Thanks for the review. Pushed a fix for all three. Moved the zoom constants to file scope with the Tested locally, everything works as expected. |
Implements per-document zoom as requested in #335.
This PR adds standard macOS zoom shortcuts:
Implementation:
Fixes #335