Avoid panic by accessing view handle by global in wrong window (#2801)

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

Release Notes:

- N/A

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);
             });