Closed DJMcNab closed 1 year ago
Patch coverage: 75.60%
and project coverage change: -1.68%
:warning:
Comparison is base (
d468cc9
) 88.79% compared to head (33aed4a
) 87.11%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Also, it looks like the signals
test is failing? Is it because of the nix update?
So there are three changes you're asking for there:
1) To make AdditionalLifecycleEventsSet
not have the inner RefCell
. The current design was intended to avoid doing the borrow_mut
operations in the cases where they weren't needed, but that is only a small difference, as these shouldn't be on the hot path. This transformation hardly changes the code length, so I'll defer to @elinorbgr on this
1) To make the vector inside AdditionalLifecycleEventsSet
not type-safe. There is definitely a slight verbosity cost, but I believe it's generally better to be more type safe than less type safe. Again, I'll defer to @elinorbgr
1) To use the key which is already present in TokenFactory, thus avoiding AdditionalLifecycleEventsRegister
. I had avoided doing this because the key field on TokenFactory
isn't public. (Edit: I had made it public in a prior commit, which I never used) I think there are two aspects to this:
TokenFactory
should be exposed outside of sys.rs
. I personally that would be a layering violation, but I think it is also much cleaner for this PR. I think I will make this change (by adding a pub(crate) fn registration_token(&self)->RegistrationToken
method on TokenFactory
)
Just to make sure I didn't miss anything, point 1) is just about having the RefCell
inside or outside of the AdditionalLifecycleEventsSet
right? Given how small the difference is in terms of running, I think I'd prefer to have it outside, as it'd be more coherent with the rest of the contents of LoopInner
.
Regarding 2/3, I think I like the way it is currently on the PR, keeping the type-safety and using the token from he available tokenfactory is good imo, especially given this concerns crate-internals anyway.
Also, AdditionalLifetimeEventsSet
is still named using lifetime rather than lifecycle, I guess this one slipped through your attention? :sweat_smile:
Alright, thanks a lot this is perfect! :+1:
Solves #127. Designed to work with https://github.com/Smithay/wayland-rs/pull/650 and to be used to fix #glazier > Freeze with Wayland egl