From dffb4191456553a686364ce0d6fc33994a9281f7 Mon Sep 17 00:00:00 2001 From: Jason Lee Date: Tue, 4 Feb 2025 15:43:26 +0800 Subject: [PATCH] chore: Fix subscriptions leak. (#597) --- crates/story/src/title_bar.rs | 10 ++++--- crates/ui/src/color_picker.rs | 9 +++--- crates/ui/src/dropdown.rs | 12 +++++--- crates/ui/src/input/input.rs | 48 +++++++++++++++---------------- crates/ui/src/input/otp_input.rs | 47 +++++++++++++++--------------- crates/ui/src/list/list.rs | 12 ++++---- crates/ui/src/notification.rs | 26 ++++++++++++----- crates/ui/src/popup_menu.rs | 15 ++++++---- crates/ui/src/time/date_picker.rs | 9 +++--- 9 files changed, 107 insertions(+), 81 deletions(-) diff --git a/crates/story/src/title_bar.rs b/crates/story/src/title_bar.rs index 637132f9..5796c2cd 100644 --- a/crates/story/src/title_bar.rs +++ b/crates/story/src/title_bar.rs @@ -3,7 +3,7 @@ use std::rc::Rc; use gpui::{ div, px, AnyElement, App, AppContext, ClickEvent, Context, Corner, Entity, FocusHandle, Hsla, InteractiveElement as _, IntoElement, MouseButton, ParentElement as _, Render, SharedString, - Styled as _, Window, + Styled as _, Subscription, Window, }; use ui::{ badge::Badge, @@ -24,6 +24,7 @@ pub struct AppTitleBar { font_size_selector: Entity, theme_color_picker: Entity, child: Rc AnyElement>, + _subscriptions: Vec, } impl AppTitleBar { @@ -43,7 +44,8 @@ impl AppTitleBar { picker.set_value(cx.theme().primary, window, cx); picker }); - cx.subscribe_in( + + let _subscriptions = vec![cx.subscribe_in( &theme_color_picker, window, |this, _, ev: &ColorPickerEvent, window, cx| match ev { @@ -51,8 +53,7 @@ impl AppTitleBar { this.set_theme_color(*color, window, cx); } }, - ) - .detach(); + )]; Self { title: title.into(), @@ -61,6 +62,7 @@ impl AppTitleBar { font_size_selector, theme_color_picker, child: Rc::new(|_, _| div().into_any_element()), + _subscriptions, } } diff --git a/crates/ui/src/color_picker.rs b/crates/ui/src/color_picker.rs index 6fb3a369..649ee1fa 100644 --- a/crates/ui/src/color_picker.rs +++ b/crates/ui/src/color_picker.rs @@ -2,7 +2,7 @@ use gpui::{ anchored, canvas, deferred, div, prelude::FluentBuilder as _, px, relative, App, AppContext, Bounds, Context, Corner, ElementId, Entity, EventEmitter, FocusHandle, Focusable, Hsla, InteractiveElement as _, IntoElement, KeyBinding, MouseButton, ParentElement, Pixels, Point, - Render, SharedString, StatefulInteractiveElement as _, Styled, Window, + Render, SharedString, StatefulInteractiveElement as _, Styled, Subscription, Window, }; use crate::{ @@ -66,13 +66,14 @@ pub struct ColorPicker { open: bool, bounds: Bounds, + _subscriptions: Vec, } impl ColorPicker { pub fn new(id: impl Into, window: &mut Window, cx: &mut Context) -> Self { let color_input = cx.new(|cx| TextInput::new(window, cx).xsmall()); - cx.subscribe_in( + let _subscriptions = vec![cx.subscribe_in( &color_input, window, |this, _, ev: &InputEvent, window, cx| match ev { @@ -91,8 +92,7 @@ impl ColorPicker { } _ => {} }, - ) - .detach(); + )]; Self { id: id.into(), @@ -118,6 +118,7 @@ impl ColorPicker { color_input, open: false, bounds: Bounds::default(), + _subscriptions, } } diff --git a/crates/ui/src/dropdown.rs b/crates/ui/src/dropdown.rs index d900a06a..d63be262 100644 --- a/crates/ui/src/dropdown.rs +++ b/crates/ui/src/dropdown.rs @@ -2,7 +2,8 @@ use gpui::{ actions, anchored, canvas, deferred, div, prelude::FluentBuilder, px, rems, AnyElement, App, AppContext, Bounds, ClickEvent, Context, DismissEvent, ElementId, Entity, EventEmitter, FocusHandle, Focusable, InteractiveElement, IntoElement, KeyBinding, Length, ParentElement, - Pixels, Render, SharedString, StatefulInteractiveElement, Styled, Task, WeakEntity, Window, + Pixels, Render, SharedString, StatefulInteractiveElement, Styled, Subscription, Task, + WeakEntity, Window, }; use rust_i18n::t; @@ -252,6 +253,7 @@ pub struct Dropdown { /// Store the bounds of the input bounds: Bounds, disabled: bool, + _subscriptions: Vec, } pub struct SearchableVec { @@ -352,9 +354,10 @@ where list }); - cx.on_blur(&list.focus_handle(cx), window, Self::on_blur) - .detach(); - cx.on_blur(&focus_handle, window, Self::on_blur).detach(); + let _subscriptions = vec![ + cx.on_blur(&list.focus_handle(cx), window, Self::on_blur), + cx.on_blur(&focus_handle, window, Self::on_blur), + ]; let mut this = Self { id: id.into(), @@ -372,6 +375,7 @@ where menu_width: Length::Auto, bounds: Bounds::default(), disabled: false, + _subscriptions, }; this.set_selected_index(selected_index, window, cx); this diff --git a/crates/ui/src/input/input.rs b/crates/ui/src/input/input.rs index e4fc7cb4..18a56616 100644 --- a/crates/ui/src/input/input.rs +++ b/crates/ui/src/input/input.rs @@ -15,7 +15,7 @@ use gpui::{ Context, Entity, EntityInputHandler, EventEmitter, FocusHandle, Focusable, InteractiveElement as _, IntoElement, KeyBinding, KeyDownEvent, MouseButton, MouseDownEvent, MouseMoveEvent, MouseUpEvent, ParentElement as _, Pixels, Point, Rems, Render, ScrollHandle, - ScrollWheelEvent, SharedString, Styled as _, UTF16Selection, Window, WrappedLine, + ScrollWheelEvent, SharedString, Styled as _, Subscription, UTF16Selection, Window, WrappedLine, }; // TODO: @@ -230,6 +230,7 @@ pub struct TextInput { pub(crate) scroll_size: gpui::Size, /// To remember the horizontal column (x-coordinate) of the cursor position. preferred_x_offset: Option, + _subscriptions: Vec, } impl EventEmitter for TextInput {} @@ -239,7 +240,26 @@ impl TextInput { let focus_handle = cx.focus_handle(); let blink_cursor = cx.new(|_| BlinkCursor::new()); let history = History::new().group_interval(std::time::Duration::from_secs(1)); - let input = Self { + + let _subscriptions = vec![ + // Observe the blink cursor to repaint the view when it changes. + cx.observe(&blink_cursor, |_, _, cx| cx.notify()), + // Blink the cursor when the window is active, pause when it's not. + cx.observe_window_activation(window, |input, window, cx| { + if window.is_window_active() { + let focus_handle = input.focus_handle.clone(); + if focus_handle.is_focused(window) { + input.blink_cursor.update(cx, |blink_cursor, cx| { + blink_cursor.start(cx); + }); + } + } + }), + cx.on_focus(&focus_handle, window, Self::on_focus), + cx.on_blur(&focus_handle, window, Self::on_blur), + ]; + + Self { focus_handle: focus_handle.clone(), text: "".into(), multi_line: false, @@ -272,28 +292,8 @@ impl TextInput { scrollbar_state: Rc::new(Cell::new(ScrollbarState::default())), scroll_size: gpui::size(px(0.), px(0.)), preferred_x_offset: None, - }; - - // Observe the blink cursor to repaint the view when it changes. - cx.observe(&input.blink_cursor, |_, _, cx| cx.notify()) - .detach(); - // Blink the cursor when the window is active, pause when it's not. - cx.observe_window_activation(window, |input, window, cx| { - if window.is_window_active() { - let focus_handle = input.focus_handle.clone(); - if focus_handle.is_focused(window) { - input.blink_cursor.update(cx, |blink_cursor, cx| { - blink_cursor.start(cx); - }); - } - } - }) - .detach(); - - cx.on_focus(&focus_handle, window, Self::on_focus).detach(); - cx.on_blur(&focus_handle, window, Self::on_blur).detach(); - - input + _subscriptions, + } } /// Use the text input field as a multi-line Textarea. diff --git a/crates/ui/src/input/otp_input.rs b/crates/ui/src/input/otp_input.rs index a09c44f9..ee8ab93c 100644 --- a/crates/ui/src/input/otp_input.rs +++ b/crates/ui/src/input/otp_input.rs @@ -1,7 +1,7 @@ use gpui::{ div, prelude::FluentBuilder, px, AnyElement, AppContext as _, Context, Entity, EventEmitter, FocusHandle, Focusable, InteractiveElement, IntoElement, KeyDownEvent, MouseButton, - MouseDownEvent, ParentElement as _, Render, SharedString, Styled as _, Window, + MouseDownEvent, ParentElement as _, Render, SharedString, Styled as _, Subscription, Window, }; use crate::{h_flex, v_flex, ActiveTheme, Icon, IconName, Sizable, Size}; @@ -29,13 +29,33 @@ pub struct OtpInput { value: SharedString, blink_cursor: Entity, size: Size, + _subscriptions: Vec, } impl OtpInput { pub fn new(length: usize, window: &mut Window, cx: &mut Context) -> Self { let focus_handle = cx.focus_handle(); let blink_cursor = cx.new(|_| BlinkCursor::new()); - let input = Self { + + let _subscriptions = vec![ + // Observe the blink cursor to repaint the view when it changes. + cx.observe(&blink_cursor, |_, _, cx| cx.notify()), + // Blink the cursor when the window is active, pause when it's not. + cx.observe_window_activation(window, |this, window, cx| { + if window.is_window_active() { + let focus_handle = this.focus_handle.clone(); + if focus_handle.is_focused(window) { + this.blink_cursor.update(cx, |blink_cursor, cx| { + blink_cursor.start(cx); + }); + } + } + }), + cx.on_focus(&focus_handle, window, Self::on_focus), + cx.on_blur(&focus_handle, window, Self::on_blur), + ]; + + Self { focus_handle: focus_handle.clone(), length, number_of_groups: 2, @@ -43,27 +63,8 @@ impl OtpInput { masked: false, blink_cursor: blink_cursor.clone(), size: Size::Medium, - }; - - // Observe the blink cursor to repaint the view when it changes. - cx.observe(&blink_cursor, |_, _, cx| cx.notify()).detach(); - // Blink the cursor when the window is active, pause when it's not. - cx.observe_window_activation(window, |this, window, cx| { - if window.is_window_active() { - let focus_handle = this.focus_handle.clone(); - if focus_handle.is_focused(window) { - this.blink_cursor.update(cx, |blink_cursor, cx| { - blink_cursor.start(cx); - }); - } - } - }) - .detach(); - - cx.on_focus(&focus_handle, window, Self::on_focus).detach(); - cx.on_blur(&focus_handle, window, Self::on_blur).detach(); - - input + _subscriptions, + } } /// Set number of groups in the OTP Input. diff --git a/crates/ui/src/list/list.rs b/crates/ui/src/list/list.rs index 60883384..814b826e 100644 --- a/crates/ui/src/list/list.rs +++ b/crates/ui/src/list/list.rs @@ -13,7 +13,7 @@ use gpui::{ ListSizingBehavior, MouseButton, ParentElement, Render, SharedString, Styled, Task, UniformListScrollHandle, Window, }; -use gpui::{px, App, Context, EventEmitter, ScrollStrategy}; +use gpui::{px, App, Context, EventEmitter, ScrollStrategy, Subscription}; use smol::Timer; use super::loading::Loading; @@ -158,6 +158,7 @@ pub struct List { right_clicked_index: Option, _search_task: Task<()>, _load_more_task: Task<()>, + _query_input_subscription: Subscription, } impl List @@ -173,8 +174,8 @@ where .cleanable() }); - cx.subscribe_in(&query_input, window, Self::on_query_input_event) - .detach(); + let _query_input_subscription = + cx.subscribe_in(&query_input, window, Self::on_query_input_event); Self { focus_handle: cx.focus_handle(), @@ -192,6 +193,7 @@ where size: Size::default(), _search_task: Task::ready(()), _load_more_task: Task::ready(()), + _query_input_subscription, } } @@ -233,8 +235,8 @@ where window: &mut Window, cx: &mut Context, ) { - cx.subscribe_in(&query_input, window, Self::on_query_input_event) - .detach(); + self._query_input_subscription = + cx.subscribe_in(&query_input, window, Self::on_query_input_event); self.query_input = Some(query_input); } diff --git a/crates/ui/src/notification.rs b/crates/ui/src/notification.rs index c82cef1c..670f9f9a 100644 --- a/crates/ui/src/notification.rs +++ b/crates/ui/src/notification.rs @@ -1,9 +1,15 @@ -use std::{any::TypeId, collections::VecDeque, sync::Arc, time::Duration}; +use std::{ + any::TypeId, + collections::{HashMap, VecDeque}, + sync::Arc, + time::Duration, +}; use gpui::{ div, prelude::FluentBuilder, px, Animation, AnimationExt, App, AppContext, ClickEvent, Context, DismissEvent, ElementId, Entity, EventEmitter, InteractiveElement as _, IntoElement, - ParentElement as _, Render, SharedString, StatefulInteractiveElement, Styled, Window, + ParentElement as _, Render, SharedString, StatefulInteractiveElement, Styled, Subscription, + Window, }; use smol::Timer; @@ -20,7 +26,7 @@ pub enum NotificationType { Error, } -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, PartialEq, Clone, Hash, Eq)] pub(crate) enum NotificationId { Id(TypeId), IdAndElementId(TypeId, ElementId), @@ -286,6 +292,7 @@ pub struct NotificationList { /// Notifications that will be auto hidden. pub(crate) notifications: VecDeque>, expanded: bool, + _subscriptions: HashMap, } impl NotificationList { @@ -293,6 +300,7 @@ impl NotificationList { Self { notifications: VecDeque::new(), expanded: false, + _subscriptions: HashMap::new(), } } @@ -310,10 +318,14 @@ impl NotificationList { self.notifications.retain(|note| note.read(cx).id != id); let notification = cx.new(|_| notification); - cx.subscribe(¬ification, move |view, _, _: &DismissEvent, cx| { - view.notifications.retain(|note| id != note.read(cx).id); - }) - .detach(); + + self._subscriptions.insert( + id.clone(), + cx.subscribe(¬ification, move |view, _, _: &DismissEvent, cx| { + view.notifications.retain(|note| id != note.read(cx).id); + view._subscriptions.remove(&id); + }), + ); self.notifications.push_back(notification.clone()); if autohide { diff --git a/crates/ui/src/popup_menu.rs b/crates/ui/src/popup_menu.rs index 7dd5b5f7..4f4260ae 100644 --- a/crates/ui/src/popup_menu.rs +++ b/crates/ui/src/popup_menu.rs @@ -4,6 +4,7 @@ use crate::{ button::Button, h_flex, list::ListItem, popover::Popover, v_flex, ActiveTheme, Icon, IconName, Selectable, Sizable as _, }; +use gpui::Subscription; use gpui::{ actions, anchored, canvas, div, prelude::FluentBuilder, px, rems, Action, AnyElement, App, AppContext, Bounds, Context, Corner, DismissEvent, Edges, Entity, EventEmitter, FocusHandle, @@ -107,7 +108,7 @@ pub struct PopupMenu { scroll_state: Rc>, action_focus_handle: Option, - _subscriptions: [gpui::Subscription; 1], + _subscriptions: Vec, } impl PopupMenu { @@ -118,10 +119,12 @@ impl PopupMenu { ) -> Entity { cx.new(|cx| { let focus_handle = cx.focus_handle(); - let _on_blur_subscription = - cx.on_blur(&focus_handle, window, |this: &mut PopupMenu, window, cx| { - this.dismiss(&Dismiss, window, cx) - }); + let _subscriptions = + vec![ + cx.on_blur(&focus_handle, window, |this: &mut PopupMenu, window, cx| { + this.dismiss(&Dismiss, window, cx) + }), + ]; let menu = Self { focus_handle, @@ -138,7 +141,7 @@ impl PopupMenu { scrollable: false, scroll_handle: ScrollHandle::default(), scroll_state: Rc::new(Cell::new(ScrollbarState::default())), - _subscriptions: [_on_blur_subscription], + _subscriptions, }; window.refresh(); f(menu, window, cx) diff --git a/crates/ui/src/time/date_picker.rs b/crates/ui/src/time/date_picker.rs index 52fb931d..2be1fa7f 100644 --- a/crates/ui/src/time/date_picker.rs +++ b/crates/ui/src/time/date_picker.rs @@ -3,7 +3,7 @@ use gpui::{ anchored, deferred, div, prelude::FluentBuilder as _, px, App, AppContext, Context, ElementId, Entity, EventEmitter, FocusHandle, Focusable, InteractiveElement as _, KeyBinding, Length, MouseButton, ParentElement as _, Render, SharedString, StatefulInteractiveElement as _, Styled, - Window, + Subscription, Window, }; use rust_i18n::t; @@ -68,6 +68,7 @@ pub struct DatePicker { calendar: Entity, number_of_months: usize, presets: Option>, + _subscriptions: Vec, } impl DatePicker { @@ -103,7 +104,7 @@ impl DatePicker { this }); - cx.subscribe_in( + let _subscriptions = vec![cx.subscribe_in( &calendar, window, |this, _, ev: &CalendarEvent, window, cx| match ev { @@ -112,8 +113,7 @@ impl DatePicker { this.focus_handle.focus(window); } }, - ) - .detach(); + )]; Self { id: id.into(), @@ -128,6 +128,7 @@ impl DatePicker { number_of_months: 1, placeholder: None, presets: None, + _subscriptions, } }