Skip to content

Add zoom keyboard shortcuts (Cmd+, Cmd-, Cmd+0)#337

Open
ssstonebraker wants to merge 2 commits intoschuyler:mainfrom
ssstonebraker:feature/zoom-shortcuts-and-fix-strikethrough
Open

Add zoom keyboard shortcuts (Cmd+, Cmd-, Cmd+0)#337
ssstonebraker wants to merge 2 commits intoschuyler:mainfrom
ssstonebraker:feature/zoom-shortcuts-and-fix-strikethrough

Conversation

@ssstonebraker
Copy link
Copy Markdown

Implements per-document zoom as requested in #335.

This PR adds standard macOS zoom shortcuts:

  • Cmd+ zooms in (10% increments, max 300%)
  • Cmd- zooms out (10% decrements, min 50%)
  • Cmd+0 resets to actual size

Implementation:

  • Uses a transient per-document zoom multiplier (not saved to preferences)
  • Zoom applies to both editor and preview when the preference is enabled
  • Menu items validate and disable at zoom limits
  • Does not modify the base font preference
  • Preserves existing Cmd+Shift+0 shortcut for split view

Fixes #335

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
Copy link
Copy Markdown
Owner

@schuyler schuyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@ssstonebraker
Copy link
Copy Markdown
Author

Thanks for the review. Pushed a fix for all three.

Moved the zoom constants to file scope with the kMP prefix to match the rest of the codebase. Restructured scaleWebview so the zoom multiplier always applies to the preview regardless of the previewZoomRelativeToBaseFontSize setting. Fixed the floating-point comparison in resetZoom: validation.

Tested locally, everything works as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Cmd+/Cmd- zoom keyboard shortcuts

2 participants