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

[Brainstorm] Signal handling overhaul #15

Closed idanarye closed 3 years ago

idanarye commented 3 years ago

As seen in several issues (#7, #10, #13, ...) WoAB's signal handling is not working too well when used for anything more complex than simple click events. The main problems are:

The answer to both these problems is a new flow:

  1. Sends the signal as actix::Message (instead of using a Tokio stream)
  2. The message's result can be the inhibit value.
  3. The signal handler will block_on with the Tokio runtime until it gets that result - which will make the WoAB signal handling code run during the GTK signal handling.

The main problem with this approach is that Actix signals need to be Send - and many GTK signals contain GTK widgets or special data types (like GDK events or Cairo context) that are !Send.

The obvious solution is to use send_wrapper, but this would make the signal handling extremely ugly. I thought I could wrap it, but Rust's orphan rules won't let me.

Since I can't have my ideal solution - I'm going to have to brainstorm some other ideas, hoping that at least one of them can be refined to something presentable.

@piegamesde - I'd appreciate your feedback on this, and if you have ideas of your own I'd appreciated them as well.

idanarye commented 3 years ago

The simplest solution is to just have the BuilderSignal derive macro emit:

unsafe impl Send for SignalType {}

The main problem with this solution is that it's... well... unsafe. And I would be releasing that unsafety out to the wild, since nothing stops users from passing these signal values to other threads. So I really don't want to do this - but it is a possible solution.

idanarye commented 3 years ago

Another option is to wrap the signal:

impl actix::Handler<woab::Signal<SignalType>> {
    type Result = Option<gtk::Inhibit>;

    fn handle(&mut self, msg: woab::Signal<SignalType>, ctx: &mut Self::Context) -> Self::Result {
        match msg.signal() {
            SignalType::Sig1(...) => {
                ...
            }
            SignalType::Sig2(...) => {
                ...
            }
            ...
        }
    }
}

This is a bit more verbose than what we have now, but... it might not be that bad. I can also put the tag in woab::Signal with a default type of ().

idanarye commented 3 years ago

A third option is to use procedural macros and generate new syntax:

#[woab::signal_handling]
impl MyActor {
    fn button_click(self, ctx: &mut Self::Context, _: gtk::Button) {
        // do some stuff
    }
    fn button_press(self, ctx: &mut Self::Context, _: gtk::Button, value: gdk::EVentButton) -> gtk::Inhibit {
        // do some stuff
        gtk::Inhibit(false)
    }
}

The big advantage of this are:

  1. The parameter types are defined in their place of usage.
  2. Each handler can explicitly define if it returns nothing or returns inhibitness.

The problem are:

  1. It brings us back to version 0.1 when it comes to signal connection, because you'd have to connect the builder to the actor without having a signal type you could use. Unless I expose a signal type - which will make the syntax weird.
  2. One needs to declare self, ctx, and maybe even tag for every handler, and Rust Analyzer is not going to help with that.
idanarye commented 3 years ago

A variant of the woab::Signal solution would be to not parametrize it, and instead send the raw signal data and build the concrete signal in the handler:

impl actix::Handler<woab::Signal> {
    type Result = Option<gtk::Inhibit>;

    fn handle(&mut self, msg: woab::Signal, ctx: &mut Self::Context) -> Self::Result {
        match msg.signal().unwrap() {
            SignalType::Sig1(...) => {
                ...
            }
            SignalType::Sig2(...) => {
                ...
            }
            ...
        }
    }
}

Which... I thought will remove more boilerplate, but turns out it doesn't solve that much. It also has the problem of the initial routing - unlike woab::Signal<SignalType>, woab::Signal does not contain the list of supported signals, so if we want to split the signals of one builder to multiple actors we are going to have a hard time. Also, adding a tag when we are not already parametrized just feels weird...

So I don't like that idea that much, but this is a brainstorm so I'm writing all my ideas.

idanarye commented 3 years ago

Speaking of "all my ideas" - we don't have to switch from streams to messages. We can also pass a tokio::stream::once and have the handler fill it once it's done. This can be ugly - but maybe we can wrap it somehow?

piegamesde commented 3 years ago

I'd like to explore the marco idea some more. Effectively, the following pitch: let us restrict every actor to have at most one type for WoAB signals.

piegamesde commented 3 years ago

Regarding the other ideas:

idanarye commented 3 years ago

I'd like to explore the marco idea some more. Effectively, the following pitch: let us restrict every actor to have at most one type for WoAB signals.

  • Make a WoabActor trait that extends Actor
  • That trait has an associated type for the signal enum and a handle method
  • There is a proc-macro (derive macro?) that generates the signal enum and the trait implementation based on annotated methods on the actor type.

Why would we need that trait? The macro is just going to generate a signal handler that accepts a new woab::RawSignal that contains the signal name and Vec<glib::Value>, and in that handler distribute it to the functions. I'd rather not create a nameless type if I can avoid it. There will only be one type of woab::RawSignal - though I will probably parametrize it for tags.

Actually, one thing I'll need to put on a trait is the list of supported signals.

  • The StreamHandler and using a channel for backwards communication is fine for me, as long as we still pursue the idea of cranking tokio within the handler.

We will crank the Tokio within the handler either way. This is the main reason we are doing this change.

BTW - this idea can be combined with the macro idea. The main problem with this idea is that it's usage will be ugly and manual - and macros are really good at solving this kind of problem.

piegamesde commented 3 years ago

The macro is just going to generate a signal handler that accepts a new woab::RawSignal that contains the signal name and Vec, and in that handler distribute it to the functions.

I see. You are basically moving the Raw signal -> types conversion to a later point in the pipeline, thus avoiding the special message enum. That's of course a far better solution than my trait suggestion.

idanarye commented 3 years ago

Let's talk about the connecting syntax. In the tagged signals example we are creating a brand new actor like this:

factories
    .win_app
    .instantiate()
    .actor()
    .connect_signals(WindowSignal::connector())
    .create(|ctx| WindowActor {
        widgets: ctx.widgets().unwrap(),
        factories,
        next_addend_id: 0,
        addends: Default::default(),
    });

And we connect signals with a tag to an existing actor like this:

let widgets: AddendWidgets = self
    .factories
    .row_addend
    .instantiate()
    .connect_signals(ctx, AddendSignal::connector().tag(addend_id))
    .widgets()
    .unwrap();

The most straightforward conversion would be to create the connector from the actor type instead of the signal type:

factories
    .win_app
    .instantiate()
    .actor()
    .connect_signals(WindowActor::connector())
    .create(|ctx| WindowActor {
        widgets: ctx.widgets().unwrap(),
        factories,
        next_addend_id: 0,
        addends: Default::default(),
    });

let widgets: AddendWidgets = self
    .factories
    .row_addend
    .instantiate()
    .connect_signals(ctx, WindowActor::connector().tag(addend_id))
    .widgets()
    .unwrap();

However, since we are dropping the inhibit, I'm thinking of going back to the old style and drop ActorBuilder and BuilderSignalConnector. This means the actor will be created directly from the BuilderConnector, and the signals will be connected automatically:

factories
    .win_app
    .instantiate()
    .create_actor(|ctx| WindowActor {
        widgets: ctx.widgets().unwrap(),
        factories,
        next_addend_id: 0,
        addends: Default::default(),
    });

As for the tagged signals and connecting to an already existing actor - we'd just use a special method for the tag:

let widgets: AddendWidgets = self
    .factories
    .row_addend
    .instantiate()
    .connect_signals_tagged(ctx, addend_id)
    .widgets()
    .unwrap();

This feels like a downgrade, and if we ever have another special case like inhibit we'll have to go back to the more complex style, but I'm not sure it's worth keeping the more complex style just for future-proofing...

idanarye commented 3 years ago

We still need to support connecting builderless signals, but we probably don't need BuilderSignalConnector for that. SignalRouter would do:

let router = WindowActor::route_to(ctx);
router.connect(&action1, "activate", "Action1").unwrap();
router.connect(&action2, "activate", "Action2").unwrap();
router.connect(&action3, "activate", "Action3").unwrap();
idanarye commented 3 years ago

It turns out that RawSignal may not be a very good idea. If I don't parse the signal into a concrete type in the GTK signal handler itself, I need to convert the &[glib::Value] to a vector - which means allocation. Allocation that I was hoping to avoid...

piegamesde commented 3 years ago

I think there may be some tricks around this. An obvious solution would be to use ArrayVecs on the stack, (in the assumptions that most callbacks only have a few arguments). One could also try to re-use the allocation buffer across signals.

But I must admit that I'm starting to lose track of which change is causing this, and what motivated it. I suggest you to simply use Vecs and not worry for now. We can still try to minimize/avoid allocations afterwards if the current approach turns out to be fruitful.

idanarye commented 3 years ago

OK, I haven't worked on this in a while. I just came back from reserve duty, so I didn't have the time. And before that I didn't have the energy. But these are excuses - I think my problem is that the solution we picked is too big - I need to try to break it down a little.

Observation - one rarely needs all the parameters a GTK signal carries. So far I was focusing the design on the ergonomics of specifying all these parameters - but what if I don't?

Consider this:

impl actix::Handler<woab::Signal> {
    type result = woab::SignalResult; // Result<Option<Inhibit>, woab::Error>

    fn handle(&mut self, msg: woab::Signal, ctx: &mut Self::Context) -> Self::Result {
        match msg.signal {
            "signal1" => {
                let param0: gtk::Button = msg.param(0)?; // 99% of the time you won't need this one
                let param1: gdk::EventButton = msg.param_event(1)?;
                // Signal handling code.
                Ok(Some(gtk::Inhibit(false)))
            }
            "signal2" => {
                // Signal handling code without getting any parameters.
                Ok(None)
            }
            _ => msg.cant_handle()? // Generate a proper exception
        }
    }
}

Note that:

The main advantage of this style is that there are no macros. Not that I hate macros - a glance at my Rust projects will tell you how much I love them - but macros are harder to create, harder to maintain, harder to document, and compile slower - so if we can get something ergonomic without them that's better. That being said - this style should be easy to sugarize with macros in the future, if we want to, without breaking anything.

BTW - I put the exception on the return type on a whim, but I'm not sure what I'm going to do with it in the handler side other thanunwrapping it. It just feels cleaner to use ? on the arg parsing and on the no-match instead of doing the unwrap in user code:

impl actix::Handler<woab::Signal> {
    type result = Option<gtk::Inhibit>;

    fn handle(&mut self, msg: woab::Signal, ctx: &mut Self::Context) -> Self::Result {
        match msg.signal {
            "signal1" => {
                let param0: gtk::Button = msg.param(0).unwrap();
                let param1: gdk::EventButton = msg.param_event(1).unwrap();
                // Signal handling code.
                Some(gtk::Inhibit(false))
            }
            "signal2" => {
                // Signal handling code without getting any parameters.
                None
            }
            _ => msg.cant_handle().unwrap()
        }
    }
}
piegamesde commented 3 years ago

That's basically the same breakdown I did in #19. We agreed on the Message and return type of the Handler, so the first step is to hook it up from the engine. This is probably the harder part anyways. Then, we can decide on how to make the handler implementation ergonomic – with or without macros.

idanarye commented 3 years ago

Not really. #19 still relies on a macro - or that's what I assume, looking at the scaffolds of the API you have there. It still has a const SIGNALS: &'static [&'static str]; which tells WoAB which signals to connect to the actor and allows them to be &static str (I think I'll have to make mine Rc<String>...). My idea is to ditch that - the signals will only be compiled into the match's arms, and there will be no macro that extracts a SIGNALS list.

Which, now that I think about it, kind of comes in the way of #3. If the actor (/ signal enum - which we no longer have) can't declare the list of signals, I'll have to either send the signal to all the actors (ugh...) or - which is probably what I'm going to do - namespace the signals (which will be much more elegant in this design than it was in my original design)

piegamesde commented 3 years ago

That's pretty much it, yes. And namespacing may indeed be the best solution for this.

idanarye commented 3 years ago

Speaking of me being fixtated about things - I was also being fixtated on using the Actix Context for the routing even when we use messages just to enforce same-threadness. But if I trust the users (and send_wrapper) to not go multithreaded here, and use the Recipient instead, I can allow the users create the actors themselves and just pass WoAB the Recipient!

So, the full sytnax will look something like that:

factories.win_app.instantiate()
    .tag(my_tag) // this, of course, is optional
    .connect(|bld| {
        bld.connect(Actor1 {
            widgets: bld.widgets().unwrap(),
            ...
        }.start().receipient());
        bld.connect_ns("actor2", Actor2 {
            widgets: bld.widgets().unwrap(),
            ...
        }.start().receipient());
        bld.connect_ns("actor3", Actor3 {
            widgets: bld.widgets().unwrap(),
            ...
        }.start().receipient());
    });

I'll also add a connect_to sugar that skips the closure:

factories.win_app.instantiate().connect_to(Actor1.new().start().receipient());

Though I dobut it'll get used that much, considering how one usually wants the widgets and would have to write:

let bld = factories.win_app.instantiate();
let actor1 = Actor1 {
    widgets: bld.widgets().unwrap(),
    ...
}.start();
bld.connect_to(actor1.receipient());

And at this point it's easier to just use the closure. But this sugar is easy enough to add so why not?

BTW - note that here I have a single tag for the entire instantiation - this is a "downgrade" from the current implementation that allows giving each actor a different tag. Since the purpose of the tag is to identify the instantiation, I don't think this option will be missed and considering how simpler it makes the implementation in this case I say it's worth it.

One thing we still have to decide here is what to do with namespacing errors. It's actually multiple things:

  1. What happens if we get a signal that's not in any namespace?
  2. What happens if the user registers multiple actors on the same namespace?
  3. Do we want to support the "empty" namespace "::signal_name"? Would it be considered the same as "signal_name" or a different namespace?
  4. Speaking of the default/empty namespace - do we enforce its usage?
  5. Do we return an error or just panic?
  6. Do we support nested namepsaces? ("ns1::ns2::ns3::signal_name")
  7. Do we strip the namespace from the signal we pass to the handler?

What I think (but can be persuaded otherwise) is:

  1. If a signal is not in any registered namespace - just pass it to the default namespace's actor. This will allow actors that do more complex routing.
  2. Error. Not sure why I even wrote it in the list - it should be obvious...
  3. For simplicity - yes. But it'll make a different for 7.
  4. No - one can skip it. But if you skip it and there are namespaceless signals in the builder (or signals that don't match any namespace) - that's an error.
  5. I think panic. These really are coding errors, and we already panic on similar errors.
  6. Yes and no. We can allow them but not do anything special with them - the actors that get them can use them for farther routing.
  7. This is the only one I'm 50-50 about. On the one hand, not stripping the namespace is good for greppability. On the other hand, if we add a macro we'll have to tell it what the namespace is. Then again - if the macro knows what the namespace is maybe it could pass it to connect? So let's not strip them.
piegamesde commented 3 years ago

I don't know much about Actix, so I can't judge if Recipient is good or not.

Regarding name spaces:

  1. All WoAB actors have a namespace. Either error or ignore it.
  2. Error. There's no need to do such things, and again this is a use case in which the Builder should be split.
  3. See 1.
  4. See 1.
  5. Panic on programmer errors
  6. The namespace is an opaque prefix. It may be "nested", but we shouldn't care.
  7. Same. I'm slightly in favor of stripping.

My idea is that signals look like "ActorName::method_name". You can easily grep ActorName to find out the target. It has the additional advantage to map well to associated methods (I still hope for that macro ^^). If there are multiple actors with the same name in different modules, use module_name::ActorName::method_name. Maybe it's easier for now to simply always use the absolute path. It adds a bit of boilerplate to the XML file, but given the usual flat module hierarchy in Rust programs it would be fine for me.

idanarye commented 3 years ago

I don't know much about Actix, so I can't judge if Recipient is good or not.

Recipient are like Addr, except they only accept one message type. Since we are converging to a single message type (woab::Signal) it'll be simpler to use that than Addr.

Is the namespace part of the Actor instance or part of the Actor type? I think we can do the latter, because every use case that I can think of which requires the former would be better off splitting the builder file …

If the namespace is part of the actor type, we'll have to add another trait just for specifying the namespace. It'll be easy with macros, but since we are initially going for a macroless signals and only add the macros later I don't want to burden people with that trait.

Then again - once we add a macro we may want to add a trait that'll also set the namespace when connecting.

The connection syntax looks fine, please check if it allows to easily create actors that cross-reference each other.

.connect(|bld| {
    let actor1 = Actor1::create(|ctx| {
        let actor2 = Actor2 {
            widgets: bld.widgets().unwrap(),
            actor1: actor1.clone(),
        }.start();
        bld.connect(actor2.receipient());
        Actor1 {
            widgets: bld.widgets().unwrap(),
            actor2: ctx.address(),
        }
    });
    bld.connect(actor1.receipient());
});

All WoAB actors have a namespace. Either error or ignore it.

This will mess things up for both the simple usecase (each instantiation routes to one actor) and to the routing actor idea which I'm not ready to give up just yet.

My idea is that signals look like "ActorName::method_name". You can easily grep ActorName to find out the target. It has the additional advantage to map well to associated methods (I still hope for that macro ^^). If there are multiple actors with the same name in different modules, use module_name::ActorName::method_name. Maybe it's easier for now to simply always use the absolute path. It adds a bit of boilerplate to the XML file, but given the usual flat module hierarchy in Rust programs it would be fine for me.

This means we have to use std::any::type_name. I'm not sure I like this - the docs do say "diagnostic use" and "best effort" - but than again, if we clean up the string and forbid generics we can probably get something consistent and reliable.

According to the docs, type_name does not guarantee to return the namespace (or maybe I got it wrong and it just doesn't gurantee to return the namespace for types in the current namespace?) so if we do go down that path we are going to have to use only the unqualified actor name, and panic on collisions. If you must have a collision you'll have to specify the namespace manually.

At any rate, I want to store Receipients internally so the base method will accept namespace+Recipient and we'll have a helper method that accepts and Addr and extrat the namespace and Receipient from it.

idanarye commented 3 years ago

@piegamesde: See #20 for basic implementation. Specifically https://github.com/idanarye/woab/pull/20/files#diff-30bd9eb64a60552a9b1d360b896fec233b67da63d6d28c9f7fe8b7275e840f32

idanarye commented 3 years ago

Actually, with this style I think the routing actor solution works really well:

factories.win_app.instantiate().connect_to(
    woab::SignalRouter::new()
    .route("actor1", Actor1 { ... }.start().receipient())
    .route("actor2", Actor2 { ... }.start().receipient())
    .route("actor3", Actor3 { ... }.start().receipient())
    .start().receipient()
);
piegamesde commented 3 years ago

Looks great so far! Your code snippet above looks a bit weird though, I'm not sure I fully understand it.

idanarye commented 3 years ago

woab::SignalRouter is an actor. Before starting it, we are registering all the real actors with .route() calls. Then we start it, and pass it to connect_to(), so as far as the builder considers - it sends all the GTK signals from that builder to a single actor. When SignalRouter gets a woab::Signal, it checks its namespace and passes it to the appropriate actor.

Now that I think about it, though, SignalRouter will have to search the string for :: every single time - which seems a bit wasteful. So maybe I should keep the routing at signal connection?

New idea - I'll add a new trait:

pub trait RouteBuilderSignal {
    fn route_builder_signal(bld: BuilderSignalsRoutingContext, signal: &str) -> RawSignalCallback
}

(I'll have to push the tag in there. I'll see how to do that...)

BuilderConnector.connect_to will accept values that implement it, and call route_builder_signal for every signal name. I'll implement it for Recipient<woab::Signal> and also for Addrs of actors that handle woab::Signal (so that you won't have to call recipient() every time), but I'll also implement it for the new woab::SignalRouter which will not be an actor and be used like this:

factories.win_app.instantiate().connect_to(
    woab::SignalRouter::new()
    .route("actor1", Actor1 { ... }.start())
    .route("actor2", Actor2 { ... }.start())
    .route("actor3", Actor3 { ... }.start())
);

Actually - since we are separating the multi-routing from regular single-routing, we can just go ahead and make the auto-namespace the default behavior:

factories.win_app.instantiate().connect_to(
    woab::SignalRouter::new()
    .route(Actor1 { ... }.start())
    .route(Actor2 { ... }.start())
    .route_ns("Actor3NeedsADifferentNamespace", Actor3 { ... }.start())
);
idanarye commented 3 years ago

Regarding tags - originally I thought to put them on the BuilderConnector, but since I'm introducing a new trait I think the simplest approach would be to piggyback on it. So, if Receipent is a tagless (the tag is ()), RouteBuilderSignal, (Tag, Receipent) would be a tagged RouteBuilderSignal.

SignalRouter can also be tagged:

factories.win_app.instantiate().connect_to(
    woab::SignalRouter::new()
    .tag(tag_value)
    .route(...)
);
idanarye commented 3 years ago

Problem: apparently a remove signal can be called from within another signal. This is a problem, because:

For now, I'm solving this by detecting this scenario (nested block_on) and when it happens I just do a non-blocking do_send and return None. This works for remove, but I'm not sure there aren't more complex signals that can be triggered inside signal handlers, and require inhibitness value...

piegamesde commented 3 years ago

That's interesting, I didn't think this was possible. On the one hand, every signal can trigger any other signal in its handler. However, normally, an event is simply added to the GDK event queue – which will be processed after our current handler finishes. Nested event handlers running is something I didn't know was possible, even from a plain GTK point of view.

idanarye commented 3 years ago

Done in #20.

piegamesde commented 3 years ago

Regarding the nested runtimes: I asked the devs and they told me that signals are synchronous, i.e. they eagerly execute all the callbacks. Therefore, it is obviously possible to create nested callbacks from an Actix perspective.

Now that I think of this, the order of events indeed does matter. And while it is a bit of a side effect, it is a good thing that WoAB now has the same semantics regarding this after the overhaul. It probably saved us from a few bugs that we hadn't run into yet.

However, this is not how Actix works. Messages are processed in a more lazy fashion. And users know this, and may make use of this. And we must be really, really careful to never mix both styles and create behaviour that was not intended. I just spent half an hour failing to find an example where things break, but expect me to succeed sooner or later (albeit accidentally) …

idanarye commented 3 years ago

Because of how I had to implement this, WoAB will process even the synchronous GTK asynchronously. The main problem with this approach is that I can't get the return value, but the only case I encountered where this actually happened was for the remove signal, which does not expect an inhibit decision.

I did some experimenting of my own, and apparently the event signal can also get emitted from inside another handler - and this one does require an inhibit decision, so currently WoAB cannot support it. I'm not sure if this is a real life usecase though...

I'm starting to think that the only signals that need to return an inhibit decision are GDK event signals (the only exception I encountered is draw, which is not getting called from inside another signal). So maybe I can always return Some(false) if the second parameter is a gdk::Event?

idanarye commented 3 years ago

Let us continue this discussion in #23.

piegamesde commented 3 years ago

I have trouble creating two actors from one builder that cross-reference each other. Can you please add an example for this?

idanarye commented 3 years ago

This is an Actix issue, not a WoAB issue, so I'm not sure if I should include an example in the code. But it should go something like this:

let bld = factories.specific_factory.instantiate();
Actor1::create(|ctx1| {
    let actor2 = Actor2::create(|ctx2| {
        bld.connect_to(woab::NamespacedSignalRouter::new()
            .route(ctx1.address())
            .route(ctx2.address())
        );
        Actor2 {
            widgets: bld.widgets().unwrap(),
            actor1: ctx1.address(),
        }
    })
    Actor1 {
        widgets: bld.widgets().unwrap(),
        actor2,
    }
})

Or - if you are feeling hacky - you can do it like this:

let (_, rx) = actix::dev::channel::channel(16);
let ctx1 = Context::<Actor1>::with_receiver(rx);

let (_, rx) = actix::dev::channel::channel(16);
let ctx2 = Context::<Actor2>::with_receiver(rx);

let bld = factories.specific_factory.instantiate();
bld.connect_to(woab::NamespacedSignalRouter::new()
    .route(ctx1.address())
    .route(ctx2.address())
);

let actor1 = Actor1 {
    widgets: bld.widgets().unwrap(),
    actor2: ctx2.address(),
}

let actor2 = Actor2 {
    widgets: bld.widgets().unwrap(),
    actor1: ctx1.address(),
}

ctx1.run(actor1);
ctx2.run(actor2);
piegamesde commented 3 years ago

I tried the non-hacky solution and it does not work: bld.connect_to moves bld into the inner closure and I have no good way to get it back.

idanarye commented 3 years ago

bld.connect_to consumes the builder connector, and returns a BuilderConnectorWidgetsOnly which can be used for accessing the widgets but not for connecting signals. This is done to encode in the typesystem that after connect_to signals cannot be connected again.