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

Use TypeId to identify subscription::Map #2237

Merged
merged 4 commits into from
Feb 5, 2024
Merged

Conversation

hecrj
Copy link
Member

@hecrj hecrj commented Feb 4, 2024

This PR fixes #2236. The idea here is to use the TypeId of a closure instead of the hash of a function pointer in subscription::Map, which I am assuming is more stable.

Coincidentally, this also introduces closure support for Subscription::map!

@hecrj hecrj added bug Something isn't working feature New feature or request shell change fix labels Feb 4, 2024
@hecrj hecrj added this to the 0.12 milestone Feb 4, 2024
@hecrj hecrj force-pushed the fix/mapped-subscription-id branch 2 times, most recently from 73ce77c to 708665c Compare February 4, 2024 23:27
@hecrj hecrj mentioned this pull request Feb 4, 2024
2 tasks
@snaggen
Copy link

snaggen commented Feb 5, 2024

I also had to add

diff --git a/winit/src/application.rs b/winit/src/application.rs
index 9cd992f4..99c7a666 100644
--- a/winit/src/application.rs
+++ b/winit/src/application.rs
@@ -830,7 +830,7 @@ where

 /// Updates an [`Application`] by feeding it the provided messages, spawning any
 /// resulting [`Command`], and tracking its [`Subscription`].
-pub fn update<A: Application, C, E: Executor>(
+pub fn update<A: Application + 'static, C, E: Executor + 'static>(
     application: &mut A,
     compositor: &mut C,
     surface: &mut C::Surface,

to my local libcosmic tree for it to build. But with that compile fix things seems to work

@snaggen
Copy link

snaggen commented Feb 5, 2024

I assume using TypeId will change the semantics slightly, that will only update the subscription if the closure is changed in such a way that the type changes. If you change the closure in such a way that the type is still the same, the subscription will not be updated. So, if is desired to have this kind of functionality, maybe an explicit id like in the subscription::channel function is preferred. That would break the API though. So, not sure what is better, change the semantics or break API.

@hecrj
Copy link
Member Author

hecrj commented Feb 5, 2024

I assume using TypeId will change the semantics slightly, that will only update the subscription if the closure is changed in such a way that the type changes.

Technically yes, but it should work as expected with function item types; which is the intended way to use the API here.

It'd be great to be able to restrict the closure to function item types, but I don't think that's possible in Rust yet.

@snaggen
Copy link

snaggen commented Feb 5, 2024

In that case, I say, just document it and ship it :)

@hecrj hecrj enabled auto-merge February 5, 2024 20:36
@hecrj hecrj merged commit 8e76d53 into master Feb 5, 2024
26 checks passed
@hecrj hecrj deleted the fix/mapped-subscription-id branch February 5, 2024 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working change feature New feature or request fix shell
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subscritptions are run twice
2 participants