Avoid panic by accessing view handle by global in wrong window

Julia and Mikayla Maki created

View handles are window specific but this global will be doing things
in all windows, that would cause a panic when it attempted to update
a status bar mode indicator in a background window

Co-Authored-By: Mikayla Maki <mikayla@zed.dev>

Change summary

crates/vim/src/mode_indicator.rs        | 61 +++++++++++++++++++++++---
crates/vim/src/test.rs                  | 14 +++---
crates/vim/src/test/vim_test_context.rs |  4 +
crates/vim/src/vim.rs                   | 51 ++-------------------
crates/zed/src/zed.rs                   |  2 
5 files changed, 72 insertions(+), 60 deletions(-)

Detailed changes

crates/vim/src/mode_indicator.rs 🔗

@@ -1,20 +1,60 @@
-use gpui::{elements::Label, AnyElement, Element, Entity, View, ViewContext};
+use gpui::{
+    elements::{Empty, Label},
+    AnyElement, Element, Entity, Subscription, View, ViewContext,
+};
+use settings::SettingsStore;
 use workspace::{item::ItemHandle, StatusItemView};
 
-use crate::state::Mode;
+use crate::{state::Mode, Vim, VimEvent, VimModeSetting};
 
 pub struct ModeIndicator {
-    pub mode: Mode,
+    pub mode: Option<Mode>,
+    _subscription: Subscription,
 }
 
 impl ModeIndicator {
-    pub fn new(mode: Mode) -> Self {
-        Self { mode }
+    pub fn new(cx: &mut ViewContext<Self>) -> Self {
+        let handle = cx.handle().downgrade();
+
+        let _subscription = cx.subscribe_global::<VimEvent, _>(move |&event, cx| {
+            if let Some(mode_indicator) = handle.upgrade(cx) {
+                match event {
+                    VimEvent::ModeChanged { mode } => {
+                        cx.update_window(mode_indicator.window_id(), |cx| {
+                            mode_indicator.update(cx, move |mode_indicator, cx| {
+                                mode_indicator.set_mode(mode, cx);
+                            })
+                        });
+                    }
+                }
+            }
+        });
+
+        cx.observe_global::<SettingsStore, _>(move |mode_indicator, cx| {
+            if settings::get::<VimModeSetting>(cx).0 {
+                mode_indicator.mode = cx
+                    .has_global::<Vim>()
+                    .then(|| cx.global::<Vim>().state.mode);
+            } else {
+                mode_indicator.mode.take();
+            }
+        })
+        .detach();
+
+        // Vim doesn't exist in some tests
+        let mode = cx
+            .has_global::<Vim>()
+            .then(|| cx.global::<Vim>().state.mode);
+
+        Self {
+            mode,
+            _subscription,
+        }
     }
 
     pub fn set_mode(&mut self, mode: Mode, cx: &mut ViewContext<Self>) {
-        if mode != self.mode {
-            self.mode = mode;
+        if self.mode != Some(mode) {
+            self.mode = Some(mode);
             cx.notify();
         }
     }
@@ -30,11 +70,16 @@ impl View for ModeIndicator {
     }
 
     fn render(&mut self, cx: &mut ViewContext<Self>) -> AnyElement<Self> {
+        let Some(mode) = self.mode.as_ref() else {
+            return Empty::new().into_any();
+        };
+
         let theme = &theme::current(cx).workspace.status_bar;
+
         // we always choose text to be 12 monospace characters
         // so that as the mode indicator changes, the rest of the
         // UI stays still.
-        let text = match self.mode {
+        let text = match mode {
             Mode::Normal => "-- NORMAL --",
             Mode::Insert => "-- INSERT --",
             Mode::Visual { line: false } => "-- VISUAL --",

crates/vim/src/test.rs 🔗

@@ -215,7 +215,7 @@ async fn test_status_indicator(
 
     assert_eq!(
         cx.workspace(|_, cx| mode_indicator.read(cx).mode),
-        Mode::Normal
+        Some(Mode::Normal)
     );
 
     // shows the correct mode
@@ -223,7 +223,7 @@ async fn test_status_indicator(
     deterministic.run_until_parked();
     assert_eq!(
         cx.workspace(|_, cx| mode_indicator.read(cx).mode),
-        Mode::Insert
+        Some(Mode::Insert)
     );
 
     // shows even in search
@@ -231,7 +231,7 @@ async fn test_status_indicator(
     deterministic.run_until_parked();
     assert_eq!(
         cx.workspace(|_, cx| mode_indicator.read(cx).mode),
-        Mode::Visual { line: false }
+        Some(Mode::Visual { line: false })
     );
 
     // hides if vim mode is disabled
@@ -239,15 +239,15 @@ async fn test_status_indicator(
     deterministic.run_until_parked();
     cx.workspace(|workspace, cx| {
         let status_bar = workspace.status_bar().read(cx);
-        let mode_indicator = status_bar.item_of_type::<ModeIndicator>();
-        assert!(mode_indicator.is_none());
+        let mode_indicator = status_bar.item_of_type::<ModeIndicator>().unwrap();
+        assert!(mode_indicator.read(cx).mode.is_none());
     });
 
     cx.enable_vim();
     deterministic.run_until_parked();
     cx.workspace(|workspace, cx| {
         let status_bar = workspace.status_bar().read(cx);
-        let mode_indicator = status_bar.item_of_type::<ModeIndicator>();
-        assert!(mode_indicator.is_some());
+        let mode_indicator = status_bar.item_of_type::<ModeIndicator>().unwrap();
+        assert!(mode_indicator.read(cx).mode.is_some());
     });
 }

crates/vim/src/test/vim_test_context.rs 🔗

@@ -43,6 +43,10 @@ impl<'a> VimTestContext<'a> {
                     toolbar.add_item(project_search_bar, cx);
                 })
             });
+            workspace.status_bar().update(cx, |status_bar, cx| {
+                let vim_mode_indicator = cx.add_view(ModeIndicator::new);
+                status_bar.add_right_item(vim_mode_indicator, cx);
+            });
         });
 
         Self { cx }

crates/vim/src/vim.rs 🔗

@@ -43,6 +43,11 @@ struct Number(u8);
 actions!(vim, [Tab, Enter]);
 impl_actions!(vim, [Number, SwitchMode, PushOperator]);
 
+#[derive(Copy, Clone, Debug)]
+enum VimEvent {
+    ModeChanged { mode: Mode },
+}
+
 pub fn init(cx: &mut AppContext) {
     settings::register::<VimModeSetting>(cx);
 
@@ -121,8 +126,6 @@ pub fn observe_keystrokes(cx: &mut WindowContext) {
 pub struct Vim {
     active_editor: Option<WeakViewHandle<Editor>>,
     editor_subscription: Option<Subscription>,
-    mode_indicator: Option<ViewHandle<ModeIndicator>>,
-
     enabled: bool,
     state: VimState,
 }
@@ -181,9 +184,7 @@ impl Vim {
         self.state.mode = mode;
         self.state.operator_stack.clear();
 
-        if let Some(mode_indicator) = &self.mode_indicator {
-            mode_indicator.update(cx, |mode_indicator, cx| mode_indicator.set_mode(mode, cx))
-        }
+        cx.emit_global(VimEvent::ModeChanged { mode });
 
         // Sync editor settings like clip mode
         self.sync_vim_settings(cx);
@@ -271,44 +272,6 @@ impl Vim {
         }
     }
 
-    fn sync_mode_indicator(cx: &mut WindowContext) {
-        let Some(workspace) = cx.root_view()
-            .downcast_ref::<Workspace>()
-            .map(|workspace| workspace.downgrade()) else {
-                return;
-            };
-
-        cx.spawn(|mut cx| async move {
-            workspace.update(&mut cx, |workspace, cx| {
-                Vim::update(cx, |vim, cx| {
-                    workspace.status_bar().update(cx, |status_bar, cx| {
-                        let current_position = status_bar.position_of_item::<ModeIndicator>();
-
-                        if vim.enabled && current_position.is_none() {
-                            if vim.mode_indicator.is_none() {
-                                vim.mode_indicator =
-                                    Some(cx.add_view(|_| ModeIndicator::new(vim.state.mode)));
-                            };
-                            let mode_indicator = vim.mode_indicator.as_ref().unwrap();
-                            let position = status_bar
-                                .position_of_item::<language_selector::ActiveBufferLanguage>();
-                            if let Some(position) = position {
-                                status_bar.insert_item_after(position, mode_indicator.clone(), cx)
-                            } else {
-                                status_bar.add_left_item(mode_indicator.clone(), cx)
-                            }
-                        } else if !vim.enabled {
-                            if let Some(position) = current_position {
-                                status_bar.remove_item_at(position, cx)
-                            }
-                        }
-                    })
-                })
-            })
-        })
-        .detach_and_log_err(cx);
-    }
-
     fn set_enabled(&mut self, enabled: bool, cx: &mut AppContext) {
         if self.enabled != enabled {
             self.enabled = enabled;
@@ -359,8 +322,6 @@ impl Vim {
                 self.unhook_vim_settings(editor, cx);
             }
         });
-
-        Vim::sync_mode_indicator(cx);
     }
 
     fn unhook_vim_settings(&self, editor: &mut Editor, cx: &mut ViewContext<Editor>) {

crates/zed/src/zed.rs 🔗

@@ -308,6 +308,7 @@ pub fn initialize_workspace(
             );
             let active_buffer_language =
                 cx.add_view(|_| language_selector::ActiveBufferLanguage::new(workspace));
+            let vim_mode_indicator = cx.add_view(|cx| vim::ModeIndicator::new(cx));
             let feedback_button = cx.add_view(|_| {
                 feedback::deploy_feedback_button::DeployFeedbackButton::new(workspace)
             });
@@ -319,6 +320,7 @@ pub fn initialize_workspace(
                 status_bar.add_right_item(feedback_button, cx);
                 status_bar.add_right_item(copilot, cx);
                 status_bar.add_right_item(active_buffer_language, cx);
+                status_bar.add_right_item(vim_mode_indicator, cx);
                 status_bar.add_right_item(cursor_position, cx);
             });