Implement .as_weak() and .upgrade() for global component instances#9436
Implement .as_weak() and .upgrade() for global component instances#9436bennysj wants to merge 1 commit intoslint-ui:masterfrom
Conversation
ab2a50a to
595f2e2
Compare
d2882a0 to
91264dc
Compare
internal/compiler/generator/rust.rs
Outdated
| #[repr(C)] | ||
| #[pin] | ||
| #pub_token struct #inner_component_id { | ||
| pub struct #inner_component_id { |
There was a problem hiding this comment.
Is this needed as part of this feature?
There was a problem hiding this comment.
Need to remember,... but it's due to the PinnedInner type, unfortunately to make this work, this may need to be 'pub'
internal/core/api.rs
Outdated
| pub fn to_global<'a>(&'a self) -> T::Global<'a> { | ||
| T::to_self(&self.0) | ||
| } |
There was a problem hiding this comment.
Would it perhaps make more sense to implement AsRef<T> for GlobalStrong<T>, instead of this new function?
There was a problem hiding this comment.
That might be a good idea, let me try implement that.
There was a problem hiding this comment.
Implementing AsRef and/or Deref for GlobalStrong is not possible (unless there is some Rust wizardy that I don't know about yet) The reason is that Global has a reference '&' and a lifetime 'a to the inner.
I've just renamed this method to 'global', that may be more appropriate.
'as_ref' does not feel right, as it's not returning a reference '&'
| /// If the current thread is not the thread that created the component the functor | ||
| /// will not be called and this function will do nothing. |
There was a problem hiding this comment.
It's a bit unfortunate that that's a silent error. I think my preference would be to omit this function, as it can "fail" for two unrelated reasons that the user won't be able to find out about.
If I call upgrade_in() from the wrong thread, it's a programmer error. If the upgrade fails because the global doesn't exist anymore, it's what could be a normal condition.
There was a problem hiding this comment.
Thinking about it, I think both should be a programmer error.
- Different thread, let's just panic
- If it does not exist anymore, then it's called after the MainWindow is destroyed, I think this is also a programmer error, let also panic
There was a problem hiding this comment.
Panicking in Rust is a last resort, I would much appreciate it if we can avoid it.
This is too easy to get wrong accidentally for a panic IMO.
I believe the most useful way to allow the user to get feedback here is to just return a Result with two Error values: NotInMainThread and GlobalDeallocated or something like that (we can bikeshed the naming).
Then we can make fun also return a value R, and just have this function return a Result<R, Error>.
That allows you to both return values from the global (which is currently not possible, and get an indication of whether this operation was succesful).
If you don't care about either of these, just call .ok().
| move |bdata| { | ||
| { | ||
| let blogica_api = blogica_api.upgrade().unwrap(); | ||
| let blogica_api = blogica_api.to_global(); |
There was a problem hiding this comment.
This is where I think .as_ref() is the idiomatic name. It could also implement Deref perhaps? Conceptually I think this should behave like a smart pointer.
There was a problem hiding this comment.
On my previous comment, it does not return a reference '&', therefor, at least me, it does not feel correct
46a0a06 to
14718b4
Compare
6fab6d9 to
01aacdd
Compare
|
What's more is needed to get this PR merged ? |
LeonMatthes
left a comment
There was a problem hiding this comment.
Overall I'm in favor of merging this into master, now that 1.15 has branched. Then we can refine the API until the next release or hide it behind an experimental feature flag if needed.
(Or we put it behind experimental in this PR directly)
I suggest we also leave the corresponding issue open, as the API will still need some iteration.
Thank you for the work on this @bennysj
cc @ogoffart
| /// If the current thread is not the thread that created the component the functor | ||
| /// will not be called and this function will do nothing. |
There was a problem hiding this comment.
Panicking in Rust is a last resort, I would much appreciate it if we can avoid it.
This is too easy to get wrong accidentally for a panic IMO.
I believe the most useful way to allow the user to get feedback here is to just return a Result with two Error values: NotInMainThread and GlobalDeallocated or something like that (we can bikeshed the naming).
Then we can make fun also return a value R, and just have this function return a Result<R, Error>.
That allows you to both return values from the global (which is currently not possible, and get an indication of whether this operation was succesful).
If you don't care about either of these, just call .ok().
| blogica_api.set_code4(SharedString::from("One more important thing")); | ||
|
|
||
| blogica_api.on_update({ | ||
| let blogica_api = blogica_api.as_weak(); |
There was a problem hiding this comment.
Seeing this pattern again gave me another idea for #1714 (no need to implement in this PR).
We're seeing this pattern quite often with callbacks:
- Downgrade the handle
- Move it into the callback as weak
- In the callback, upgrade the handle
Can we put this in a higher-order function inside the handle? (pseudo code)
impl ComponentHandle {
fn callback(func: impl FnMut(...) -> T) -> impl FnMut(&self, ...) -> T {
let weak = self.as_weak();
move |...| {
let Some(strong) = weak.upgrade() else { return; }
func(strong, ...);
}
}
}Then from the outside it would be:
blogica_api.on_update(blogica_api.callback(|blogica_api, bdata|{
// blogica_api is a strong reference!
}))cc @ogoffart
| /// The type for the public global component interface. | ||
| #[doc(hidden)] | ||
| type Global<'a>; | ||
| /// The internal Inner type for `Weak<Self>::inner`. | ||
| #[doc(hidden)] | ||
| type WeakInner: Clone + Default; | ||
| /// The internal Inner type for the 'Pin<sp::Rc<InnerSelf>'. | ||
| #[doc(hidden)] | ||
| type PinnedInner: Clone; |
There was a problem hiding this comment.
If my understanding is correct here, the Global is already a StrongRef, but with a restricted lifetime.
(And this is how it is already implemented today).
So could we just remove the lifetime from the Global? (i.e. make all Global lifetimes 'static for backwards-compatibility)?
Then we would not need a third type (weak, strong, global), but would just have weak and global.
For backwards compatibility we could keep the restricted lifetime if you get the global from the window, but allow you to "make it strong", which would just morph the lifetime to 'static.
There was a problem hiding this comment.
If that approach is possible, I'm all in for it. I spent a lot of time trying to achieve that, but ofc, there are still things / tricks with the Rust language that I don't know about.
@LeonMatthes maybe if you have some time, if want to help with that 'morph the lifetime to 'static.' :)
There was a problem hiding this comment.
I will definitely experiment with this at some point. If you want to try it together we can set up a call :)
There was a problem hiding this comment.
I just remembered that we did something similar for Cxx-Qt.
You can lifetime restrict a type, but then type-alias it to a 'static version, which basically removes the lifetime restriction.
KDAB/cxx-qt@04e2041#diff-01122b5fd9adeaf324e2140e91407ffd699d5ca184cb31a21f2024c7e945ed2aR30
In our case, this will still mean we end up with a separate "type" from our API side, but not for the implementation, and we wouldn't need the to_global function anymore.
If Rust supported default lifetimes, we could just add 'static as the default lifetime, thereby effectively removing the need to specify the type while keeping compatibility.
ogoffart
left a comment
There was a problem hiding this comment.
If merged, this should be kept in experimental right now.
I'd like to make more experiment to see if it is possible to avoid having two different public trait and types for the global handle.
I'm wondering if we can re-use the one we have by making some subtle changes while keeping compatibility.
internal/compiler/generator/rust.rs
Outdated
| #[repr(C)] | ||
| #[pin] | ||
| #pub_token struct #inner_component_id { | ||
| pub struct #inner_component_id { |
There was a problem hiding this comment.
This is also unfortunate.
Does this allow users to access internal implementation of all components?
There was a problem hiding this comment.
It's indeed a bit unfortunate, but so far there is no other way (as far I know), and also when making external libraries (.as_library()) it's the same. It also needs to have the 'inner' to be public.
There was a problem hiding this comment.
For now it will only be 'public' if this experimental feature is enabled.
| pub struct GlobalWeak<T: GlobalComponentHandle> { | ||
| inner: T::WeakInner, | ||
| #[cfg(feature = "std")] | ||
| thread: std::thread::ThreadId, | ||
| } |
There was a problem hiding this comment.
I think we should be able to unify this with the normal weak reference we have. If we make the normal ComponentHandle trait inherit from a Upgradable/WeakHandle trait that is shared between the global and the component.
Unless we actively want to separate the different types of weak handles.
There was a problem hiding this comment.
This is now under 'experimental-library-module' and .to_global() step is gone
df8b611 to
323f4a4
Compare
The idea to make it easier and more convenient to pass a reference to a Slint global instance into a Rust closure / lambda. Fixes slint-ui#9389
323f4a4 to
fbb705a
Compare
The idea to make it easier and more convenient to pass a reference to a Slint global instance into a Rust closure / lambda.
The current implementation works the following way:
Fixes #6968