Conversation
This comment was marked as outdated.
This comment was marked as outdated.
2736291 to
d687474
Compare
02b0057 to
354aeda
Compare
tmandry
left a comment
There was a problem hiding this comment.
- New actors should have tests; we can mock out their system calls with
#[cfg(test)] - App actor can remove its dependence on sys::window_server::get_windows: Just call this in WindowServer
This fixes an issue where the events going through window_server could get reordered wrt the events being sent directly to the reactor.
Change animation_focus_wid from Option<WindowId> to a Vec<WindowId> so that multiple windows can be marked as "new" during a single event handling pass. This allows all of them to be resized immediately rather than animated, which is the correct behavior when multiple windows become visible at once. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…indowServer Introduce a `WindowsOnScreen` struct that bundles the visible window ID list with the full `WindowServerInfo` vec. The WindowServer actor constructs this struct and feeds it forward through WmController to the Reactor, centralizing window list queries in one place. The WmController no longer calls `get_visible_windows_with_layer` directly. For WmController-initiated space changes (login window, expose exit, toggle commands), the `SpaceChanged` event carries `None` for the window list since those paths don't have a fresh snapshot. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rom other events Extract window-on-screen information into a dedicated `reactor::Event::WindowsOnScreenUpdated` event. This removes the `on_screen` field from `ScreenParametersChanged`, simplifies `SpaceChanged` to a single argument, and removes `on_screen` from `ApplicationLaunched`. The WindowServer actor now sends `WindowsOnScreenUpdated` after `ScreenParametersChanged`, `SpaceChanged`, and `WindowCreated`. App actors send it before `ApplicationLaunched` so visible windows are populated when the reactor processes the launch. --- This centralizes knowledge of window-on-screen lists in the WindowServer actor, preparing for layer filtering and caching in follow-up commits. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Only report windows on layers 0 (normal), 3 (floating), and 8 (status panels). This drops system UI elements like the menu bar, screensaver overlays, and other high-layer windows before they reach the reactor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…vents Only send WindowsOnScreenUpdated when the list of visible window IDs actually changes, reducing unnecessary work in the reactor when the window server state hasn't changed between consecutive queries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
SpaceChanged now always carries a WindowsOnScreen snapshot alongside the space list. WmController passes WindowsOnScreen::default() for now – this will be replaced when SpaceManager is introduced. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
SpaceManager lives on the reactor thread alongside WindowServer and Reactor. WindowServer now sends reactor events through SpaceManager instead of directly to the Reactor. For now SpaceManager simply forwards everything unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
SpaceManager now owns all space/screen enablement state (starting_space, cur_space, disabled/enabled spaces, login_window_active, expose_active, is_globally_enabled). WindowServer sends ScreenParametersChanged and SpaceChanged directly to SpaceManager instead of WmController. SpaceManager sends HotkeysActive(bool) to WmController when the hotkey registration state should change. SpaceManager sends RequestSpaceRefresh to WindowServer when it needs a fresh window list (after toggling a space, expose exit, or login window state change). WmController is slimmed to handle only main-thread concerns: hotkey registration, app launching, login_window_pid detection, and command dispatch. It forwards events to SpaceManager via sm_tx. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This isn't necessary because it can just send commands via the wm_tx.
4626e72 to
c5cdebb
Compare
|
/gemini review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request introduces a major architectural refactor by adding a SpaceManager actor and restructuring the WindowServer. The SpaceManager now centralizes space enablement logic, global state management (login window, expose mode), and hotkey activation, which were previously handled by the WmController. The WindowServer has been split into a main-thread SkylightWatcher for system events and an off-main-thread actor that enriches application events with window server metadata. Additionally, WindowId now utilizes a bitmask to differentiate between system and manual indices, and the Reactor has been updated to use WindowsOnScreen snapshots for more reliable state synchronization. Review feedback suggests grouping the extensive list of channel arguments in Reactor::spawn into a struct to improve readability and maintainability.
| pub fn spawn( | ||
| config: Arc<Config>, | ||
| one_space: bool, | ||
| layout: LayoutManager, | ||
| record: Record, | ||
| mouse_tx: mouse::Sender, | ||
| status_tx: status::Sender, | ||
| group_indicators_tx: group_bars::Sender, | ||
| ) -> Sender { | ||
| let (events_tx, events) = crate::actor::channel(); | ||
| let events_tx_clone = events_tx.clone(); | ||
| reactor_tx: Sender, | ||
| events: Receiver, | ||
| wm_tx: wm_controller::Sender, | ||
| ws_tx: window_server::Sender, | ||
| ws_rx: window_server::Receiver, | ||
| sm_tx: space_manager::Sender, | ||
| sm_rx: space_manager::Receiver, | ||
| skylight_tx: window_server::SkylightSender, | ||
| ) { |
There was a problem hiding this comment.
This function has a large number of arguments (16), which can make it hard to use and maintain. While clippy::too_many_arguments is suppressed, it would be beneficial to group related arguments into a struct. For instance, all the actor channels could be grouped into a single Channels struct. This would improve readability and make the function signature more manageable.
Example:
struct ActorChannels {
mouse_tx: mouse::Sender,
status_tx: status::Sender,
group_indicators_tx: group_bars::Sender,
reactor_tx: reactor::Sender,
reactor_rx: reactor::Receiver,
wm_tx: wm_controller::Sender,
ws_tx: window_server::Sender,
ws_rx: window_server::Receiver,
sm_tx: space_manager::Sender,
sm_rx: space_manager::Receiver,
skylight_tx: window_server::SkylightSender,
}
// ...
pub fn spawn(
config: Arc<Config>,
one_space: bool,
layout: LayoutManager,
record: Record,
channels: ActorChannels,
) {
// ...
}There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Event::WindowsOnScreenUpdated { pid, on_screen } => match pid { | ||
| Some(_) => self.update_partial_window_server_info(on_screen), | ||
| None => self.update_complete_window_server_info(on_screen), | ||
| }, |
There was a problem hiding this comment.
Handling Event::WindowsOnScreenUpdated only updates visible_windows/window_server_info, but it does not trigger any follow-up to refresh accessibility-visible windows (e.g. Request::GetVisibleWindows / on_windows_discovered). As a result, visibility-only changes (like minimize/unminimize, which now emit WindowVisibilityChanged → WindowsOnScreenUpdated) won't update the layout until the next space change. Consider requesting GetVisibleWindows for the affected app (when pid is Some) or calling update_visible_windows() after applying the snapshot.
| Event::WindowsOnScreenUpdated { pid, on_screen } => match pid { | |
| Some(_) => self.update_partial_window_server_info(on_screen), | |
| None => self.update_complete_window_server_info(on_screen), | |
| }, | |
| Event::WindowsOnScreenUpdated { pid, on_screen } => { | |
| match pid { | |
| Some(_) => self.update_partial_window_server_info(on_screen), | |
| None => self.update_complete_window_server_info(on_screen), | |
| } | |
| self.update_visible_windows(); | |
| } |
|
I've been using this for a few days without running into regressions. Should be ready to merge. |
Significant refactor of the actors. This centralizes knowledge of window server state in the WindowServer actor, including the current space, screen configuration, and windows visible on screen.
This refactoring is intended as a lead up to supporting #65. It's also useful in its own right in that it gives each actor a clearer scope and set of responsibilities.