Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug with auto replacing components in compositor #1711

Merged
merged 2 commits into from
Mar 3, 2022

Conversation

sudormrfbin
Copy link
Member

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. Ideally we can use some other mechanism which doesn't depend on type_name().

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.
@archseer
Copy link
Member

But if an ID is specified, we use that instead of the type name? All popups already use a custom id. The type name is only a fallback on components that don't expose an id

@sudormrfbin
Copy link
Member Author

Ah we use both the id and type name instead of just the id for find_id:

.find(|component| component.type_name() == type_name && component.id() == Some(id))

I changed it now to only use the id.

find_id() filters a list of boxed components to find the component with the given id, but it still needs to downcast it into a concrete component type like Popup to return it. So I kept around the type parameter in replace_or_push() instead of reverting it into the old behavior of using a trait object.

@archseer archseer merged commit 5c810e5 into helix-editor:master Mar 3, 2022
@sudormrfbin sudormrfbin deleted the fix-compositor-find-id branch March 3, 2022 02:37
@sudormrfbin sudormrfbin mentioned this pull request Mar 5, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants