Per-OS-window zoom override (refs #1394)#10145
Per-OS-window zoom override (refs #1394)#10145Eridanus117 wants to merge 4 commits intowarpdotdev:masterfrom
Conversation
When UIZoom is enabled, Cmd++/Cmd+-/Cmd+0 in one window currently
propagates to every window because zoom_factor lives on AppContext and
set_zoom_factor invalidates all views. The handlers also wrote through
WindowSettings::zoom_level, which is an app-wide singleton.
Add a per-window override on top of the existing app-wide default:
effective_zoom = window.zoom_factor_override ?? AppContext.zoom_factor
- core::Window: zoom_factor_override: Option<ZoomFactor>
- AppContext: window_zoom_factor / set_window_zoom_factor /
reset_window_zoom_factor; the setters only invalidate the target
window's views.
- presenter.rs: build_scene + dispatch_event read window_zoom_factor.
- Workspace adjust_zoom / reset_zoom now write the per-window override
directly. WindowSettings.zoom_level remains the surface for the
Settings dropdown ("Default Zoom"), which goes through the existing
AppContext::set_zoom_factor path; windows without overrides follow it.
Aligns with the prior art shape used by VS Code (window.zoomLevel +
per-window override), Kitty (change_font_size affects current OS
window), and iTerm2 (per-session font size on top of profile).
Rev 2 addresses dual-reviewer block on Rev 1: the patch only routed
build_scene + dispatch_event through window_zoom_factor, but four
other zoom readers in the chrome/layout path still consulted the
app-wide WindowSettings::zoom_level. Cmd++/Cmd+- on macOS would
therefore scale the rendered tab bar but leave the native titlebar
height and traffic-light reservation at the global zoom, producing a
visibly broken titlebar.
- view.rs:11990 update_titlebar_height
- view.rs:17494 traffic-light reservation
- view.rs:17516 compute_tab_bar_left_padding
- theme_chooser.rs:593 traffic-light header margin
All four now read ctx.window_zoom_factor(window_id).as_f32().
Workspace::adjust_zoom / reset_zoom now invoke update_titlebar_height
explicitly because the per-window override path no longer mutates
WindowSettings::zoom_level (so WindowSettingsChangedEvent::ZoomLevel
no longer fires for Cmd++/Cmd+-/Cmd+0).
set_window_zoom_factor clamp tightened to [0.5, 3.5] to match
ZoomLevel::VALUES (50%–350%); the original [0.5, 4.0] left a 3.5–4.0
band that adjust_zoom's position lookup couldn't step within.
Add unit tests (crates/warpui_core/src/core/zoom_test.rs) covering:
- default fallback to AppContext.zoom_factor
- per-window isolation under set
- global default change leaves overrides untouched, updates
non-overridden windows
- reset clears override
- missing window_id is silent noop
- clamp [0.5, 3.5] enforced
The app-wide setter clamped to [0.5, 4.0] while the new per-window setter clamps to [0.5, 3.5] (matching ZoomLevel::VALUES, 50%–350%). Asymmetry is observable: a caller that pushes the global default into the [3.5, 4.0] band would leave non-overridden windows at an effective zoom that adjust_zoom's discrete-step lookup cannot find in VALUES, making Cmd++/Cmd+- silently no-op until reset_window_zoom_factor. No internal caller in this PR drives set_zoom_factor outside the VALUES range (lib.rs startup and appearance_page.rs settings dropdown both read percentages straight from VALUES), so the immediate symptom is not user-reachable. Aligning the clamp now keeps both setters honest and removes a future-trap ahead of upstream review.
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @Eridanus117 on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
github-actions
Bot
added
the
external-contributor
|
Signed. |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds per-OS-window zoom overrides in WarpUI core and routes renderer, input scaling, and several chrome/layout zoom readers through the window-effective zoom.
Concerns
- The keyboard zoom path can still become a no-op for valid in-range effective zoom factors that are not exactly present in
ZoomLevel::VALUES.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| let Some(current_index) = crate::window_settings::ZoomLevel::VALUES | ||
| .iter() | ||
| .position(|zoom| *zoom == current_zoom) | ||
| .position(|zoom| *zoom == current_percent) |
There was a problem hiding this comment.
ZoomLevel::VALUES (for example, an app default of 1.3 after reset_window_zoom_factor), so Cmd++/Cmd+- becomes a no-op; snap to the nearest table entry before stepping or store overrides as ZoomLevel percentages.
Addresses oz-agent review on PR warpdotdev#10145: keyboard zoom path could no-op when the effective zoom factor was a valid in-range value but not exactly one of `ZoomLevel::VALUES`. The previous implementation looked up the current percentage in VALUES and stepped by index; if the current value wasn't in VALUES (e.g. a caller set the global default to 105% or any non-discrete percentage in [0.5, 3.5]), `position()` returned `None` and `adjust_zoom` early-returned, leaving Cmd++/Cmd+- silently inert. Replace the index lookup with a directional search for the next/prev VALUES entry strictly above/below `current_percent`, falling back to the boundary value if there is no further step. Behaviour for values already in VALUES is unchanged; non-VALUES values now snap to the nearest VALUES entry in the chosen direction. Extract the step computation as a free `next_zoom_step` so the logic can be unit-tested in isolation. Add 8 tests covering: in-VALUES step up/down, non-VALUES snap up/down, boundary stays (at min/max), and out-of-range graceful fallback.
|
Thanks for the review @oz-agent. The concern is valid — even with the [0.5, 3.5] clamp, Pushed Extracted the step computation as a free
|
|
I'm re-reviewing this pull request in response to a review request. I reviewed this pull request and requested human review from: Comment Powered by Oz |
oz-for-oss
Bot
dismissed
their stale review
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR adds per-OS-window zoom overrides in WarpUI, routes rendering/input and chrome layout through the effective window zoom, and adds focused unit coverage for the new zoom invariants.
Concerns
- No blocking correctness or security concerns found in the annotated diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
1 participant
Refs #1394.
Summary
Cmd+=/Cmd+-/Cmd+0in one Warp OS window only affects that window. Other windows keep their current zoom, which falls back to the app-wide default (WindowSettings.zoom_level) when the window has no override. The Settings UI "Default Zoom" dropdown still drives the app-wide default; windows without a per-window override follow it.Conceptually mirrors VS Code's
window.zoomLevel(global) + per-window override viawindow.zoomPerWindow, and Kitty'schange_font_size(defaults to current OS window).Implementation shape
Three commits on top of
a057a10,+191 / -28total across 9 files:c3a6997Window::zoom_factor_override: Option<ZoomFactor>+ new AppContext methodswindow_zoom_factor/set_window_zoom_factor/reset_window_zoom_factor; renderer + Cmd-keys route through per-window override6d38c6bupdate_titlebar_heightcall in adjust/reset because per-window path no longer firesWindowSettingsChangedEvent::ZoomLevel; tightened clamp; unit tests16cff28set_zoom_factorclamp[0.5, 4.0]→[0.5, 3.5]so neither setter can leaveadjust_zoom's VALUES lookup outside the discrete tableResolution model:
Pane-level overrides (PR #9705 implementing per-pane font for #6739) are on a separate axis and untouched:
Why it's app-wide today (current master)
crates/warpui_core/src/core/app.rs:1188set_zoom_factorinvalidate_all_views().crates/warpui_core/src/presenter.rs:346/347/520ctx.zoom_factor()from AppContext (every window same value).app/src/window_settings.rs:73zoom_levelWindowSettings, butWindowSettingsis an app-wide singleton (WindowSettings::handle(ctx)/as_ref(ctx)accessors take noWindowId).app/src/workspace/view.rs:15545-15580increase_zoom/decrease_zoom/reset_zoom/adjust_zoomwrite through that singleton.app/src/settings_view/appearance_page.rs:925set_zoom_factor, closing the global path.Four additional zoom-aware sites in chrome/layout read the same singleton — any per-window scheme that only touches the renderer would leave native macOS titlebar / traffic-light spacing misaligned, so the patch covers those too:
app/src/workspace/view.rs:11990update_titlebar_heightapp/src/workspace/view.rs:17494traffic-light reservationapp/src/workspace/view.rs:17516compute_tab_bar_left_paddingapp/src/themes/theme_chooser.rs:593traffic-light header marginBehavioral notes
window.zoomLevelsemantics — reset is viaCmd+0which clears the override).Out of scope (follow-ups)
window.zoomPerWindow: falsetoggle for users who prefer global-sync legacy behavior.Test plan
cargo build --bin warp-oss --features gui(build clean)cargo test --package warpui_core zoom_tests(unit tests pass: per-window isolation, global fallback, override reset, missing-window noop, clamp range)Cmd+=× N in window-A only changes window-A;Cmd+0returns to default; new window opens at default; native macOS titlebar height tracks per-window zoom.Posture
Filed as draft — wanting to surface implementation shape against #1394 (canonical per-OS-window tracker per the earlier bot routing on #10115 and #1394) before requesting review. Happy to mark ready-for-review with any specific design preference on the resolution shape, scope, or clamp/range.