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

Signals that get triggered synchronously #23

Closed idanarye closed 3 years ago

idanarye commented 3 years ago

(continuing the discussion from #15 and https://github.com/idanarye/woab/pull/20#pullrequestreview-627452843)

Previously WoAB handled signals completely asynchronously - the signal handler would send a message to the Actix actor and would not wait for a result. If the handler needed to return a special value (inhibit decision) it would have to be configured by other means.

20 changes it so that signal handlers would block_on the message, waiting for Actix to finish handling it, and have the result of that message be the inhibit decision if the signal requires one.

This approach raises a problem - some signals can be triggered from inside GTK functions. For example, if you are removing a widget from inside the Actix runtime, the remove signal will be triggered immediately - still inside the Actix runtime. This means you can't use Actix' block_on!

The current solution is to detect this scenario and when it happen - schedule the signal to run later and return None immediately. When the actual signal happens, it makes sure it didn't fail and that it returned None (to enforce consistency). This works for remove, which does not expect an inhibit decision, but I cannot guarantee it'll work properly for all signals. I experimented a bit and the very generic event signal - which does expect an inhibit decision - does run synchronously sometimes, but I'm not sure if that signal is actually used and is an actual problem.

Still - this is something that needs to be addressed.

piegamesde commented 3 years ago

Some minor clarifications:

piegamesde commented 3 years ago

The more I think about this, the more I come to the conclusion that it an unsolvable problem within our constraints. Gtk and Actix are fundamentally different by design, and by making this work we'd lose the comfort Actix gives us. Thus, we need to define some restrictions, enforce them in the API and gracefully quit when they are violated; without sacrificing the usability.

First of all, let me show you what I mean: we're in a signal handler, and thus got a &mut self. We cannot borrow it again, therefore even if we managed to get the async runtime to cooperate, we still couldn't run the inner handler. The convenience of Actix explicitly comes from the fact that events are queued and processed deferred, because it gives us a &mut self.

We need to disallow the direct execution of a signal within a signal handler. Here's how we could do this: Most signal handlers don't care if they're run directly or deferred, that was the default before the overhaul and it worked fine most of the time. We need to distinguish between both execution modes, and allow triggering signals as long as they are deferred.

Here's another idea that solves this (not sure about the ergonomics though): our Signal handler not only returns a value, but also a closure. That closure has no access to the actor, but is allowed to trigger other signals, as it will run outside of the engine. This is inspired by Actix's approach to async: "do the synchronous work first, then return a Future".

idanarye commented 3 years ago
  • Synchronous signal processing is not only needed for Inhibit values, but also for handling draw calls and using the block_signal API.

Why would the block_signal API need synchronous signal processing? Doesn't ObjectExt::block_signal accept a signal handler? Why must it be called while the handler is running?

  • All signal handlers can be triggered from within other signal handlers, simply by emitting the signal. I'm not 100% sure, but this might also include events because of handle_event. The only exception from this that I see is queue_draw and similar functions.

I'm going to draw the line here. I want to support any signal that comes from GTK, but I'm not going to bend WoAB that much to let users send signals to themselves. Yes, GLib signals can be used as a method of communication between the different parts of your application code, but if you are doing that with signals instead of Actix messages why would even use WoAB?

piegamesde commented 3 years ago

Why would the block_signal API need synchronous signal processing? Doesn't ObjectExt::block_signal accept a signal handler? Why must it be called while the handler is running?

You're right, the block_signal should work regardless.

I'm going to draw the line here.

That's the thing: Widgets (or Actions, or objects in general) have properties, they emit signals when they change, and you register signal handlers to get notified. As soon as you programatically set any properties – which is rather common in GUI development – it synchronously runs the signal handler for that property again. :tada:, you just sent yourself a signal.

idanarye commented 3 years ago

Yea, this is the same problem I had with the remove signal. The problem with properties changes is that property-notify-event expect an inhibit decision - which the current solution cannot provide.

I like your idea of making the user schedule the action that triggers the signal to happen outside the Actix runtime. I don't like, however, the idea of putting this in the message result, for the following reasons:

  1. It makes the handler methods return type - which users will always have to interact with - more complex.
  2. These actions are not necessarily triggered from other signals. They can be triggered from handlers that handle any Actix message. They can be triggered from other Tokio sources (like a socket stream). They can be triggered inside Future maps.
  3. Having to put it in the return value can complicate the logic of the handler.

Instead, how having a woab::schedule_outside() that can be called from anywhere and schedules a task to run outside the Actix runtime? https://mmstick.keybase.pub/rust-gtk-practices/ says we can use glib::Receiver::attach for that, but we can also do it with a simple VecDequeue.

piegamesde commented 3 years ago

Instead, how having a woab::schedule_outside() that can be called from anywhere and schedules a task to run outside the Actix runtime?

It's worth a try. Doesn't look bad so far, but I'll only be able to tell after having tried it out myself.

Regarding the cloning, if you need to clone many widgets, you can put your Widgets struct into an Rc (since you never mutate it, because all widgets in it have interior mutation), and only clone that. But that's more a usage idiom that doesn't need API (I don't think it'll ever be worth a macro, but time will tell), so everything's fine.