oak-team/oak-gpui
3
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Switch from Arc/RwLock to Rc/RefCell for CodeContextMenu (#22035)

Browse Source 
`CodeContextMenu` is always accessed on one thread, so only `Rc`s and
`Rc<RefCell<_>>` are needed. There should be tiny performance benefits
from this. The main benefit of this is that when seeing code accessing a
`RwLock` it would be reasonable to wonder whether it will block. The
only potential downside is the potential for panics due to overlapping
borrows of the RefCells. I think this is an acceptable risk because most
errors of this nature will be local or will be caught by clippy via the
check for holding a RefCell reference over an `await`.

Release Notes:

- N/A
This commit is contained in:
Michael Sloan
2024-12-16 01:50:21 -07:00
committed by GitHub
parent 7b721efe2c
commit a94afbc062
12 changed files with 108 additions and 93 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/editor/src/code_context_menus.rs
+18 -17
View File
@@ -1,4 +1,5 @@
use std::{cell::Cell, cmp::Reverse, ops::Range, sync::Arc};
use std::cell::RefCell;
use std::{cell::Cell, cmp::Reverse, ops::Range, rc::Rc};
use fuzzy::{StringMatch, StringMatchCandidate};
use gpui::{
@@ -11,7 +12,6 @@ use language::{CodeLabel, Documentation};
use lsp::LanguageServerId;
use multi_buffer::{Anchor, ExcerptId};
use ordered_float::OrderedFloat;
use parking_lot::RwLock;
use project::{CodeAction, Completion, TaskSourceKind};
use task::ResolvedTask;
use ui::{
@@ -137,9 +137,9 @@ pub struct CompletionsMenu {
sort_completions: bool,
pub initial_position: Anchor,
pub buffer: Model<Buffer>,
pub completions: Arc<RwLock<Box<[Completion]>>>,
match_candidates: Arc<[StringMatchCandidate]>,
pub matches: Arc<[StringMatch]>,
pub completions: Rc<RefCell<Box<[Completion]>>>,
match_candidates: Rc<[StringMatchCandidate]>,
pub matches: Rc<[StringMatch]>,
pub selected_item: usize,
scroll_handle: UniformListScrollHandle,
resolve_completions: bool,
@@ -169,7 +169,7 @@ impl CompletionsMenu {
initial_position,
buffer,
show_completion_documentation,
completions: Arc::new(RwLock::new(completions)),
completions: RefCell::new(completions).into(),
match_candidates,
matches: Vec::new().into(),
selected_item: 0,
@@ -223,7 +223,7 @@ impl CompletionsMenu {
sort_completions,
initial_position: selection.start,
buffer,
completions: Arc::new(RwLock::new(completions)),
completions: RefCell::new(completions).into(),
match_candidates,
matches,
selected_item: 0,
@@ -329,13 +329,13 @@ impl CompletionsMenu {
workspace: Option<WeakView<Workspace>>,
cx: &mut ViewContext<Editor>,
) -> AnyElement {
let completions = self.completions.borrow_mut();
let show_completion_documentation = self.show_completion_documentation;
let widest_completion_ix = self
.matches
.iter()
.enumerate()
.max_by_key(|(_, mat)| {
let completions = self.completions.read();
let completion = &completions[mat.candidate_id];
let documentation = &completion.documentation;
@@ -350,14 +350,12 @@ impl CompletionsMenu {
})
.map(|(ix, _)| ix);
let completions = self.completions.clone();
let matches = self.matches.clone();
let selected_item = self.selected_item;
let style = style.clone();
let multiline_docs = if show_completion_documentation {
let mat = &self.matches[selected_item];
match &self.completions.read()[mat.candidate_id].documentation {
match &completions[mat.candidate_id].documentation {
Some(Documentation::MultiLinePlainText(text)) => {
Some(div().child(SharedString::from(text.clone())))
}
@@ -401,13 +399,16 @@ impl CompletionsMenu {
.occlude()
});
drop(completions);
let completions = self.completions.clone();
let matches = self.matches.clone();
let list = uniform_list(
cx.view().clone(),
"completions",
matches.len(),
move |_editor, range, cx| {
let start_ix = range.start;
let completions_guard = completions.read();
let completions_guard = completions.borrow_mut();
matches[range]
.iter()
@@ -548,7 +549,7 @@ impl CompletionsMenu {
}
}
let completions = self.completions.read();
let completions = self.completions.borrow_mut();
if self.sort_completions {
matches.sort_unstable_by_key(|mat| {
// We do want to strike a balance here between what the language server tells us
@@ -611,13 +612,13 @@ impl CompletionsMenu {
pub struct AvailableCodeAction {
pub excerpt_id: ExcerptId,
pub action: CodeAction,
pub provider: Arc<dyn CodeActionProvider>,
pub provider: Rc<dyn CodeActionProvider>,
}
#[derive(Clone)]
pub struct CodeActionContents {
pub tasks: Option<Arc<ResolvedTasks>>,
pub actions: Option<Arc<[AvailableCodeAction]>>,
pub tasks: Option<Rc<ResolvedTasks>>,
pub actions: Option<Rc<[AvailableCodeAction]>>,
}
impl CodeActionContents {
@@ -702,7 +703,7 @@ pub enum CodeActionsItem {
CodeAction {
excerpt_id: ExcerptId,
action: CodeAction,
provider: Arc<dyn CodeActionProvider>,
provider: Rc<dyn CodeActionProvider>,
},
}
crates/editor/src/editor.rs
+29 -33
View File
@@ -127,7 +127,6 @@ pub use multi_buffer::{
use multi_buffer::{
ExpandExcerptDirection, MultiBufferDiffHunk, MultiBufferPoint, MultiBufferRow, ToOffsetUtf16,
};
use parking_lot::RwLock;
use project::{
lsp_store::{FormatTarget, FormatTrigger, OpenLspBufferHandle},
project_settings::{GitGutterSetting, ProjectSettings},
@@ -606,7 +605,7 @@ pub struct Editor {
scrollbar_marker_state: ScrollbarMarkerState,
active_indent_guides_state: ActiveIndentGuidesState,
nav_history: Option<ItemNavHistory>,
context_menu: RwLock<Option<CodeContextMenu>>,
context_menu: RefCell<Option<CodeContextMenu>>,
mouse_context_menu: Option<MouseContextMenu>,
hunk_controls_menu_handle: PopoverMenuHandle<ui::ContextMenu>,
completion_tasks: Vec<(CompletionId, Task<Option<()>>)>,
@@ -614,7 +613,7 @@ pub struct Editor {
auto_signature_help: Option<bool>,
find_all_references_task_sources: Vec<Anchor>,
next_completion_id: CompletionId,
available_code_actions: Option<(Location, Arc<[AvailableCodeAction]>)>,
available_code_actions: Option<(Location, Rc<[AvailableCodeAction]>)>,
code_actions_task: Option<Task<Result<()>>>,
document_highlights_task: Option<Task<()>>,
linked_editing_range_task: Option<Task<Option<()>>>,
@@ -635,7 +634,7 @@ pub struct Editor {
gutter_hovered: bool,
hovered_link_state: Option<HoveredLinkState>,
inline_completion_provider: Option<RegisteredInlineCompletionProvider>,
code_action_providers: Vec<Arc<dyn CodeActionProvider>>,
code_action_providers: Vec<Rc<dyn CodeActionProvider>>,
active_inline_completion: Option<InlineCompletionState>,
// enable_inline_completions is a switch that Vim can use to disable
// inline completions based on its mode.
@@ -1192,7 +1191,7 @@ impl Editor {
let mut code_action_providers = Vec::new();
if let Some(project) = project.clone() {
get_unstaged_changes_for_buffers(&project, buffer.read(cx).all_buffers(), cx);
code_action_providers.push(Arc::new(project) as Arc<_>);
code_action_providers.push(Rc::new(project) as Rc<_>);
}
let mut this = Self {
@@ -1238,7 +1237,7 @@ impl Editor {
scrollbar_marker_state: ScrollbarMarkerState::default(),
active_indent_guides_state: ActiveIndentGuidesState::default(),
nav_history: None,
context_menu: RwLock::new(None),
context_menu: RefCell::new(None),
mouse_context_menu: None,
hunk_controls_menu_handle: PopoverMenuHandle::default(),
completion_tasks: Default::default(),
@@ -1384,7 +1383,7 @@ impl Editor {
key_context.add("renaming");
}
if self.context_menu_visible() {
match self.context_menu.read().as_ref() {
match self.context_menu.borrow().as_ref() {
Some(CodeContextMenu::Completions(_)) => {
key_context.add("menu");
key_context.add("showing_completions")
@@ -1886,10 +1885,9 @@ impl Editor {
if local {
let new_cursor_position = self.selections.newest_anchor().head();
let mut context_menu = self.context_menu.write();
let mut context_menu = self.context_menu.borrow_mut();
let completion_menu = match context_menu.as_ref() {
Some(CodeContextMenu::Completions(menu)) => Some(menu),
_ => {
*context_menu = None;
None
@@ -1913,7 +1911,7 @@ impl Editor {
.await;
this.update(&mut cx, |this, cx| {
let mut context_menu = this.context_menu.write();
let mut context_menu = this.context_menu.borrow_mut();
let Some(CodeContextMenu::Completions(menu)) = context_menu.as_ref()
else {
return;
@@ -3651,7 +3649,7 @@ impl Editor {
return;
};
if !self.snippet_stack.is_empty() && self.context_menu.read().as_ref().is_some() {
if !self.snippet_stack.is_empty() && self.context_menu.borrow().as_ref().is_some() {
return;
}
@@ -3670,7 +3668,7 @@ impl Editor {
let query = Self::completion_query(&self.buffer.read(cx).read(cx), position);
let aside_was_displayed = match self.context_menu.read().deref() {
let aside_was_displayed = match self.context_menu.borrow().deref() {
Some(CodeContextMenu::Completions(menu)) => menu.aside_was_displayed.get(),
_ => false,
};
@@ -3723,16 +3721,14 @@ impl Editor {
};
editor.update(&mut cx, |editor, cx| {
let mut context_menu = editor.context_menu.write();
let mut context_menu = editor.context_menu.borrow_mut();
match context_menu.as_ref() {
None => {}
Some(CodeContextMenu::Completions(prev_menu)) => {
if prev_menu.id > id {
return;
}
}
_ => return,
}
@@ -3795,7 +3791,7 @@ impl Editor {
.matches
.get(item_ix.unwrap_or(completions_menu.selected_item))?;
let buffer_handle = completions_menu.buffer;
let completions = completions_menu.completions.read();
let completions = completions_menu.completions.borrow_mut();
let completion = completions.get(mat.candidate_id)?;
cx.stop_propagation();
@@ -3961,7 +3957,7 @@ impl Editor {
}
pub fn toggle_code_actions(&mut self, action: &ToggleCodeActions, cx: &mut ViewContext<Self>) {
let mut context_menu = self.context_menu.write();
let mut context_menu = self.context_menu.borrow_mut();
if let Some(CodeContextMenu::CodeActions(code_actions)) = context_menu.as_ref() {
if code_actions.deployed_from_indicator == action.deployed_from_indicator {
// Toggle if we're selecting the same one
@@ -4041,7 +4037,7 @@ impl Editor {
};
let resolved_tasks =
tasks.zip(task_context).map(|(tasks, task_context)| {
Arc::new(ResolvedTasks {
Rc::new(ResolvedTasks {
templates: tasks.resolve(&task_context).collect(),
position: snapshot.buffer_snapshot.anchor_before(Point::new(
multibuffer_point.row,
@@ -4056,7 +4052,7 @@ impl Editor {
.as_ref()
.map_or(true, |actions| actions.is_empty());
if let Ok(task) = editor.update(&mut cx, |editor, cx| {
*editor.context_menu.write() =
*editor.context_menu.borrow_mut() =
Some(CodeContextMenu::CodeActions(CodeActionsMenu {
buffer,
actions: CodeActionContents {
@@ -4241,7 +4237,7 @@ impl Editor {
pub fn push_code_action_provider(
&mut self,
provider: Arc<dyn CodeActionProvider>,
provider: Rc<dyn CodeActionProvider>,
cx: &mut ViewContext<Self>,
) {
self.code_action_providers.push(provider);
@@ -5004,7 +5000,7 @@ impl Editor {
pub fn context_menu_visible(&self) -> bool {
self.context_menu
.read()
.borrow()
.as_ref()
.map_or(false, |menu| menu.visible())
}
@@ -5016,7 +5012,7 @@ impl Editor {
max_height: Pixels,
cx: &mut ViewContext<Editor>,
) -> Option<(ContextMenuOrigin, AnyElement)> {
self.context_menu.read().as_ref().map(|menu| {
self.context_menu.borrow().as_ref().map(|menu| {
menu.render(
cursor_position,
style,
@@ -5030,7 +5026,7 @@ impl Editor {
fn hide_context_menu(&mut self, cx: &mut ViewContext<Self>) -> Option<CodeContextMenu> {
cx.notify();
self.completion_tasks.clear();
self.context_menu.write().take()
self.context_menu.borrow_mut().take()
}
fn show_snippet_choices(
@@ -5047,7 +5043,7 @@ impl Editor {
let id = post_inc(&mut self.next_completion_id);
if let Some(buffer) = buffer {
*self.context_menu.write() = Some(CodeContextMenu::Completions(
*self.context_menu.borrow_mut() = Some(CodeContextMenu::Completions(
CompletionsMenu::new_snippet_choices(id, true, choices, selection, buffer),
));
}
@@ -7101,7 +7097,7 @@ impl Editor {
if self
.context_menu
.write()
.borrow_mut()
.as_mut()
.map(|menu| menu.select_first(self.completion_provider.as_deref(), cx))
.unwrap_or(false)
@@ -7210,7 +7206,7 @@ impl Editor {
if self
.context_menu
.write()
.borrow_mut()
.as_mut()
.map(|menu| menu.select_last(self.completion_provider.as_deref(), cx))
.unwrap_or(false)
@@ -7263,25 +7259,25 @@ impl Editor {
}
pub fn context_menu_first(&mut self, _: &ContextMenuFirst, cx: &mut ViewContext<Self>) {
if let Some(context_menu) = self.context_menu.write().as_mut() {
if let Some(context_menu) = self.context_menu.borrow_mut().as_mut() {
context_menu.select_first(self.completion_provider.as_deref(), cx);
}
}
pub fn context_menu_prev(&mut self, _: &ContextMenuPrev, cx: &mut ViewContext<Self>) {
if let Some(context_menu) = self.context_menu.write().as_mut() {
if let Some(context_menu) = self.context_menu.borrow_mut().as_mut() {
context_menu.select_prev(self.completion_provider.as_deref(), cx);
}
}
pub fn context_menu_next(&mut self, _: &ContextMenuNext, cx: &mut ViewContext<Self>) {
if let Some(context_menu) = self.context_menu.write().as_mut() {
if let Some(context_menu) = self.context_menu.borrow_mut().as_mut() {
context_menu.select_next(self.completion_provider.as_deref(), cx);
}
}
pub fn context_menu_last(&mut self, _: &ContextMenuLast, cx: &mut ViewContext<Self>) {
if let Some(context_menu) = self.context_menu.write().as_mut() {
if let Some(context_menu) = self.context_menu.borrow_mut().as_mut() {
context_menu.select_last(self.completion_provider.as_deref(), cx);
}
}
@@ -12818,7 +12814,7 @@ impl Editor {
}
pub fn has_active_completions_menu(&self) -> bool {
self.context_menu.read().as_ref().map_or(false, |menu| {
self.context_menu.borrow().as_ref().map_or(false, |menu| {
menu.visible() && matches!(menu, CodeContextMenu::Completions(_))
})
}
@@ -13314,7 +13310,7 @@ pub trait CompletionProvider {
&self,
buffer: Model<Buffer>,
completion_indices: Vec<usize>,
completions: Arc<RwLock<Box<[Completion]>>>,
completions: Rc<RefCell<Box<[Completion]>>>,
cx: &mut ViewContext<Editor>,
) -> Task<Result<bool>>;
@@ -13544,7 +13540,7 @@ impl CompletionProvider for Model<Project> {
&self,
buffer: Model<Buffer>,
completion_indices: Vec<usize>,
completions: Arc<RwLock<Box<[Completion]>>>,
completions: Rc<RefCell<Box<[Completion]>>>,
cx: &mut ViewContext<Editor>,
) -> Task<Result<bool>> {
self.update(cx, |project, cx| {
crates/editor/src/editor_tests.rs
+23 -16
View File
@@ -8342,9 +8342,9 @@ async fn test_completion(cx: &mut gpui::TestAppContext) {
additional edit
"});
cx.simulate_keystroke(" ");
assert!(cx.editor(|e, _| e.context_menu.read().is_none()));
assert!(cx.editor(|e, _| e.context_menu.borrow_mut().is_none()));
cx.simulate_keystroke("s");
assert!(cx.editor(|e, _| e.context_menu.read().is_none()));
assert!(cx.editor(|e, _| e.context_menu.borrow_mut().is_none()));
cx.assert_editor_state(indoc! {"
one.second_completion
@@ -8406,12 +8406,12 @@ async fn test_completion(cx: &mut gpui::TestAppContext) {
});
cx.set_state("editorˇ");
cx.simulate_keystroke(".");
assert!(cx.editor(|e, _| e.context_menu.read().is_none()));
assert!(cx.editor(|e, _| e.context_menu.borrow_mut().is_none()));
cx.simulate_keystroke("c");
cx.simulate_keystroke("l");
cx.simulate_keystroke("o");
cx.assert_editor_state("editor.cloˇ");
assert!(cx.editor(|e, _| e.context_menu.read().is_none()));
assert!(cx.editor(|e, _| e.context_menu.borrow_mut().is_none()));
cx.update_editor(|editor, cx| {
editor.show_completions(&ShowCompletions { trigger: None }, cx);
});
@@ -8468,7 +8468,8 @@ async fn test_completion_page_up_down_keys(cx: &mut gpui::TestAppContext) {
cx.executor().run_until_parked();
cx.update_editor(|editor, _| {
if let Some(CodeContextMenu::Completions(menu)) = editor.context_menu.read().as_ref() {
if let Some(CodeContextMenu::Completions(menu)) = editor.context_menu.borrow_mut().as_ref()
{
assert_eq!(
menu.matches.iter().map(|m| &m.string).collect::<Vec<_>>(),
&["first", "last"]
@@ -8480,7 +8481,8 @@ async fn test_completion_page_up_down_keys(cx: &mut gpui::TestAppContext) {
cx.update_editor(|editor, cx| {
editor.move_page_down(&MovePageDown::default(), cx);
if let Some(CodeContextMenu::Completions(menu)) = editor.context_menu.read().as_ref() {
if let Some(CodeContextMenu::Completions(menu)) = editor.context_menu.borrow_mut().as_ref()
{
assert!(
menu.selected_item == 1,
"expected PageDown to select the last item from the context menu"
@@ -8492,7 +8494,8 @@ async fn test_completion_page_up_down_keys(cx: &mut gpui::TestAppContext) {
cx.update_editor(|editor, cx| {
editor.move_page_up(&MovePageUp::default(), cx);
if let Some(CodeContextMenu::Completions(menu)) = editor.context_menu.read().as_ref() {
if let Some(CodeContextMenu::Completions(menu)) = editor.context_menu.borrow_mut().as_ref()
{
assert!(
menu.selected_item == 0,
"expected PageUp to select the first item from the context menu"
@@ -8560,7 +8563,8 @@ async fn test_completion_sort(cx: &mut gpui::TestAppContext) {
cx.executor().run_until_parked();
cx.update_editor(|editor, _| {
if let Some(CodeContextMenu::Completions(menu)) = editor.context_menu.read().as_ref() {
if let Some(CodeContextMenu::Completions(menu)) = editor.context_menu.borrow_mut().as_ref()
{
assert_eq!(
menu.matches.iter().map(|m| &m.string).collect::<Vec<_>>(),
&["r", "ret", "Range", "return"]
@@ -10704,13 +10708,13 @@ async fn test_completions_resolve_updates_labels_if_filter_text_matches(
cx.condition(|editor, _| editor.context_menu_visible())
.await;
cx.update_editor(|editor, _| {
let context_menu = editor.context_menu.read();
let context_menu = editor.context_menu.borrow_mut();
let context_menu = context_menu
.as_ref()
.expect("Should have the context menu deployed");
match context_menu {
CodeContextMenu::Completions(completions_menu) => {
let completions = completions_menu.completions.read();
let completions = completions_menu.completions.borrow_mut();
assert_eq!(
completions
.iter()
@@ -10761,13 +10765,13 @@ async fn test_completions_resolve_updates_labels_if_filter_text_matches(
cx.run_until_parked();
cx.update_editor(|editor, _| {
let context_menu = editor.context_menu.read();
let context_menu = editor.context_menu.borrow_mut();
let context_menu = context_menu
.as_ref()
.expect("Should have the context menu deployed");
match context_menu {
CodeContextMenu::Completions(completions_menu) => {
let completions = completions_menu.completions.read();
let completions = completions_menu.completions.borrow_mut();
assert_eq!(
completions
.iter()
@@ -10954,7 +10958,7 @@ async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppCo
.await;
cx.run_until_parked();
cx.update_editor(|editor, _| {
let menu = editor.context_menu.read();
let menu = editor.context_menu.borrow_mut();
match menu.as_ref().expect("should have the completions menu") {
CodeContextMenu::Completions(completions_menu) => {
assert_eq!(
@@ -11059,7 +11063,8 @@ async fn test_completions_in_languages_with_extra_word_characters(cx: &mut gpui:
cx.simulate_keystroke("-");
cx.executor().run_until_parked();
cx.update_editor(|editor, _| {
if let Some(CodeContextMenu::Completions(menu)) = editor.context_menu.read().as_ref() {
if let Some(CodeContextMenu::Completions(menu)) = editor.context_menu.borrow_mut().as_ref()
{
assert_eq!(
menu.matches.iter().map(|m| &m.string).collect::<Vec<_>>(),
&["bg-red", "bg-blue", "bg-yellow"]
@@ -11072,7 +11077,8 @@ async fn test_completions_in_languages_with_extra_word_characters(cx: &mut gpui:
cx.simulate_keystroke("l");
cx.executor().run_until_parked();
cx.update_editor(|editor, _| {
if let Some(CodeContextMenu::Completions(menu)) = editor.context_menu.read().as_ref() {
if let Some(CodeContextMenu::Completions(menu)) = editor.context_menu.borrow_mut().as_ref()
{
assert_eq!(
menu.matches.iter().map(|m| &m.string).collect::<Vec<_>>(),
&["bg-blue", "bg-yellow"]
@@ -11088,7 +11094,8 @@ async fn test_completions_in_languages_with_extra_word_characters(cx: &mut gpui:
cx.simulate_keystroke("l");
cx.executor().run_until_parked();
cx.update_editor(|editor, _| {
if let Some(CodeContextMenu::Completions(menu)) = editor.context_menu.read().as_ref() {
if let Some(CodeContextMenu::Completions(menu)) = editor.context_menu.borrow_mut().as_ref()
{
assert_eq!(
menu.matches.iter().map(|m| &m.string).collect::<Vec<_>>(),
&["bg-yellow"]
crates/editor/src/element.rs
+2 -2
View File
@@ -1686,7 +1686,7 @@ impl EditorElement {
deployed_from_indicator,
actions,
..
})) = editor.context_menu.read().as_ref()
})) = editor.context_menu.borrow().as_ref()
{
actions
.tasks
@@ -1777,7 +1777,7 @@ impl EditorElement {
if let Some(crate::CodeContextMenu::CodeActions(CodeActionsMenu {
deployed_from_indicator,
..
})) = editor.context_menu.read().as_ref()
})) = editor.context_menu.borrow().as_ref()
{
active = deployed_from_indicator.map_or(true, |indicator_row| indicator_row == row);
};