fix: wrong display position #241
Merged
Conversation
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
Fixes an issue where the window was placed in the wrong display in a system with 3 displays.
✅ Deploy Preview for hoppdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds a new cross-platform WindowManager that creates and manages per-monitor overlay windows, refactors Application to delegate window lifecycle and events to it, changes GraphicsContext::new to accept an Arc, and adds tests to iterate screenshare across every monitor. Changes
Sequence Diagram(s)sequenceDiagram
participant EventLoop as Event Loop
participant App as Application
participant WM as WindowManager
participant Mon as Monitor
participant Win as Window
rect rgba(100, 200, 150, 0.5)
EventLoop->>App: resumed(event_loop)
App->>WM: WindowManager::new(event_loop)
WM->>Mon: enumerate monitors
loop per monitor
WM->>Win: create overlay window (Arc\<Window\>)
Win-->>WM: Arc\<Window\>
WM->>WM: store WindowEntry(window, position)
end
WM-->>App: initialized
end
rect rgba(150, 150, 200, 0.5)
App->>WM: show_window(monitor_handle)
WM->>Win: set_fullscreen & set_visible(true)
Win-->>WM: visible Arc\<Window\>
WM-->>App: Arc\<Window\> returned
App->>App: start screenshare using Arc\<Window\>
end
rect rgba(200, 100, 100, 0.5)
EventLoop->>App: ScaleFactorChanged / Resized / Moved
App->>WM: update(event_loop)
WM->>Mon: re-enumerate monitors
WM->>WM: add/remove windows to match monitors
WM-->>App: update complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/lib.rs (1)
222-233: Don’t swallowWindowManager::updateerrors.Silent failures can mask monitor topology changes and lead to hard-to-debug window placement issues. Logging (or propagating) the error would help.
🛠️ Proposed fix
- if let Some(wm) = self.window_manager.as_mut() { - let _ = wm.update(event_loop); - } + if let Some(wm) = self.window_manager.as_mut() { + if let Err(e) = wm.update(event_loop) { + log::error!("get_available_content: WindowManager update failed: {e:?}"); + } + } @@ - if let Some(wm) = self.window_manager.as_mut() { - let _ = wm.update(event_loop); - } + if let Some(wm) = self.window_manager.as_mut() { + if let Err(e) = wm.update(event_loop) { + log::error!("window_event: WindowManager update failed: {e:?}"); + } + }Also applies to: 999-1004
🤖 Fix all issues with AI agents
In `@core/src/window_manager.rs`:
- Around line 121-218: Before removing stale entries in update(), capture the
current active window's monitor_position (if any) from
self.windows[self.active_window_index.unwrap()].monitor_position; then run the
retain as-is; after retain, re-find that monitor_position in the updated
self.windows (using iter().position(|entry| entry.monitor_position ==
saved_pos)) and set self.active_window_index = position_option (or None if not
found). This ensures hide_active_window(), update(), and other logic using
active_window_index remain valid; reference functions/fields: update(),
hide_active_window(), active_window_index, self.windows, and
entry.monitor_position.
In `@core/tests/src/screenshare_client.rs`:
- Around line 170-190: The test test_every_monitor should fail fast when no
display monitors are reported: after computing monitors from
get_available_content (filtering by ContentType::Display), check
monitors.is_empty() and return an io::Error (e.g., io::Error::new or
io::Error::other) with a clear message like "No display monitors available"
instead of allowing the test to continue and print "Success"; update the logic
around the monitors variable in test_every_monitor (and any downstream
success/print paths) so an empty monitors list yields an error rather than a
passing test.
🧹 Nitpick comments (1)
core/src/window_manager.rs (1)
221-281: Consider moving platform-specific fullscreen logic into OS modules.This file now contains OS-specific fullscreen handling; splitting that into
src/**/{linux,macos,windows}.rswould align with the project’s platform-module organization standard. As per coding guidelines, Organize platform-specific modules insrc/**/{linux,macos,windows}.rssubdirectories.
Comment on lines
+121
to
+218
| pub fn show_window( | ||
| &mut self, | ||
| monitor: &MonitorHandle, | ||
| ) -> Result<Arc<Window>, WindowManagerError> { | ||
| let monitor_position = monitor.position(); | ||
| log::info!( | ||
| "WindowManager::show_window: looking for window at {:?}", | ||
| monitor_position | ||
| ); | ||
|
|
||
| for entry in &self.windows { | ||
| log::info!( | ||
| "WindowManager::show_window: display {:?} window {:?}", | ||
| entry.monitor_position, | ||
| entry.window.outer_position() | ||
| ); | ||
| } | ||
| let index = self | ||
| .windows | ||
| .iter() | ||
| .position(|entry| entry.monitor_position == monitor_position) | ||
| .ok_or(WindowManagerError::MonitorNotFound)?; | ||
|
|
||
| let window = &self.windows[index].window; | ||
|
|
||
| log::info!("window fullscreen {:?}", window.fullscreen()); | ||
| if let Err(e) = set_fullscreen(window, monitor.clone()) { | ||
| log::error!( | ||
| "WindowManager::show_window: error setting fullscreen: {:?}", | ||
| e | ||
| ); | ||
| return Err(WindowManagerError::FullscreenError(e.to_string())); | ||
| } | ||
|
|
||
| window.set_visible(true); | ||
| self.active_window_index = Some(index); | ||
|
|
||
| Ok(window.clone()) | ||
| } | ||
|
|
||
| pub fn hide_active_window(&mut self) { | ||
| if let Some(index) = self.active_window_index.take() { | ||
| log::info!( | ||
| "WindowManager::hide_active_window: hiding window at index {}", | ||
| index | ||
| ); | ||
| self.windows[index].window.set_visible(false); | ||
| } | ||
| } | ||
|
|
||
| pub fn update(&mut self, event_loop: &ActiveEventLoop) -> Result<(), WindowManagerError> { | ||
| let monitors: Vec<MonitorHandle> = event_loop.available_monitors().collect(); | ||
| let mut monitor_positions: Vec<PhysicalPosition<i32>> = | ||
| monitors.iter().map(|m| m.position()).collect(); | ||
|
|
||
| log::info!( | ||
| "WindowManager::update: checking {} monitors", | ||
| monitors.len() | ||
| ); | ||
|
|
||
| self.windows.retain(|entry| { | ||
| if let Some(pos_index) = monitor_positions | ||
| .iter() | ||
| .position(|&pos| pos == entry.monitor_position) | ||
| { | ||
| monitor_positions.remove(pos_index); | ||
| true | ||
| } else { | ||
| log::info!( | ||
| "WindowManager::update: removing window at outdated position {:?}", | ||
| entry.monitor_position | ||
| ); | ||
| false | ||
| } | ||
| }); | ||
|
|
||
| for position in monitor_positions { | ||
| log::info!( | ||
| "WindowManager::update: adding new window for position {:?}", | ||
| position | ||
| ); | ||
|
|
||
| let monitor = monitors | ||
| .iter() | ||
| .find(|m| m.position() == position) | ||
| .ok_or(WindowManagerError::MonitorNotFound)?; | ||
|
|
||
| let window_entry = Self::create_window_entry(event_loop, monitor)?; | ||
| self.windows.push(window_entry); | ||
| } | ||
|
|
||
| log::info!( | ||
| "WindowManager::update: windows list length: {:?}", | ||
| self.windows.len() | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Prevent stale active_window_index after monitor changes.
update() can remove entries, but active_window_index isn’t revalidated. That can lead to an out-of-bounds access in hide_active_window() after a monitor topology change.
🛠️ Proposed fix
pub fn hide_active_window(&mut self) {
if let Some(index) = self.active_window_index.take() {
log::info!(
"WindowManager::hide_active_window: hiding window at index {}",
index
);
- self.windows[index].window.set_visible(false);
+ if let Some(entry) = self.windows.get(index) {
+ entry.window.set_visible(false);
+ }
}
}
pub fn update(&mut self, event_loop: &ActiveEventLoop) -> Result<(), WindowManagerError> {
+ let active_position = self
+ .active_window_index
+ .and_then(|i| self.windows.get(i).map(|e| e.monitor_position));
+
let monitors: Vec<MonitorHandle> = event_loop.available_monitors().collect();
let mut monitor_positions: Vec<PhysicalPosition<i32>> =
monitors.iter().map(|m| m.position()).collect();
@@
for position in monitor_positions {
log::info!(
"WindowManager::update: adding new window for position {:?}",
position
);
@@
let window_entry = Self::create_window_entry(event_loop, monitor)?;
self.windows.push(window_entry);
}
+
+ self.active_window_index = active_position
+ .and_then(|pos| self.windows.iter().position(|e| e.monitor_position == pos));🤖 Prompt for AI Agents
In `@core/src/window_manager.rs` around lines 121 - 218, Before removing stale
entries in update(), capture the current active window's monitor_position (if
any) from self.windows[self.active_window_index.unwrap()].monitor_position; then
run the retain as-is; after retain, re-find that monitor_position in the updated
self.windows (using iter().position(|entry| entry.monitor_position ==
saved_pos)) and set self.active_window_index = position_option (or None if not
found). This ensures hide_active_window(), update(), and other logic using
active_window_index remain valid; reference functions/fields: update(),
hide_active_window(), active_window_index, self.windows, and
entry.monitor_position.
Comment on lines
+170
to
+190
| pub fn test_every_monitor() -> io::Result<()> { | ||
| let mut socket = connect_socket()?; | ||
| println!("Connected to socket."); | ||
|
|
||
| let livekit_server_url = | ||
| env::var("LIVEKIT_URL").expect("LIVEKIT_URL environment variable not set"); | ||
| socket.send_message(Message::LivekitServerUrl(livekit_server_url))?; | ||
|
|
||
| let available_content = match get_available_content(&mut socket)? { | ||
| Message::AvailableContent(available_content) => available_content, | ||
| _ => return Err(io::Error::other("Failed to get available content")), | ||
| }; | ||
|
|
||
| let monitors: Vec<_> = available_content | ||
| .content | ||
| .into_iter() | ||
| .filter(|c| matches!(c.content.content_type, ContentType::Display)) | ||
| .collect(); | ||
|
|
||
| println!("Found {} monitors to test.", monitors.len()); | ||
|
|
There was a problem hiding this comment.
Fail fast when no display monitors are available.
Right now the test prints “Success” even if zero displays are reported, which can mask regressions. Consider returning an error when monitors is empty.
🛠️ Proposed fix
println!("Found {} monitors to test.", monitors.len());
+ if monitors.is_empty() {
+ return Err(io::Error::other("No display monitors available to test"));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn test_every_monitor() -> io::Result<()> { | |
| let mut socket = connect_socket()?; | |
| println!("Connected to socket."); | |
| let livekit_server_url = | |
| env::var("LIVEKIT_URL").expect("LIVEKIT_URL environment variable not set"); | |
| socket.send_message(Message::LivekitServerUrl(livekit_server_url))?; | |
| let available_content = match get_available_content(&mut socket)? { | |
| Message::AvailableContent(available_content) => available_content, | |
| _ => return Err(io::Error::other("Failed to get available content")), | |
| }; | |
| let monitors: Vec<_> = available_content | |
| .content | |
| .into_iter() | |
| .filter(|c| matches!(c.content.content_type, ContentType::Display)) | |
| .collect(); | |
| println!("Found {} monitors to test.", monitors.len()); | |
| pub fn test_every_monitor() -> io::Result<()> { | |
| let mut socket = connect_socket()?; | |
| println!("Connected to socket."); | |
| let livekit_server_url = | |
| env::var("LIVEKIT_URL").expect("LIVEKIT_URL environment variable not set"); | |
| socket.send_message(Message::LivekitServerUrl(livekit_server_url))?; | |
| let available_content = match get_available_content(&mut socket)? { | |
| Message::AvailableContent(available_content) => available_content, | |
| _ => return Err(io::Error::other("Failed to get available content")), | |
| }; | |
| let monitors: Vec<_> = available_content | |
| .content | |
| .into_iter() | |
| .filter(|c| matches!(c.content.content_type, ContentType::Display)) | |
| .collect(); | |
| println!("Found {} monitors to test.", monitors.len()); | |
| if monitors.is_empty() { | |
| return Err(io::Error::other("No display monitors available to test")); | |
| } |
🤖 Prompt for AI Agents
In `@core/tests/src/screenshare_client.rs` around lines 170 - 190, The test
test_every_monitor should fail fast when no display monitors are reported: after
computing monitors from get_available_content (filtering by
ContentType::Display), check monitors.is_empty() and return an io::Error (e.g.,
io::Error::new or io::Error::other) with a clear message like "No display
monitors available" instead of allowing the test to continue and print
"Success"; update the logic around the monitors variable in test_every_monitor
(and any downstream success/print paths) so an empty monitors list yields an
error rather than a passing test.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/lib.rs (1)
219-230: Don’t silently ignoreWindowManager::updatefailures.If window creation fails during update, swallowing the error makes later failures harder to diagnose and leaves stale state. At least log the failure here.
✅ Suggested tweak
if let Some(wm) = self.window_manager.as_mut() { - let _ = wm.update(event_loop); + if let Err(e) = wm.update(event_loop) { + log::error!("get_available_content: WindowManager update failed: {e:?}"); + } }
🤖 Fix all issues with AI agents
In `@core/src/lib.rs`:
- Around line 961-971: resumed() currently only creates a WindowManager when
self.window_manager is None, which leaves a stale manager after monitor topology
changes; change the logic in Application::resumed to always attempt to (re)build
or refresh the window manager: call
window_manager::WindowManager::new(event_loop) on every resume and on Ok replace
self.window_manager with the new instance (dropping the old one), and on Err log
the error (using the same log::error path) so the app uses an up-to-date
WindowManager after resume.
🧹 Nitpick comments (1)
core/src/window_manager.rs (1)
90-108: Split OS-specific behavior into dedicated platform modules.This file mixes macOS/Windows/Linux code paths inline. Please extract platform-specific window handling into per-OS modules under
core/src/**/{linux,macos,windows}.rsand call them from the shared manager.As per coding guidelines,
core/src/**/*.rs: Organize platform-specific modules insrc/**/{linux,macos,windows}.rssubdirectories.Also applies to: 240-285
Comment on lines
+961
to
+971
| fn resumed(&mut self, event_loop: &ActiveEventLoop) { | ||
| if self.window_manager.is_none() { | ||
| log::info!("Application::resumed: initializing WindowManager"); | ||
| match window_manager::WindowManager::new(event_loop) { | ||
| Ok(wm) => self.window_manager = Some(wm), | ||
| Err(e) => log::error!( | ||
| "Application::resumed: failed to initialize WindowManager: {:?}", | ||
| e | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
Refresh WindowManager on resume to handle monitor topology changes.
resumed() only initializes when None. If displays change while the app is suspended, the existing WindowManager can become stale and target the wrong display or fail to find a monitor. Update (or rebuild) it on every resume.
🛠️ Suggested fix
fn resumed(&mut self, event_loop: &ActiveEventLoop) {
- if self.window_manager.is_none() {
- log::info!("Application::resumed: initializing WindowManager");
- match window_manager::WindowManager::new(event_loop) {
- Ok(wm) => self.window_manager = Some(wm),
- Err(e) => log::error!(
- "Application::resumed: failed to initialize WindowManager: {:?}",
- e
- ),
- }
- }
+ if let Some(wm) = self.window_manager.as_mut() {
+ if let Err(e) = wm.update(event_loop) {
+ log::error!(
+ "Application::resumed: failed to update WindowManager: {:?}",
+ e
+ );
+ }
+ return;
+ }
+
+ log::info!("Application::resumed: initializing WindowManager");
+ match window_manager::WindowManager::new(event_loop) {
+ Ok(wm) => self.window_manager = Some(wm),
+ Err(e) => log::error!(
+ "Application::resumed: failed to initialize WindowManager: {:?}",
+ e
+ ),
+ }
}🤖 Prompt for AI Agents
In `@core/src/lib.rs` around lines 961 - 971, resumed() currently only creates a
WindowManager when self.window_manager is None, which leaves a stale manager
after monitor topology changes; change the logic in Application::resumed to
always attempt to (re)build or refresh the window manager: call
window_manager::WindowManager::new(event_loop) on every resume and on Ok replace
self.window_manager with the new instance (dropping the old one), and on Err log
the error (using the same log::error path) so the app uses an up-to-date
WindowManager after resume.
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.
Fixes an issue where the window was placed in the wrong display in a system with 3 displays.
Summary by CodeRabbit
New Features
Refactoring
Tests
✏️ Tip: You can customize this high-level summary in your review settings.