-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Brainstorm] Signal handling overhaul #15
Comments
The simplest solution is to just have the 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. |
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 |
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:
The problem are:
|
A variant of the 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 So I don't like that idea that much, but this is a brainstorm so I'm writing all my ideas. |
Speaking of "all my ideas" - we don't have to switch from streams to messages. We can also pass a |
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.
|
Regarding the other ideas:
|
Why would we need that trait? The macro is just going to generate a signal handler that accepts a new Actually, one thing I'll need to put on a trait is the list of supported signals.
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. |
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. |
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 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 |
We still need to support connecting builderless signals, but we probably don't need let router = WindowActor::route_to(ctx);
router.connect(&action1, "activate", "Action1").unwrap();
router.connect(&action2, "activate", "Action2").unwrap();
router.connect(&action3, "activate", "Action3").unwrap(); |
It turns out that |
I think there may be some tricks around this. An obvious solution would be to use 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 |
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 than 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()
}
}
} |
That's basically the same breakdown I did in #19. We agreed on the Message and return type of the |
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 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) |
That's pretty much it, yes. And namespacing may indeed be the best solution for this. |
Speaking of me being fixtated about things - I was also being fixtated on using the Actix 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 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:
What I think (but can be persuaded otherwise) is:
|
I don't know much about Actix, so I can't judge if
Regarding name spaces:
My idea is that signals look like |
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.
.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());
});
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.
This means we have to use According to the docs, At any rate, I want to store |
@piegamesde: See #20 for basic implementation. Specifically https://github.com/idanarye/woab/pull/20/files#diff-30bd9eb64a60552a9b1d360b896fec233b67da63d6d28c9f7fe8b7275e840f32 |
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()
); |
Looks great so far! Your code snippet above looks a bit weird though, I'm not sure I fully understand it. |
Now that I think about it, though, 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...)
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())
); |
Regarding tags - originally I thought to put them on the
factories.win_app.instantiate().connect_to(
woab::SignalRouter::new()
.tag(tag_value)
.route(...)
); |
Problem: apparently a
For now, I'm solving this by detecting this scenario (nested |
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. |
Done in #20. |
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) … |
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 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 |
Let us continue this discussion in #23. |
I have trouble creating two actors from one builder that cross-reference each other. Can you please add an example for this? |
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); |
I tried the non-hacky solution and it does not work: |
|
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:
draw
)The answer to both these problems is a new flow:
actix::Message
(instead of using a Tokio stream)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.
The text was updated successfully, but these errors were encountered: