Repository navigation
feat(window-wrapper): generic window registry for focus-existing-or-open-new - #19
Merged
Merged
Conversation
…or-open-new Extracts the ad-hoc SettingsWindowHandle pattern into a reusable WindowRegistry keyed by window-kind, so any window type gets duplicate-prevention for free. Also fixes focus: reopening an already-open window now calls Window::activate_window() to bring it forward, instead of silently no-oping.
…NT-13 Adds crates/app/src/debug.rs: a mock startup window with a button to open the real main window, and a "Debug: Open Settings + Project" menu action that opens Settings plus a stub project window together, so WindowRegistry dedup/focus can be exercised by hand across multiple window kinds at once. Everything is isolated behind DEBUG comments and marked for removal once ASNT-13 is manually verified, before ASNT-15 replaces the stub with the real project-creation wizard.
Owner
Author
|
Pushed a second commit (
Everything is isolated in |
crate::App (this crate's main window struct) and gpui::App (the context type) share a name. An explicit `use crate::App` in debug.rs, and an untyped `&mut App` param in main.rs's extracted open_settings_window, both silently resolved to the local struct instead of gpui::App since explicit imports/same-module items win over glob imports regardless of source order. Fixes it by aliasing the local import and spelling out gpui::App where needed.
Unrelated to ASNT-13, but was blocking this PR's CI entirely since the format-check step runs before clippy/build/test and was already failing on main: rustfmt drift in crates/table (panel.rs, selection.rs) and a clippy redundant-closure lint in status_items/mod.rs. Verified with the exact CI command (cargo fmt --all --check, cargo build, cargo clippy --workspace --all-targets -- -D warnings -A dead_code) — all pass now.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a reusable WindowRegistry global (in crates/window-wrapper) to deduplicate “focus existing or open new” behavior across different window kinds (starting with Settings), and updates the app to use it. It also adds temporary debug scaffolding to manually exercise multi-window behavior.
Changes:
- Added
WindowRegistrykeyed by window kind, withfocus_or_clearthat activates an existing window or clears stale handles. - Rewired Settings window opening in
crates/app/src/main.rsto useWindowRegistry, removing the old Settings-specific global handle from thesettingscrate. - Added debug-only (intended) scaffolding: debug menu action and debug startup window to test multi-window behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/window-wrapper/src/window_registry.rs | New global registry for per-kind window handle tracking and focus/clear behavior. |
| crates/window-wrapper/src/lib.rs | Exposes window_registry module and re-exports WindowRegistry. |
| crates/table/src/selection.rs | Formatting change in an if condition (currently not rustfmt-compliant). |
| crates/table/src/panel.rs | Minor refactor/formatting simplification for cx.new(...) call. |
| crates/settings/src/lib.rs | Removes SettingsWindowHandle global in favor of generic registry. |
| crates/app/src/status_items/mod.rs | Minor closure simplification (cx.new(CellLocation::new)). |
| crates/app/src/main.rs | Uses WindowRegistry for Settings; adds debug scaffolding and (currently) changes startup behavior. |
| crates/app/src/debug.rs | New debug scaffolding module for manual multi-window testing. |
| crates/app/src/app_menus.rs | Adds debug menu item (currently unconditional). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+27
to
34
| if delegate.editing | ||
| == (EditState::Editing { | ||
| row: row_ix, | ||
| col: col_ix, | ||
| }) | ||
| { | ||
| return Input::new(&delegate.editor).into_any_element(); | ||
| } |
| mod app_menus; | ||
| mod app_settings; | ||
| mod components; | ||
| mod debug; // DEBUG (ASNT-13 manual test scaffolding) — see crates/app/src/debug.rs |
Comment on lines
22
to
29
| use crate::app_settings::build_pages; | ||
| use crate::{ | ||
| actions::{ToggleBottomDock, ToggleLeftDock, ToggleRightDock}, | ||
| app_menus::{OpenSettings, Quit, app_menus}, | ||
| debug::register_debug_menu_action, // DEBUG (ASNT-13 manual test scaffolding) | ||
| status_items::build_status_bar_registry, | ||
| title_items::build_title_bar_registry, | ||
| }; |
Comment on lines
+3
to
14
| use crate::debug::DebugOpenBoth; // DEBUG (ASNT-13 manual test scaffolding) | ||
|
|
||
| actions!(nav, [OpenSettings, Quit]); | ||
|
|
||
| pub fn app_menus() -> Vec<Menu> { | ||
| vec
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
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
WindowRegistry(crates/window-wrapper) keyed by window-kind string, generalizing the Settings-onlySettingsWindowHandlepattern per ASNT-13.WindowRegistry::focus_or_clearalso callsWindow::activate_window(), so a second open request actually foregrounds the window instead of no-oping.OpenSettingsincrates/app/src/main.rsthrough the new registry; removesSettingsWindowHandlefrom thesettingscrate.Test plan
cargo buildcargo clippy --all-targets -- -D warningscargo run, open Settings twice — confirm the second attempt focuses the existing window instead of duplicating (native GUI, needs a human — not drivable from this session)