From fc9687d16334ac33f482630f9a9330bc24bd8edb Mon Sep 17 00:00:00 2001 From: Julia Date: Thu, 27 Jul 2023 18:27:36 -0400 Subject: [PATCH] Avoid panic by accessing view handle by global in wrong window 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 --- 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(-) diff --git a/crates/vim/src/mode_indicator.rs b/crates/vim/src/mode_indicator.rs index e0d2b65955..84e3e5868a 100644 --- a/crates/vim/src/mode_indicator.rs +++ b/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, + _subscription: Subscription, } impl ModeIndicator { - pub fn new(mode: Mode) -> Self { - Self { mode } + pub fn new(cx: &mut ViewContext) -> Self { + let handle = cx.handle().downgrade(); + + let _subscription = cx.subscribe_global::(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::(move |mode_indicator, cx| { + if settings::get::(cx).0 { + mode_indicator.mode = cx + .has_global::() + .then(|| cx.global::().state.mode); + } else { + mode_indicator.mode.take(); + } + }) + .detach(); + + // Vim doesn't exist in some tests + let mode = cx + .has_global::() + .then(|| cx.global::().state.mode); + + Self { + mode, + _subscription, + } } pub fn set_mode(&mut self, mode: Mode, cx: &mut ViewContext) { - 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) -> AnyElement { + 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 --", diff --git a/crates/vim/src/test.rs b/crates/vim/src/test.rs index 96d6a2b690..98d8cb8749 100644 --- a/crates/vim/src/test.rs +++ b/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::(); - assert!(mode_indicator.is_none()); + let mode_indicator = status_bar.item_of_type::().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::(); - assert!(mode_indicator.is_some()); + let mode_indicator = status_bar.item_of_type::().unwrap(); + assert!(mode_indicator.read(cx).mode.is_some()); }); } diff --git a/crates/vim/src/test/vim_test_context.rs b/crates/vim/src/test/vim_test_context.rs index 56ca654644..ea09e55091 100644 --- a/crates/vim/src/test/vim_test_context.rs +++ b/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 } diff --git a/crates/vim/src/vim.rs b/crates/vim/src/vim.rs index d8edf1a667..22bd196c67 100644 --- a/crates/vim/src/vim.rs +++ b/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::(cx); @@ -121,8 +126,6 @@ pub fn observe_keystrokes(cx: &mut WindowContext) { pub struct Vim { active_editor: Option>, editor_subscription: Option, - mode_indicator: Option>, - 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::() - .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::(); - - 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::(); - 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) { diff --git a/crates/zed/src/zed.rs b/crates/zed/src/zed.rs index 84cef99f81..a29567ac38 100644 --- a/crates/zed/src/zed.rs +++ b/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); });