mirror of
https://github.com/zed-industries/zed.git
synced 2026-06-01 03:14:56 +07:00
Cherry-pick of #56075 to preview ---- `render_settings_item_link` was calling `cx.read_from_clipboard()` during render so it could show a check icon next to the copy-link button when the matching link was on the clipboard. This had two problems: - A clipboard read per visible setting per frame is too expensive. - On Windows, reading the clipboard pumps the system message queue. If a queued message handler updates `App` while we're still rendering, GPUI panics with `RefCell already borrowed` (many occurrences observed). Track the `json_path` of the most recently copied setting locally instead. The check icon now reflects what was copied in this session via this UI rather than whatever is on the system clipboard. While this removes the most common offender, the underlying `gpui_windows` reentrancy bug still exists: `on_close` / `on_request_frame` callbacks can be invoked while `App` is already borrowed on Windows, and can be triggered by any other clipboard-touching code path. We should consider a follow-up PR that handles this at the platform layer -- either by deferring callbacks that re-borrow `App`, or by guarding individual handlers in `gpui_windows::events` against reentrant `borrow_mut` calls. Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [ ] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - Fixed a crash on Windows that could occur when closing the settings window - Improved the overall performance of the settings window Co-authored-by: Agus Zubiaga <agus@zed.dev>
This commit is contained in:
parent
074ed276a2
commit
6c6d780e47
1 changed files with 12 additions and 8 deletions
|
|
@ -763,6 +763,7 @@ pub struct SettingsWindow {
|
|||
list_state: ListState,
|
||||
shown_errors: HashSet<String>,
|
||||
pub(crate) regex_validation_error: Option<String>,
|
||||
last_copied_link_path: Option<&'static str>,
|
||||
}
|
||||
|
||||
struct SearchDocument {
|
||||
|
|
@ -1035,6 +1036,7 @@ impl SettingsPageItem {
|
|||
sub_page_link.title.clone(),
|
||||
sub_page_link.json_path,
|
||||
false,
|
||||
settings_window,
|
||||
cx,
|
||||
)),
|
||||
)
|
||||
|
|
@ -1228,6 +1230,7 @@ fn render_settings_item(
|
|||
setting_item.description,
|
||||
setting_item.field.json_path(),
|
||||
sub_field,
|
||||
settings_window,
|
||||
cx,
|
||||
))
|
||||
})
|
||||
|
|
@ -1237,16 +1240,13 @@ fn render_settings_item_link(
|
|||
id: impl Into<ElementId>,
|
||||
json_path: Option<&'static str>,
|
||||
sub_field: bool,
|
||||
settings_window: &SettingsWindow,
|
||||
cx: &mut Context<'_, SettingsWindow>,
|
||||
) -> impl IntoElement {
|
||||
let clipboard_has_link = cx
|
||||
.read_from_clipboard()
|
||||
.and_then(|entry| entry.text())
|
||||
.map_or(false, |maybe_url| {
|
||||
json_path.is_some() && maybe_url.strip_prefix("zed://settings/") == json_path
|
||||
});
|
||||
let copied_link_matches =
|
||||
json_path.is_some() && json_path == settings_window.last_copied_link_path;
|
||||
|
||||
let (link_icon, link_icon_color) = if clipboard_has_link {
|
||||
let (link_icon, link_icon_color) = if copied_link_matches {
|
||||
(IconName::Check, Color::Success)
|
||||
} else {
|
||||
(IconName::Link, Color::Muted)
|
||||
|
|
@ -1271,9 +1271,10 @@ fn render_settings_item_link(
|
|||
.shape(IconButtonShape::Square)
|
||||
.tooltip(Tooltip::text("Copy Link"))
|
||||
.when_some(json_path, |this, path| {
|
||||
this.on_click(cx.listener(move |_, _, _, cx| {
|
||||
this.on_click(cx.listener(move |this, _, _, cx| {
|
||||
let link = format!("zed://settings/{}", path);
|
||||
cx.write_to_clipboard(ClipboardItem::new_string(link));
|
||||
this.last_copied_link_path = Some(path);
|
||||
cx.notify();
|
||||
}))
|
||||
}),
|
||||
|
|
@ -1685,6 +1686,7 @@ impl SettingsWindow {
|
|||
shown_errors: HashSet::default(),
|
||||
regex_validation_error: None,
|
||||
list_state,
|
||||
last_copied_link_path: None,
|
||||
};
|
||||
|
||||
this.fetch_files(window, cx);
|
||||
|
|
@ -4472,6 +4474,7 @@ pub mod test {
|
|||
list_state: ListState::new(0, gpui::ListAlignment::Top, px(0.0)),
|
||||
shown_errors: HashSet::default(),
|
||||
regex_validation_error: None,
|
||||
last_copied_link_path: None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -4597,6 +4600,7 @@ pub mod test {
|
|||
list_state: ListState::new(0, gpui::ListAlignment::Top, px(0.0)),
|
||||
shown_errors: HashSet::default(),
|
||||
regex_validation_error: None,
|
||||
last_copied_link_path: None,
|
||||
};
|
||||
|
||||
settings_window.build_filter_table();
|
||||
|
|
|
|||
Loading…
Reference in a new issue