[Idea] Signal trapping as alternative to `(spawn_)outside`

[idanarye]

One of WoAB's greatest inconveniences is having to run things outside the Tokio runtime because otherwise they would trigger signals that cannot be handled because the runtime is occupied. This is a problem not because of the extra syntax, but because:

1. The future we run outside must be an async move block, meaning everything it uses must be .clone()ed.
2. If we want to return to the actor after doing whatever we were doing outside, we need to use Actix' actor futures - which do the job but are quite cumbersome.

So I'm thinking about an alternative solution. The signals we are worried about are usually directly triggered from the GTK operations we would otherwise run outside. Maybe we changed an editable field and triggered the signal of its change? Maybe we removed something and triggered the signal of it's removal? In these cases, what if instead of doing the complicated woab::outside dance - we just "relax" the signal handling a bit?

The idea - allow the user to bypass signal handling, so that GTK would still generate the signals inside the Tokio runtime but WoAB will not try to crank it inside the signal handler.

[idanarye]

The basic syntax would be:

let _guard = woab::trapping_signals(|signal, ctx| Ok(match signal.name() {
    "signal_i_want_to_discard" => None,
    "signal_i_want_to_handle_asynchronously" => {
        ctx.queue();
        None
    }
    "signal_i_want_to_handle_differently" => {
        let woab::params! {
            _,
            param1: gtk::Type1,
            param2: gtk::Type2,
        } = signal.params()?;
        // Handle the signal here
        None
    }
    _ => signal.cant_handle()?,
}));
self.widgets.some_widget_that_crates_signals_when_changed.change();

So I can either not handle the signal (e.g. - if I change a textfield and don't want to trigger the same action that would be triggered if the user changes it), queue it for handling later (e.g. - if I change a textfield and don't care that the signal that validates it and changes the color runs asynchronously), or handle it manually on the spot (which is probably useless - but with this design it costs nothing to allow this and would be very tricky to disallow it)

This decision of what to do with the signal is placed in a guard - so once it's dropped signal handling returns to normal. If it's not dropped before going back to the event loop bridge code, we'll should panic!.

Of course - using this is usually cumbersome, so we can have wrappers like woab::queuing_signals and woab::discarding_signals that queue/discard all signals, returning None as the inhibit decision. This should satisfy the common cases, where you suspect exactly what signals your code is going to trigger.

[piegamesde]

Hm, interesting idea. I'd like to drop the "handle manually" use case (we already have signal handlers for this) and the "drop signal" use case (GTK has signal inhibitors that do this just fine). This allows us to concentrate on better syntax for the common case, the queuing.

The thing is, wouldn't it be possible to replace the "cannot trigger GTK signal within woab engine" panic with an automatic queue all of the time? Or maybe have a simple call wrapper/marco to make it opt-in for the sake of explicitness.

[idanarye]

Hm, interesting idea. I'd like to drop the "handle manually" use case (we already have signal handlers for this) and the "drop signal" use case (GTK has signal inhibitors that do this just fine). This allows us to concentrate on better syntax for the common case, the queuing.

The underlying mechanism I had in mind easily allows all usecases, so why not have them all?

The thing is, wouldn't it be possible to replace the "cannot trigger GTK signal within woab engine" panic with an automatic queue all of the time?

23

Or maybe have a simple call wrapper/marco to make it opt-in for the sake of explicitness.

That would be the woab::queuing_signals I've mentioned.

[piegamesde]

so why not have them all

As long as the use case that we require does not suffer from it, I don't care.

Bikeshed: how about woab::delay_events?

Usage example of what I'm thinking of:

woab::delay_events(|| {
    for page in &carousel.get_children()[new_pages..old_pages] {
        carousel.remove(page);
    }
});

The code runs instantly, but all signals triggered within that closure won't cause a panic, but instead be queued for later dispatch. I think that makes the semantics rather clear when reading the code. What do you think? (syntax is open for bike shedding)

[idanarye]

So we need to decide on using a guard vs using a closure? I prefer guards because they allow more freedom - you don't need to introduce a new scope. Then again, they are a bit less intuitive...

I'm also starting to seriously consider using unsafe and implementing my own connect_signals. The gtk-rs implementation gets the signals' real name and a handle to the object, meaning I can use g_signal_query to get the signal's return type and parameters. Then we could have a simple heuristic:

1. If there is a return type, the signal cannot be handled asynchronously.
2. If there is a Context argument (cairo context? GLContext and pango context too? Do we need a list? Maybe just check if the type name ends with the string "Context"?), the signal cannot be handled asynchronously.
   • Maybe its enough to check the second argument?

Then, when a signal is triggered from inside the Tokio runtime, we automatically queue all the signals that can be handled asynchronously and fail the ones who can't with an appropriate error message.

Actually - maybe we don't even need the unsafe? The second criteria can be checked when we get the signal, and as for the first - we can queue the signal anyway, and check later if it returned an inhibit decision. If so - we panic!. GTK will print an error before that because the signal handler did not return the expected type, but at least WoAB can add it's own error message with the actual failure reason.

[piegamesde]

The problem I have with your guard proposal is that it reminds me of one of the earlier attempts to handle inhibit values. The problem with it that it was non-local to the other signal handling. Having some signals have code at two places was really irritating and made the code harder to read and maintain.

I think we could make it just work with some heuristics based on the signal type as you suggest. But my fear is that automagically altering behavior will result in some unintuitive and frustrating bugs if a user runs into an edge case where the magic turns out to not work right for them.

That's why I prefer making it explicit at the call site, at the expense of some boilerplate in the code.

[idanarye]

Maybe I should have been more clear - I'm not talking about branch guards. I'm talking about RAII guards - using a dummy variable to hold a value until the end of the scope so we can run some code when it's dropped.

The closure with the big match has nothing to do with this aspect of the design - we could have it even if we go with the closure style:

woab::trap_signals(
    |signal, ctx| Ok(match signal.name() {
        "signal_i_want_to_discard" => None,
        "signal_i_want_to_handle_asynchronously" => {
            ctx.queue();
            None
        }
        "signal_i_want_to_handle_differently" => {
            let woab::params! {
                _,
                param1: gtk::Type1,
                param2: gtk::Type2,
            } = signal.params()?;
            // Handle the signal here
            None
        }
        _ => signal.cant_handle()?,
    }),
    || {
        for page in &carousel.get_children()[new_pages..old_pages] {
            carousel.remove(page);
        }
    });

(Admittedly this does look terrible compared to the guard style trap_signals...)

And the guard style can also be used without the closure-with-a-match, automatically delaying all the signals:

let _guard = woab::delay_signals();
for page in &carousel.get_children()[new_pages..old_pages] {
    carousel.remove(page);
}

BTW, some more bikeshedding: I don't like the word "delay" in this context, because it implies pushing the signal forward by some measurable amount of time. We don't care about the temporal position of the signal handling - we care about the order. That's why I think the word "queue" is more appropriate.

[piegamesde]

The let _guard = woab::delay_signals(); is short and clean, but I don't think that the RAII guard pattern applies well here. Normally, the guard has a purpose, as in one needs to have it in order to interact with some kind of resource. But in our case, its presence would merely be for the side effects, since all interaction is done through other GTK methods.

Maybe we could make signal trapping the default and have our special code path for the inverse behaviour (need to think about a good name on this).

Regarding the automagic approach, how about this: If we're in a woab context (i.e. already in a signal handler), the signals are queued by default, otherwise they are executed immediately. Signals that cannot be queued (that have a return value) continue to throw a panic. This will give us (hopefully) sane defaults, but there must still be a mechanism to get manual control just in case (for example to enforce that draw signals are handled correctly).

[idanarye]

I'm all for queuing as the default, but I'm not sure how useful it'll be to allow overriding this inside a signal handler. Think how to overriding-draw-handling usecase is going to look like:

1. You notice weird artifacts in your canvas.
2. You spend a few hours trying to figure what you're doing wrong with your own debug code.
3. You vaguely remember WoAB listed a relevant pitfall, so you go back to the readme/docs to find out you have an queued draw event.
4. You carefully reread all your signal handlers code and all the methods it calls, trying to find the place that triggers the draw.
5. Once you find it, you try to implement your draw handler, without the data in the actor. You can't - so you clone() all that data and re-implement your draw method inside that other signal handler.
6. Artifact still happening. Go back to step 4 and repeat.

Not fun.

So I'm really going to go with that context detection thing - if the signal has a Context argument, it won't be queued and you'll fail early instead. This should be rare enough:

• Draws usually either come from outside and explicitly requested draws are queued anyway.
• WidgetExt::draw probably invokes the signal directly - but since you are providing the context, just draw directly to the context instead of reimplementing a signal handler that draws to it. The borrow checker will be more friendly that way.
• Even if there is some case that I missed - using woab::outside()/woab::spawn_outside() is still easier than reimplementing the draw.