idanarye / woab

Widgets on Actors Bridge - a GUI microframework for combining GTK with Actix
https://crates.io/crates/woab
MIT License
14 stars 1 forks source link

Remove `woab::schedule_outside` #28

Closed piegamesde closed 3 years ago

piegamesde commented 3 years ago

The same functionality is already provided by MainContext::invoke_local and – after the next release – glib::source::idle_add_local_once. There are also async methods, which is neat.

idanarye commented 3 years ago

Maybe WoAB should provide a shortcut for that? glib::MainContext::ref_thread_default()::invoke_local is a bit too long...

piegamesde commented 3 years ago

I'm using glib::MainContext::default()::invoke_local, but that's beside the point. One could simply re-export glib::source::idle_add_local_once under a shorter name. I don't really know if there are internal differences between all of these or not.

idanarye commented 3 years ago

invoke_local doesn't work. I tried replacing the schedule_outside in example_taggged_signals.rs with both

glib::MainContext::default().invoke_local(move || {

and

glib::MainContext::ref_thread_default().invoke_local(move || {

And both of them were running the closure immediately. I think using invoke_local when you already are local runs the closure immediately instead of scheduling it to run later.

idle_add_local_once is not available yet, but I did try this:

glib::source::idle_add_local(move || {
    lst_addition.remove(&addend);
    glib::Continue(false)
});

And this one did work. However, this one is something I'd want to wrap to avoid having to manually return glib::Continue(false), and at this point I see no reason to not just go with my mechanism which does not require FFI.

piegamesde commented 3 years ago

That's really interesting. I haven't used invoke_local local recently, only spawn_local and it seemed to work for me. I assumed one was simply the async version of the other, so I'm a bit surprised that they have different behavior.

I contributed idle_add_local_once as a wrapper specially for that use case, but yes it'll have to wait until the next release. In the end it doesn't matter which implementation we use, I just wanted to point out that technically schedule_outside provides redundant functionality and could as well be a re-export.

On the other hand, I mainly used the async version instead, so maybe that's worth adding a re-export as well for consistency? The pattern that I use at the moment is like:


Off-topic. By the way I wrote this little hack. I found it cleaner to use it instead of writing custom messages+handlers for actions that are triggered from only one place. You might be interested in this.

#[derive(actix::Message)]
#[rtype(result = "()")]
struct RunLater<T: Actor, F: FnOnce(&mut T)>(send_wrapper::SendWrapper<(F, std::marker::PhantomData<T>)>);

impl <T: Actor, F: FnOnce(&mut T)> RunLater<T, F> {
    fn new(closure: F) -> Self {
        Self(send_wrapper::SendWrapper::new((closure, std::marker::PhantomData)))
    }
}

impl <F: FnOnce(&mut AppActor)> actix::Handler<RunLater<AppActor, F>> for AppActor {
    type Result = ();

    fn handle(&mut self, message: RunLater<AppActor, F>, _ctx: &mut Self::Context) -> Self::Result {
        message.0.take().0(self);
    }
}
idanarye commented 3 years ago

Instead of spawning your future on GTK with spawn_local(), why not spawn it on Actix with ctx.spawn() and use ActorFuture to go back to the actor once the future is done?

Something like:

fn handle(...) -> woab::SignalResult {
    Ok(match msg {
        "the relevant signal" => {
            // Clone relevant widgets
            ctx.spawn(async move {
                // Do everything with the dialog
            }.into_actor(self)
            .map(|_, actor, _ctx| {
                // Finish up inside the actor
                Ok(())
            }));
            None
        }
        ...
    })
}
piegamesde commented 3 years ago

Conceptually, that's exactly what I need, but remember that the problem is to execute things outside of the actix engine, and spawn runs inside it. It works fine as long as I don't trigger some signals, but I can easily get "thread 'main' panicked at 'called Result::unwrap() on an Err value: "idle_add_local called inside Actix context"'" with this.

idanarye commented 3 years ago

I see. woab::schedule_outside deals with running stuff from synchronous Actix environment (handler) but here you want run stuff from an asynchronous Actix environment (spawned future) and return to that environment after the GTK stuff is done.

I think this calls for a new ticket. I'll open one.

idanarye commented 3 years ago

"idle_add_local called inside Actix context"'"

I should probably fix that error message - it's quite misleading. I'm more concerned, though, that this even happened - try_block_on should have in the action that triggered the signal, not here...

Is this easily reproducible?

idanarye commented 3 years ago

BTW - I checked spawn_local and it does run the future outside Actix, so it is usable. I kind of overlooked it because I was fixated on passing a closure, but passing an actor is also good.

I do insist though on giving it a wrapper method - I think woab::spawn_outside. My reasons:

piegamesde commented 3 years ago

Is this easily reproducible?

I think. Create a dialog inside a ctx.spawn( and then call dialog.run(). That should trigger it.

The error message is indeed misleading.

idanarye commented 3 years ago

OK - this is weird. The dialog runs in a nested GTK loop, but after you start it the main GTK loop's idle_add function we registered gets called again exactly one time - which is enough to trigger that error - and then not called until the dialog is resolved, which means anything on the Actix runtime which is not GTK signal handling gets frozen.

I'll open a new ticket for that.