[Windows] fix session restoration for maximized and fullscreen windows#9536
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and posted feedback on this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR skips the Windows vertical offscreen-position adjustment when a newly created window is already maximized or fullscreen, preventing session restoration from moving windows that should be governed by the OS/window state.
Concerns
- No blocking correctness or security concerns were identified in the changed lines.
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
lucieleblanc
left a comment
There was a problem hiding this comment.
Thanks for the fix!
…dev#9536) ## Description We currently have a bug with session restoration where maximized windows won't restore into the maximized state. This is due to the "vertical adjustment" code which tries to make sure the title bar is on screen (it moves the window downward if that isn't the case). Without getting too much into the details, on Windows when windows are maximized, winit reports the physical size of the window _as if_ it had the native window decorations, even when it doesn't. It treats the native title bar as being off the screen and represents that with a negative origin (above the screen). This triggers `fn maybe_adjust_window_vertically` to return a value which triggers the caller to try to move the window which makes Windows un-maximize the window. This simple guard fixes the problem ## Testing 1. Run Warp on Windows 2. Make sure session restoration is enabled 3. Maximize the window 4. quit warp 5. Start warp again 6. The window should be maximized but it isn't On this branch, that bug no longer occurs.
Closes #9383. ### Description `set_ime_allowed(true)` was only being called inside the `#[cfg(windows)]` block in `crates/warpui/src/windowing/winit/window.rs::create_window` (the block that ends around line 1421 on master). On Linux/Wayland, winit's text-input plugin only sends `zwp_text_input_v3.enable()` to the compositor after `set_ime_allowed(true)` is called — without that call IME stays inactive, so non-Latin input methods (CJK in particular) appear to do nothing in Warp. ### Fix Add a parallel `#[cfg(target_os = "linux")]` block right after the Windows one calling the same method on the created window. macOS continues to use the native NSTextInputClient route and does not need this winit hint. A repo-wide grep for `set_ime_allowed` confirms there is no other Linux-specific call site that was already covering this — the previous behaviour was "Windows-only" by oversight, not by design. ```rust #[cfg(target_os = "linux")] if let Ok(window) = created_window.as_ref() { // On Linux/Wayland, winit only sends `zwp_text_input_v3.enable()` when IME is allowed, // so without this call IME stays inactive and non-Latin input (CJK, etc.) is unusable. window.set_ime_allowed(true); } ``` ### Testing - `cargo fmt -p warpui -- --check` — passes. - Compile-only verification possible from my checkout (Metal toolchain unavailable for full `cargo nextest`, mirroring #9277). The change is a 7-line addition that mirrors the existing Windows block exactly. - Functional verification needs a Wayland session, which I can't run locally; CI on Linux runners should cover compilation. A maintainer with a Wayland setup could confirm IME behaviour with a CJK input method. ### Coordination with #9536 PR #9536 (open) modifies the same file but inside `maybe_adjust_window_vertically` (lines ~1433+ on master, adding 3 lines that early-return on maximize/fullscreen). My edit lands in `create_window` on lines 1422–1428, which #9536 does not touch — no merge conflict expected. ### Server API No server changes. ### Agent Mode Not applicable. ### Changelog Entries `CHANGELOG-BUG-FIX`: IME (input method) now activates on Linux/Wayland, restoring CJK and other non-Latin input that had been silently broken since first launch. Co-authored-by: anshul-garg27 <13553550+anshul-garg27@users.noreply.github.com>
Closes warpdotdev#9383. ### Description `set_ime_allowed(true)` was only being called inside the `#[cfg(windows)]` block in `crates/warpui/src/windowing/winit/window.rs::create_window` (the block that ends around line 1421 on master). On Linux/Wayland, winit's text-input plugin only sends `zwp_text_input_v3.enable()` to the compositor after `set_ime_allowed(true)` is called — without that call IME stays inactive, so non-Latin input methods (CJK in particular) appear to do nothing in Warp. ### Fix Add a parallel `#[cfg(target_os = "linux")]` block right after the Windows one calling the same method on the created window. macOS continues to use the native NSTextInputClient route and does not need this winit hint. A repo-wide grep for `set_ime_allowed` confirms there is no other Linux-specific call site that was already covering this — the previous behaviour was "Windows-only" by oversight, not by design. ```rust #[cfg(target_os = "linux")] if let Ok(window) = created_window.as_ref() { // On Linux/Wayland, winit only sends `zwp_text_input_v3.enable()` when IME is allowed, // so without this call IME stays inactive and non-Latin input (CJK, etc.) is unusable. window.set_ime_allowed(true); } ``` ### Testing - `cargo fmt -p warpui -- --check` — passes. - Compile-only verification possible from my checkout (Metal toolchain unavailable for full `cargo nextest`, mirroring warpdotdev#9277). The change is a 7-line addition that mirrors the existing Windows block exactly. - Functional verification needs a Wayland session, which I can't run locally; CI on Linux runners should cover compilation. A maintainer with a Wayland setup could confirm IME behaviour with a CJK input method. ### Coordination with warpdotdev#9536 PR warpdotdev#9536 (open) modifies the same file but inside `maybe_adjust_window_vertically` (lines ~1433+ on master, adding 3 lines that early-return on maximize/fullscreen). My edit lands in `create_window` on lines 1422–1428, which warpdotdev#9536 does not touch — no merge conflict expected. ### Server API No server changes. ### Agent Mode Not applicable. ### Changelog Entries `CHANGELOG-BUG-FIX`: IME (input method) now activates on Linux/Wayland, restoring CJK and other non-Latin input that had been silently broken since first launch. Co-authored-by: anshul-garg27 <13553550+anshul-garg27@users.noreply.github.com> (cherry picked from commit 543d54e)
Closes warpdotdev#9383. ### Description `set_ime_allowed(true)` was only being called inside the `#[cfg(windows)]` block in `crates/warpui/src/windowing/winit/window.rs::create_window` (the block that ends around line 1421 on master). On Linux/Wayland, winit's text-input plugin only sends `zwp_text_input_v3.enable()` to the compositor after `set_ime_allowed(true)` is called — without that call IME stays inactive, so non-Latin input methods (CJK in particular) appear to do nothing in Warp. ### Fix Add a parallel `#[cfg(target_os = "linux")]` block right after the Windows one calling the same method on the created window. macOS continues to use the native NSTextInputClient route and does not need this winit hint. A repo-wide grep for `set_ime_allowed` confirms there is no other Linux-specific call site that was already covering this — the previous behaviour was "Windows-only" by oversight, not by design. ```rust #[cfg(target_os = "linux")] if let Ok(window) = created_window.as_ref() { // On Linux/Wayland, winit only sends `zwp_text_input_v3.enable()` when IME is allowed, // so without this call IME stays inactive and non-Latin input (CJK, etc.) is unusable. window.set_ime_allowed(true); } ``` ### Testing - `cargo fmt -p warpui -- --check` — passes. - Compile-only verification possible from my checkout (Metal toolchain unavailable for full `cargo nextest`, mirroring warpdotdev#9277). The change is a 7-line addition that mirrors the existing Windows block exactly. - Functional verification needs a Wayland session, which I can't run locally; CI on Linux runners should cover compilation. A maintainer with a Wayland setup could confirm IME behaviour with a CJK input method. ### Coordination with warpdotdev#9536 PR warpdotdev#9536 (open) modifies the same file but inside `maybe_adjust_window_vertically` (lines ~1433+ on master, adding 3 lines that early-return on maximize/fullscreen). My edit lands in `create_window` on lines 1422–1428, which warpdotdev#9536 does not touch — no merge conflict expected. ### Server API No server changes. ### Agent Mode Not applicable. ### Changelog Entries `CHANGELOG-BUG-FIX`: IME (input method) now activates on Linux/Wayland, restoring CJK and other non-Latin input that had been silently broken since first launch. Co-authored-by: anshul-garg27 <13553550+anshul-garg27@users.noreply.github.com>
Closes warpdotdev#9383. ### Description `set_ime_allowed(true)` was only being called inside the `#[cfg(windows)]` block in `crates/warpui/src/windowing/winit/window.rs::create_window` (the block that ends around line 1421 on master). On Linux/Wayland, winit's text-input plugin only sends `zwp_text_input_v3.enable()` to the compositor after `set_ime_allowed(true)` is called — without that call IME stays inactive, so non-Latin input methods (CJK in particular) appear to do nothing in Warp. ### Fix Add a parallel `#[cfg(target_os = "linux")]` block right after the Windows one calling the same method on the created window. macOS continues to use the native NSTextInputClient route and does not need this winit hint. A repo-wide grep for `set_ime_allowed` confirms there is no other Linux-specific call site that was already covering this — the previous behaviour was "Windows-only" by oversight, not by design. ```rust #[cfg(target_os = "linux")] if let Ok(window) = created_window.as_ref() { // On Linux/Wayland, winit only sends `zwp_text_input_v3.enable()` when IME is allowed, // so without this call IME stays inactive and non-Latin input (CJK, etc.) is unusable. window.set_ime_allowed(true); } ``` ### Testing - `cargo fmt -p warpui -- --check` — passes. - Compile-only verification possible from my checkout (Metal toolchain unavailable for full `cargo nextest`, mirroring warpdotdev#9277). The change is a 7-line addition that mirrors the existing Windows block exactly. - Functional verification needs a Wayland session, which I can't run locally; CI on Linux runners should cover compilation. A maintainer with a Wayland setup could confirm IME behaviour with a CJK input method. ### Coordination with warpdotdev#9536 PR warpdotdev#9536 (open) modifies the same file but inside `maybe_adjust_window_vertically` (lines ~1433+ on master, adding 3 lines that early-return on maximize/fullscreen). My edit lands in `create_window` on lines 1422–1428, which warpdotdev#9536 does not touch — no merge conflict expected. ### Server API No server changes. ### Agent Mode Not applicable. ### Changelog Entries `CHANGELOG-BUG-FIX`: IME (input method) now activates on Linux/Wayland, restoring CJK and other non-Latin input that had been silently broken since first launch. Co-authored-by: anshul-garg27 <13553550+anshul-garg27@users.noreply.github.com> (cherry picked from commit 543d54e)
Closes warpdotdev#9383. ### Description `set_ime_allowed(true)` was only being called inside the `#[cfg(windows)]` block in `crates/warpui/src/windowing/winit/window.rs::create_window` (the block that ends around line 1421 on master). On Linux/Wayland, winit's text-input plugin only sends `zwp_text_input_v3.enable()` to the compositor after `set_ime_allowed(true)` is called — without that call IME stays inactive, so non-Latin input methods (CJK in particular) appear to do nothing in Warp. ### Fix Add a parallel `#[cfg(target_os = "linux")]` block right after the Windows one calling the same method on the created window. macOS continues to use the native NSTextInputClient route and does not need this winit hint. A repo-wide grep for `set_ime_allowed` confirms there is no other Linux-specific call site that was already covering this — the previous behaviour was "Windows-only" by oversight, not by design. ```rust #[cfg(target_os = "linux")] if let Ok(window) = created_window.as_ref() { // On Linux/Wayland, winit only sends `zwp_text_input_v3.enable()` when IME is allowed, // so without this call IME stays inactive and non-Latin input (CJK, etc.) is unusable. window.set_ime_allowed(true); } ``` ### Testing - `cargo fmt -p warpui -- --check` — passes. - Compile-only verification possible from my checkout (Metal toolchain unavailable for full `cargo nextest`, mirroring warpdotdev#9277). The change is a 7-line addition that mirrors the existing Windows block exactly. - Functional verification needs a Wayland session, which I can't run locally; CI on Linux runners should cover compilation. A maintainer with a Wayland setup could confirm IME behaviour with a CJK input method. ### Coordination with warpdotdev#9536 PR warpdotdev#9536 (open) modifies the same file but inside `maybe_adjust_window_vertically` (lines ~1433+ on master, adding 3 lines that early-return on maximize/fullscreen). My edit lands in `create_window` on lines 1422–1428, which warpdotdev#9536 does not touch — no merge conflict expected. ### Server API No server changes. ### Agent Mode Not applicable. ### Changelog Entries `CHANGELOG-BUG-FIX`: IME (input method) now activates on Linux/Wayland, restoring CJK and other non-Latin input that had been silently broken since first launch. Co-authored-by: anshul-garg27 <13553550+anshul-garg27@users.noreply.github.com>
Closes warpdotdev#9383. ### Description `set_ime_allowed(true)` was only being called inside the `#[cfg(windows)]` block in `crates/warpui/src/windowing/winit/window.rs::create_window` (the block that ends around line 1421 on master). On Linux/Wayland, winit's text-input plugin only sends `zwp_text_input_v3.enable()` to the compositor after `set_ime_allowed(true)` is called — without that call IME stays inactive, so non-Latin input methods (CJK in particular) appear to do nothing in Warp. ### Fix Add a parallel `#[cfg(target_os = "linux")]` block right after the Windows one calling the same method on the created window. macOS continues to use the native NSTextInputClient route and does not need this winit hint. A repo-wide grep for `set_ime_allowed` confirms there is no other Linux-specific call site that was already covering this — the previous behaviour was "Windows-only" by oversight, not by design. ```rust #[cfg(target_os = "linux")] if let Ok(window) = created_window.as_ref() { // On Linux/Wayland, winit only sends `zwp_text_input_v3.enable()` when IME is allowed, // so without this call IME stays inactive and non-Latin input (CJK, etc.) is unusable. window.set_ime_allowed(true); } ``` ### Testing - `cargo fmt -p warpui -- --check` — passes. - Compile-only verification possible from my checkout (Metal toolchain unavailable for full `cargo nextest`, mirroring warpdotdev#9277). The change is a 7-line addition that mirrors the existing Windows block exactly. - Functional verification needs a Wayland session, which I can't run locally; CI on Linux runners should cover compilation. A maintainer with a Wayland setup could confirm IME behaviour with a CJK input method. ### Coordination with warpdotdev#9536 PR warpdotdev#9536 (open) modifies the same file but inside `maybe_adjust_window_vertically` (lines ~1433+ on master, adding 3 lines that early-return on maximize/fullscreen). My edit lands in `create_window` on lines 1422–1428, which warpdotdev#9536 does not touch — no merge conflict expected. ### Server API No server changes. ### Agent Mode Not applicable. ### Changelog Entries `CHANGELOG-BUG-FIX`: IME (input method) now activates on Linux/Wayland, restoring CJK and other non-Latin input that had been silently broken since first launch. Co-authored-by: anshul-garg27 <13553550+anshul-garg27@users.noreply.github.com>
Description
We currently have a bug with session restoration where maximized windows won't restore into the maximized state. This is due to the "vertical adjustment" code which tries to make sure the title bar is on screen (it moves the window downward if that isn't the case). Without getting too much into the details, on Windows when windows are maximized, winit reports the physical size of the window as if it had the native window decorations, even when it doesn't. It treats the native title bar as being off the screen and represents that with a negative origin (above the screen). This triggers
fn maybe_adjust_window_verticallyto return a value which triggers the caller to try to move the window which makes Windows un-maximize the window. This simple guard fixes the problemTesting
On this branch, that bug no longer occurs.