weird behavior on wake with multiple displays

[acsandmann]

On main now (v0.3.8), on wake I get one of two behaviours:

1. Windows stay in correct layout until I click/activate a window, and then the window layout resets
2. Windows rapidly oscillate between displays

See debug logs attached for each scenario.

038test3_wake_window_oscillation_snippet.log
038test_wake_layout_reset_snippet.log

Originally posted by @adamjhf in #217

[brianwk]

I get the same experience. I have to quit Rift and restart it when I plug in my display at the office. Otherwise the windows go all wonky and it's actually impossible to use my computer until I restart Rift.

[acsandmann]

I get the same experience. I have to quit Rift and restart it when I plug in my display at the office. Otherwise the windows go all wonky and it's actually impossible to use my computer until I restart Rift.

do you have complete logs of this? the logs provided by adamjhf are not really complete and i can't really reproduce it

[brianwk]

I will have to get some logs when I go to the office again. Either tomorrow or Monday.

[acsandmann]

this may fix it

diff --git a/src/actor/reactor/events/window.rs b/src/actor/reactor/events/window.rs
index e058cf2..53438f9 100644
--- a/src/actor/reactor/events/window.rs
+++ b/src/actor/reactor/events/window.rs
@@ -235,10 +235,6 @@ impl WindowEventHandler {
                 return false;
             }
 
-            if requested.0 {
-                return false;
-            }
-
             if triggered_by_rift {
                 let Some(window) = reactor.window_manager.windows.get_mut(&wid) else {
                     return false;
@@ -274,6 +270,10 @@ impl WindowEventHandler {
                 return false;
             }
 
+            if requested.0 {
+                return false;
+            }
+
             let old_space = reactor.best_space_for_window(&old_frame, server_id);
             let new_space = reactor.best_space_for_window(&new_frame, server_id);
             let old_active = old_space.is_some_and(|space| reactor.is_space_active(space));

@adamjhf see if this works

[adamjhf]

@acsandmann I tried that patch on main, but still get the window layout reset issues unfortunately.

I notice there are two different scenarios (wake without login screen and wake with login screen), but on main with that patch I get window layout reset issue for both, and specifically for the no-login-screen scenario I also get the rapid window movement.

This diff seems to generally fix for both scenarios for me:

diff --git a/src/actor/notification_center.rs b/src/actor/notification_center.rs
index 451ef7f..bf83176 100644
--- a/src/actor/notification_center.rs
+++ b/src/actor/notification_center.rs
@@ -27,10 +27,10 @@ const REFRESH_DEFAULT_DELAY_NS: i64 = 150_000_000;
 const REFRESH_RETRY_DELAY_NS: i64 = 150_000_000;
 const REFRESH_MAX_RETRIES: u8 = 10;
 
-const DISPLAY_CHURN_QUIET_NS: i64 = 300_000_000;
-const DISPLAY_STABILIZE_RETRY_NS: i64 = 200_000_000;
-const DISPLAY_STABILIZE_MAX_ATTEMPTS: u8 = 25;
-const DISPLAY_STABLE_REQUIRED_HITS: u8 = 2;
+const DISPLAY_CHURN_QUIET_NS: i64 = 2_000_000_000;
+const DISPLAY_STABILIZE_RETRY_NS: i64 = 400_000_000;
+const DISPLAY_STABILIZE_MAX_ATTEMPTS: u8 = 8;
+const DISPLAY_STABLE_REQUIRED_HITS: u8 = 3;
 
 #[derive(Debug, Clone, PartialEq, Eq)]
 struct DisplayTopologyFingerprint(Vec<(String, u64, u64, u64, u64)>);
@@ -155,6 +155,11 @@ impl NotificationCenterInner {
         if unsafe { NSWorkspaceActiveSpaceDidChangeNotification } == name
             || name.to_string() == "NSWorkspaceActiveDisplayDidChangeNotification"
         {
+            let ivars = self.ivars();
+            if ivars.display_churn_active.get() {
+                trace!("Skipping screen changed event during display churn");
+                return;
+            }
             self.send_current_space();
         } else {
             warn!("Unexpected screen changed event: {notif:?}");
diff --git a/src/actor/reactor.rs b/src/actor/reactor.rs
index 9cf9475..58924af 100644
--- a/src/actor/reactor.rs
+++ b/src/actor/reactor.rs
@@ -257,6 +257,7 @@ pub struct Reactor {
     active_spaces: HashSet<SpaceId>,
     display_churn_active: bool,
     display_churn_pending_full_refresh: bool,
+    space_display_snapshot: Option<HashMap<SpaceId, Option<String>>>,
 }
 
 impl Reactor {
@@ -382,6 +383,7 @@ impl Reactor {
             active_spaces: HashSet::default(),
             display_churn_active: false,
             display_churn_pending_full_refresh: false,
+            space_display_snapshot: None,
         }
     }
 
@@ -692,6 +694,30 @@ impl Reactor {
 
     fn recover_from_display_churn(&mut self) {
         self.display_churn_pending_full_refresh = false;
+
+        if let Some(snapshot) = self.space_display_snapshot.take() {
+            for (old_space, display_uuid_opt) in snapshot {
+                if let Some(display_uuid) = display_uuid_opt {
+                    if let Some(new_space) =
+                        crate::sys::screen::current_space_for_display_uuid(&display_uuid)
+                    {
+                        if old_space != new_space
+                            && self
+                                .layout_manager
+                                .layout_engine
+                                .space_display_map()
+                                .contains_key(&old_space)
+                        {
+                            self.layout_manager.layout_engine.remap_space(old_space, new_space);
+                        }
+                    }
+                }
+            }
+        }
+
+        let spaces: Vec<Option<SpaceId>> =
+            self.space_manager.screens.iter().map(|screen| screen.space).collect();
+        self.reconcile_spaces_with_display_history(&spaces, true);
         self.refresh_window_server_snapshot_after_churn();
         self.resync_windows_after_churn();
         self.force_refresh_all_windows();
@@ -764,6 +790,8 @@ impl Reactor {
             Event::DisplayChurnBegin => {
                 self.display_churn_active = true;
                 self.display_churn_pending_full_refresh = true;
+                self.space_display_snapshot =
+                    Some(self.layout_manager.layout_engine.space_display_map().clone());
                 return;
             }
             Event::DisplayChurnEnd => {
@@ -1144,6 +1172,7 @@ impl Reactor {
         allow_remap: bool,
     ) {
         let mut seen_displays: HashSet<String> = HashSet::default();
+        let skip_updates = self.display_churn_active || self.display_churn_pending_full_refresh;
 
         for (screen, space_opt) in self.space_manager.screens.iter().zip(spaces.iter()) {
             let Some(space) = space_opt else {
@@ -1174,9 +1203,11 @@ impl Reactor {
                     }
                 }
             }
-            self.layout_manager
-                .layout_engine
-                .update_space_display(*space, Some(display_uuid.to_string()));
+            if !skip_updates {
+                self.layout_manager
+                    .layout_engine
+                    .update_space_display(*space, Some(display_uuid.to_string()));
+            }
         }
     }
 
diff --git a/src/actor/reactor/managers.rs b/src/actor/reactor/managers.rs
index 64e5568..bd1e149 100644
--- a/src/actor/reactor/managers.rs
+++ b/src/actor/reactor/managers.rs
@@ -222,10 +222,12 @@ impl LayoutManager {
                 .layout
                 .gaps
                 .effective_for_display(display_uuid_opt.as_deref());
-            reactor
-                .layout_manager
-                .layout_engine
-                .update_space_display(space, display_uuid_opt.clone());
+            if !reactor.display_churn_active && !reactor.display_churn_pending_full_refresh {
+                reactor
+                    .layout_manager
+                    .layout_engine
+                    .update_space_display(space, display_uuid_opt.clone());
+            }
             let layout =
                 reactor.layout_manager.layout_engine.calculate_layout_with_virtual_workspaces(
                     space,
diff --git a/src/layout_engine/engine.rs b/src/layout_engine/engine.rs
index cc44ec6..5f6aea5 100644
--- a/src/layout_engine/engine.rs
+++ b/src/layout_engine/engine.rs
@@ -1725,6 +1725,10 @@ impl LayoutEngine {
         &mut self.virtual_workspace_manager
     }
 
+    pub(crate) fn space_display_map(&self) -> &HashMap<SpaceId, Option<String>> {
+        &self.space_display_map
+    }
+
     pub fn active_workspace(&self, space: SpaceId) -> Option<crate::model::VirtualWorkspaceId> {
         self.virtual_workspace_manager.active_workspace(space)
     }

This prevents some layout updates while display churn is still active. It also captures snapshot of spaces at start of churn and applies these when recovering from churn, otherwise multiple SpaceChanged can cause the old space IDs to no longer exist.

[adamjhf]

Here are full debug logs for different scenarios, using your patch on main (v0.39):

1. Wake with no login window, windows rapidly move between displays - no_login_rapid_movement.log
2. Wake with no login window, window layout resets - no_login_layout_reset.log
3. Wake with login window, window layout resets - login_window_layout_reset.log

[adamjhf]

My patch above has been working quite consistently for me so far.

[acsandmann]

@adamjhf sorry for all the deleted mentions, i kept on finding "almost" solutions and then encountering a weird edge case. your patch solved your specific bug but it broke a few other things which is why i didn't want to commit it. however, i think the patch below should resolve all issues. let me know:

diff --git a/src/actor/reactor.rs b/src/actor/reactor.rs
index 5970e03..51d85ee 100644
--- a/src/actor/reactor.rs
+++ b/src/actor/reactor.rs
@@ -257,6 +257,7 @@ pub struct Reactor {
     active_spaces: HashSet<SpaceId>,
     display_churn_active: bool,
     display_churn_pending_full_refresh: bool,
+    display_churn_saw_screen_refresh: bool,
 }
 
 impl Reactor {
@@ -382,6 +383,7 @@ impl Reactor {
             active_spaces: HashSet::default(),
             display_churn_active: false,
             display_churn_pending_full_refresh: false,
+            display_churn_saw_screen_refresh: false,
         }
     }
 
@@ -658,40 +660,58 @@ impl Reactor {
     }
 
     fn handle_loop_event(&mut self, event: Event) {
-        let is_screen_params_changed = matches!(event, Event::ScreenParametersChanged(..));
-        if self.display_churn_active
-            && !matches!(
-                event,
-                Event::ScreenParametersChanged(..)
-                    | Event::DisplayChurnBegin
-                    | Event::DisplayChurnEnd
-            )
-        {
-            Self::note_windowserver_activity_during_display_churn(&event);
-            if matches!(
-                event,
-                Event::WindowServerDestroyed(..)
-                    | Event::WindowServerAppeared(..)
-                    | Event::ResyncAppForWindow(..)
-            ) {
-                trace!("reactor_drop event={:?}", event);
-            }
-            self.display_churn_pending_full_refresh = true;
+        // During display churn we hard-pause operational processing. Once churn
+        // ends, we continue pausing until we get a fresh display snapshot.
+        if self.display_churn_active {
+            match event {
+                Event::DisplayChurnBegin | Event::DisplayChurnEnd => {
+                    self.handle_event(event);
+                }
+                Event::ScreenParametersChanged(..) => {
+                    // Keep the latest display snapshot while churn is active, but
+                    // continue deferring operational processing until churn ends.
+                    self.handle_event(event);
+                    self.display_churn_pending_full_refresh = true;
+                    self.display_churn_saw_screen_refresh = true;
+                }
+                _ => {
+                    Self::note_windowserver_activity_during_display_churn(&event);
+                    if matches!(
+                        event,
+                        Event::WindowServerDestroyed(..)
+                            | Event::WindowServerAppeared(..)
+                            | Event::ResyncAppForWindow(..)
+                    ) {
+                        trace!("reactor_drop event={:?}", event);
+                    }
+                    self.display_churn_pending_full_refresh = true;
+                }
+            }
             return;
         }
 
-        self.handle_event(event);
-
-        if is_screen_params_changed
-            && self.display_churn_pending_full_refresh
-            && !self.display_churn_active
-        {
-            self.recover_from_display_churn();
+        if self.display_churn_pending_full_refresh {
+            match event {
+                Event::DisplayChurnBegin | Event::DisplayChurnEnd => {
+                    self.handle_event(event);
+                }
+                Event::ScreenParametersChanged(..) => {
+                    self.handle_event(event);
+                    self.recover_from_display_churn();
+                }
+                _ => {
+                    Self::note_windowserver_activity_during_display_churn(&event);
+                }
+            }
+            return;
         }
+
+        self.handle_event(event);
     }
 
     fn recover_from_display_churn(&mut self) {
         self.display_churn_pending_full_refresh = false;
+        self.display_churn_saw_screen_refresh = false;
         self.refresh_window_server_snapshot_after_churn();
         self.resync_windows_after_churn();
         self.force_refresh_all_windows();
@@ -764,10 +784,14 @@ impl Reactor {
             Event::DisplayChurnBegin => {
                 self.display_churn_active = true;
                 self.display_churn_pending_full_refresh = true;
+                self.display_churn_saw_screen_refresh = false;
                 return;
             }
             Event::DisplayChurnEnd => {
                 self.display_churn_active = false;
+                if self.display_churn_pending_full_refresh && self.display_churn_saw_screen_refresh {
+                    self.recover_from_display_churn();
+                }
                 return;
             }
             _ => {}
diff --git a/src/actor/reactor/replay.rs b/src/actor/reactor/replay.rs
index 4920e3d..4c96ad9 100644
--- a/src/actor/reactor/replay.rs
+++ b/src/actor/reactor/replay.rs
@@ -5,7 +5,7 @@ use std::path::Path;
 
 #[cfg(test)]
 use tempfile::NamedTempFile;
-use tracing::Span;
+use tracing::{debug, Span};
 
 use super::{Event, Reactor};
 use crate::actor::app::{AppThreadHandle, Request};
@@ -60,8 +60,14 @@ impl Record {
 
     pub(super) fn on_event(&mut self, event: &Event) {
         let Some(file) = self.file() else { return };
-        let line = ron::ser::to_string(&event).unwrap();
-        write!(file, "{line}\n").unwrap();
+        match ron::ser::to_string(&event) {
+            Ok(line) => {
+                write!(file, "{line}\n").unwrap();
+            }
+            Err(err) => {
+                debug!(?event, ?err, "Skipping non-serializable event in recorder");
+            }
+        }
     }
 }
 

cc @brianwk lmk if this works