Skip to content

Commit

Permalink
Fix bug with auto replacing components in compositor (#1711)
Browse files Browse the repository at this point in the history
* Fix bug with auto replacing components in compositor

This was last known to be working with 5995568 at the
time of commit, but now doesn't work with latest rust
stable.

The issue probably stems from using
std::any::type_name() for finding a component in the
compositor, for which the docs explicitly warn against
considering it as a unique identifier for types.

`replace_or_push()` takes a boxed `Component` and
passes it to `find_id()` which compares this with a
bare Component. `type_name()` returns `Box<T>` for
the former and `T` for latter and we have a false
negative. This has been solved by using a generics
instead of trait objects to pass in a `T: Component`
and then use it for comparison.

I'm not exactly sure how this worked fine at the
time of commit of 5995568; maybe the internal
implementation of `type_name()` changed to properly
indicate indirection with Box.

* Do not compare by type name in compositor find_id
  • Loading branch information
sudormrfbin authored Mar 3, 2022
1 parent 86b1236 commit 5c810e5
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 7 deletions.
2 changes: 1 addition & 1 deletion helix-term/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2980,7 +2980,7 @@ pub mod cmd {
Box::new(move |editor: &mut Editor, compositor: &mut Compositor| {
let contents = ui::Markdown::new(contents, editor.syn_loader.clone());
let popup = Popup::new("hover", contents).auto_close(true);
compositor.replace_or_push("hover", Box::new(popup));
compositor.replace_or_push("hover", popup);
});
Ok(call)
};
Expand Down
4 changes: 2 additions & 2 deletions helix-term/src/commands/lsp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ pub fn code_action(cx: &mut Context) {
vertical: 1,
horizontal: 1,
});
compositor.replace_or_push("code-action", Box::new(popup));
compositor.replace_or_push("code-action", popup);
},
)
}
Expand Down Expand Up @@ -637,7 +637,7 @@ pub fn hover(cx: &mut Context) {

let contents = ui::Markdown::new(contents, editor.syn_loader.clone());
let popup = Popup::new("hover", contents).auto_close(true);
compositor.replace_or_push("hover", Box::new(popup));
compositor.replace_or_push("hover", popup);
}
},
);
Expand Down
7 changes: 3 additions & 4 deletions helix-term/src/compositor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,11 @@ impl Compositor {

/// Replace a component that has the given `id` with the new layer and if
/// no component is found, push the layer normally.
pub fn replace_or_push(&mut self, id: &'static str, layer: Box<dyn Component>) {
pub fn replace_or_push<T: Component>(&mut self, id: &'static str, layer: T) {
if let Some(component) = self.find_id(id) {
*component = layer;
} else {
self.push(layer)
self.push(Box::new(layer))
}
}

Expand Down Expand Up @@ -221,10 +221,9 @@ impl Compositor {
}

pub fn find_id<T: 'static>(&mut self, id: &'static str) -> Option<&mut T> {
let type_name = std::any::type_name::<T>();
self.layers
.iter_mut()
.find(|component| component.type_name() == type_name && component.id() == Some(id))
.find(|component| component.id() == Some(id))
.and_then(|component| component.as_any_mut().downcast_mut())
}
}
Expand Down

0 comments on commit 5c810e5

Please sign in to comment.