diff --git a/crates/gpui/src/subscription.rs b/crates/gpui/src/subscription.rs index cf44b68d2bcbf7ca7d02c4b9e956f15079f8bdb6..b0c55a3966192ea0a15011e3cd2d14498c680c46 100644 --- a/crates/gpui/src/subscription.rs +++ b/crates/gpui/src/subscription.rs @@ -1,9 +1,8 @@ -use collections::{BTreeMap, BTreeSet}; +use collections::BTreeMap; use gpui_util::post_inc; use std::{ cell::{Cell, RefCell}, fmt::Debug, - mem, rc::Rc, }; @@ -19,12 +18,12 @@ impl Clone for SubscriberSet { struct SubscriberSetState { subscribers: BTreeMap>>>, - dropped_subscribers: BTreeSet<(EmitterKey, usize)>, next_subscriber_id: usize, } struct Subscriber { active: Rc>, + dropped: Rc>, callback: Callback, } @@ -36,7 +35,6 @@ where pub fn new() -> Self { Self(Rc::new(RefCell::new(SubscriberSetState { subscribers: Default::default(), - dropped_subscribers: Default::default(), next_subscriber_id: 0, }))) } @@ -51,6 +49,7 @@ where callback: Callback, ) -> (Subscription, impl FnOnce() + use) { let active = Rc::new(Cell::new(false)); + let dropped = Rc::new(Cell::new(false)); let mut lock = self.0.borrow_mut(); let subscriber_id = post_inc(&mut lock.next_subscriber_id); lock.subscribers @@ -61,6 +60,7 @@ where subscriber_id, Subscriber { active: active.clone(), + dropped: dropped.clone(), callback, }, ); @@ -68,9 +68,10 @@ where let subscription = Subscription { unsubscribe: Some(Box::new(move || { + dropped.set(true); + let mut lock = this.borrow_mut(); let Some(subscribers) = lock.subscribers.get_mut(&emitter_key) else { - // remove was called with this emitter_key return; }; @@ -79,14 +80,7 @@ where if subscribers.is_empty() { lock.subscribers.remove(&emitter_key); } - return; } - - // We didn't manage to remove the subscription, which means it was dropped - // while invoking the callback. Mark it as dropped so that we can remove it - // later. - lock.dropped_subscribers - .insert((emitter_key, subscriber_id)); })), }; (subscription, move || active.set(true)) @@ -128,11 +122,14 @@ where }; subscribers.retain(|_, subscriber| { - if subscriber.active.get() { - f(&mut subscriber.callback) - } else { - true + if !subscriber.active.get() { + return true; } + if subscriber.dropped.get() { + return false; + } + let keep = f(&mut subscriber.callback); + keep && !subscriber.dropped.get() }); let mut lock = self.0.borrow_mut(); @@ -141,12 +138,6 @@ where subscribers.extend(new_subscribers); } - // Remove any dropped subscriptions that were dropped while invoking the callback. - for (dropped_emitter, dropped_subscription_id) in mem::take(&mut lock.dropped_subscribers) { - debug_assert_eq!(*emitter, dropped_emitter); - subscribers.remove(&dropped_subscription_id); - } - if !subscribers.is_empty() { lock.subscribers.insert(emitter.clone(), Some(subscribers)); } @@ -207,3 +198,154 @@ impl std::fmt::Debug for Subscription { f.debug_struct("Subscription").finish() } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::{Global, TestApp}; + + #[test] + fn test_unsubscribe_during_callback_with_insert() { + struct TestGlobal; + impl Global for TestGlobal {} + + let mut app = TestApp::new(); + app.set_global(TestGlobal); + + let observer_a_count = Rc::new(Cell::new(0usize)); + let observer_b_count = Rc::new(Cell::new(0usize)); + + let sub_a: Rc>> = Default::default(); + let sub_b: Rc>> = Default::default(); + + // Observer A fires first (lower subscriber_id). It drops itself and + // inserts a new observer for the same global. + *sub_a.borrow_mut() = Some(app.update({ + let count = observer_a_count.clone(); + let sub_a = sub_a.clone(); + move |cx| { + cx.observe_global::(move |cx| { + count.set(count.get() + 1); + sub_a.borrow_mut().take(); + cx.observe_global::(|_| {}).detach(); + }) + } + })); + + // Observer B fires second. It just drops itself. + *sub_b.borrow_mut() = Some(app.update({ + let count = observer_b_count.clone(); + let sub_b = sub_b.clone(); + move |cx| { + cx.observe_global::(move |_cx| { + count.set(count.get() + 1); + sub_b.borrow_mut().take(); + }) + } + })); + + // Both fire once. + app.update(|cx| cx.set_global(TestGlobal)); + assert_eq!(observer_a_count.get(), 1); + assert_eq!(observer_b_count.get(), 1); + + // Neither should fire again — both dropped their subscriptions. + app.update(|cx| cx.set_global(TestGlobal)); + assert_eq!(observer_a_count.get(), 1); + assert_eq!(observer_b_count.get(), 1, "orphaned subscriber fired again"); + } + + #[test] + fn test_callback_dropped_by_earlier_callback_does_not_fire() { + struct TestGlobal; + impl Global for TestGlobal {} + + let mut app = TestApp::new(); + app.set_global(TestGlobal); + + let observer_b_count = Rc::new(Cell::new(0usize)); + let sub_b: Rc>> = Default::default(); + + // Observer A fires first and drops B's subscription. + app.update({ + let sub_b = sub_b.clone(); + move |cx| { + cx.observe_global::(move |_cx| { + sub_b.borrow_mut().take(); + }) + .detach(); + } + }); + + // Observer B fires second — but A already dropped it. + *sub_b.borrow_mut() = Some(app.update({ + let count = observer_b_count.clone(); + move |cx| { + cx.observe_global::(move |_cx| { + count.set(count.get() + 1); + }) + } + })); + + app.update(|cx| cx.set_global(TestGlobal)); + assert_eq!( + observer_b_count.get(), + 0, + "B should not fire — A dropped its subscription" + ); + } + + #[test] + fn test_self_drop_during_callback() { + struct TestGlobal; + impl Global for TestGlobal {} + + let mut app = TestApp::new(); + app.set_global(TestGlobal); + + let count = Rc::new(Cell::new(0usize)); + let sub: Rc>> = Default::default(); + + *sub.borrow_mut() = Some(app.update({ + let count = count.clone(); + let sub = sub.clone(); + move |cx| { + cx.observe_global::(move |_cx| { + count.set(count.get() + 1); + sub.borrow_mut().take(); + }) + } + })); + + app.update(|cx| cx.set_global(TestGlobal)); + assert_eq!(count.get(), 1); + + app.update(|cx| cx.set_global(TestGlobal)); + assert_eq!(count.get(), 1, "should not fire after self-drop"); + } + + #[test] + fn test_subscription_drop() { + struct TestGlobal; + impl Global for TestGlobal {} + + let mut app = TestApp::new(); + app.set_global(TestGlobal); + + let count = Rc::new(Cell::new(0usize)); + + let subscription = app.update({ + let count = count.clone(); + move |cx| { + cx.observe_global::(move |_cx| { + count.set(count.get() + 1); + }) + } + }); + + drop(subscription); + + app.update(|cx| cx.set_global(TestGlobal)); + assert_eq!(count.get(), 0, "should not fire after drop"); + } +} diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index a111517c21fa3bf401c8c94538253ed5e9a16528..52c84b82a46b58830a8fe936a2a6bbb09f895588 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -14542,4 +14542,72 @@ mod tests { assert!(panel.is_zoomed(window, cx)); }); } + + #[gpui::test] + async fn test_panels_stay_open_after_position_change_and_settings_update( + cx: &mut gpui::TestAppContext, + ) { + init_test(cx); + let fs = FakeFs::new(cx.executor()); + let project = Project::test(fs, [], cx).await; + let (workspace, cx) = + cx.add_window_view(|window, cx| Workspace::test_new(project, window, cx)); + + // Add two panels to the left dock and open it. + let (panel_a, panel_b) = workspace.update_in(cx, |workspace, window, cx| { + let panel_a = cx.new(|cx| TestPanel::new(DockPosition::Left, 100, cx)); + let panel_b = cx.new(|cx| TestPanel::new(DockPosition::Left, 101, cx)); + workspace.add_panel(panel_a.clone(), window, cx); + workspace.add_panel(panel_b.clone(), window, cx); + workspace.left_dock().update(cx, |dock, cx| { + dock.set_open(true, window, cx); + dock.activate_panel(0, window, cx); + }); + (panel_a, panel_b) + }); + + workspace.update_in(cx, |workspace, _, cx| { + assert!(workspace.left_dock().read(cx).is_open()); + }); + + // Simulate a feature flag changing default dock positions: both panels + // move from Left to Right. + workspace.update_in(cx, |_workspace, _window, cx| { + panel_a.update(cx, |p, _cx| p.position = DockPosition::Right); + panel_b.update(cx, |p, _cx| p.position = DockPosition::Right); + cx.update_global::(|_, _| {}); + }); + + // Both panels should now be in the right dock. + workspace.update_in(cx, |workspace, _, cx| { + let right_dock = workspace.right_dock().read(cx); + assert_eq!(right_dock.panels_len(), 2); + }); + + // Open the right dock and activate panel_b (simulating the user + // opening the panel after it moved). + workspace.update_in(cx, |workspace, window, cx| { + workspace.right_dock().update(cx, |dock, cx| { + dock.set_open(true, window, cx); + dock.activate_panel(1, window, cx); + }); + }); + + // Now trigger another SettingsStore change + workspace.update_in(cx, |_workspace, _window, cx| { + cx.update_global::(|_, _| {}); + }); + + workspace.update_in(cx, |workspace, _, cx| { + assert!( + workspace.right_dock().read(cx).is_open(), + "Right dock should still be open after a settings change" + ); + assert_eq!( + workspace.right_dock().read(cx).panels_len(), + 2, + "Both panels should still be in the right dock" + ); + }); + } }