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

the trait bound `gdk::EventWindowState: glib::value::FromValueOptional<'_>` is not satisfied #2

Closed piegamesde closed 3 years ago

piegamesde commented 3 years ago

I'm trying to connect to the window_state_event, but I can't add gdk::EventWindowState to my enum variant because it doesn't implement glib::value::FromValueOptional. The error originates within the woab::BuilderSignal macro.

idanarye commented 3 years ago

OK - so apparently unlike most of the parameters passed in the signals, gdk::EventWindowState is not a glib::Value. gdk::Event is a glib::Value, and you can downcast gdk::Event to specific event types. So - we have three options:

  1. Just use gdk::Event in the variant and downcast it on usage.
  2. I can add syntax for making WoAB do the downcasting - something like:
    WindowState(gtk::Window, #[signal(event)] gdk::EventWindowState),
  3. I can add more complex syntax
    WindowState(
        gtk::Window,
        #[signal(convert = |arg| arg.get::<gdk::Event>()??.downcast()?)]
        gdk::EventWindowState
    ),

I lean toward the second option. While the third option is more flexible, I is much more verbose and much less elegant and I'm not sure we need that flexibility.

Another thing I noticed is that window-state-event must return a boolean - unlike other signals that must return nothing. The return value cannot be decided in the handler, because the GTK signal handler is not async, but luckily it only determines if the signal should stop in that handler and not be propagated farther - and there is no reason why we can't decide that statically. So the final syntax should look something like:

#[signal(return = false)]
WindowState(gtk::Window, #[signal(event)] gdk::EventWindowState),
piegamesde commented 3 years ago

I don't fully understand the inheritance relationship. Why should a subclass not be of a superclass? Also, do I assume correctly that the first solution would work without further code changes if it weren't for the return type?

Regarding the return type: I've looked at my existing code (I want to migrate) and I found the following return types: (), bool, Continue, Inhibit. I don't think it's reasonable to make general assumptions about the return type of callbacks. The next problem is that the return value is not static: I do have at least two callbacks that don't have a constant return value.

The obvious problem with dynamic return values is that the current "match on event enum" approach does not type. Maybe we need something function-based, like suggested in https://github.com/gtk-rs/gtk-rs/issues/128 ? I think maybe what I need right now is a "less beautiful but more powerful" solution to bridge my callbacks into Actix, that even works on actions defined outside of any UI file. It must be powerful to cover all use cases, and then we can try to keep the common patterns fast with specialized solutions like woab::BuilderSignal.

idanarye commented 3 years ago

I've pushed a commit that fixes this. Can you check if it works for you? You'll have to tell Cargo to get it from GitHub because I'm not doing a release yet - I prefer to do a few more iterations with you and catch other edge cases before I do 0.2.

The syntax is:

#[signal(ret = false)]
WindowState(gtk::Window, #[signal(event)] gdk::EventWindowState),

I used ret instead of return because return is a keyword and I do a little hacky parsing to allow complex expressions so I can't use keywords for attribute parameter names.

I don't fully understand the inheritance relationship. Why should a subclass not be of a superclass?

Because they are not really "subclasses". Maybe they are in GTK, but not it Rust. woab:BuilderSignal is getting a vector of glib::Value which can be directly casted to numbers, strings, booleans etc. It can also be directly casted to gdk::Event. All of this is done with it's get function, which does not support the specific event types. To do that, I must first convert it to gdk::Event and then use a different fucntion - downcast - to do that final conversion.

Also, do I assume correctly that the first solution would work without further code changes if it weren't for the return type?

Yes, but I've already pushed a commit that fixes both the return type issue and the second casting issue, so you can use the second solution.

The obvious problem with dynamic return values is that the current "match on event enum" approach does not type.

Actually, no. All the signals handlers are returning Option<glib::Value> (it gets translated to the concrete type somewhere in GTK) so I could have just made handle return that, if that was the problem.

But it isn't.

The actual problem is that I need to return the result before the StreamHandler can get the signal. This is because the signal handler function that GTK accepts is not an async function, so it can't yield execution back to the Tokio runtime, and Tokio can't give that execution to Actix to run StreamHandler::handle because it needs to run in the same thread as GTK.

Maybe we can work something out with cells...

piegamesde commented 3 years ago

I copied your code, it panics with thread 'main' panicked at 'Signal 'window-state-event' of type 'HdyApplicationWindow' required return value of type 'gboolean' but got None'. Not sure if this is a mistake on my side or not though. An implementation note: I'd really appreciate it if the gtk-rs wrapper types like gtk::Inhibit(false) and Continue would still work within the macro.

All of this is done with its get function, which does not support the specific event types

I've asked around and this seems to be a bindings-specific issue. We could implement FromValueOptional for the event types and then wouldn't need that part of the macro extension.

The actual problem is that I need to return the result before the StreamHandler can get the signal.

I see. I've looked at my code and the part that determines the return value is independent of the current state most of the time. If we had a way of sending new signals from code within a signal handler, this would solve the problem.

idanarye commented 3 years ago

I copied your code, it panics with thread 'main' panicked at 'Signal 'window-state-event' of type 'HdyApplicationWindow' required return value of type 'gboolean' but got None'. Not sure if this is a mistake on my side or not though.

Can you show me the code? Because a similar signal works in the new example I've added: https://github.com/idanarye/woab/blob/master/examples/example_events.rs#L49

An implementation note: I'd really appreciate it if the gtk-rs wrapper types like gtk::Inhibit(false) and Continue would still work within the macro.

Can you give an example for such a signal? And how would you expect the syntax to look like?

I've asked around and this seems to be a bindings-specific issue. We could implement FromValueOptional for the event types and then wouldn't need that part of the macro extension.

That's for gtk-rs - I can't implement a foreign trait for a foreign type...

If we had a way of sending new signals from code within a signal handler, this would solve the problem.

You lost me. You mean sending a GTK signal from the StreamHandler that handles the signals? How would that help?

piegamesde commented 3 years ago

Can you show me the code?

I'll try to debug and minimize it first.

Can you give an example for such a signal?

Sure. Generally, all signals that are part of input handling have events that bubble up and down the UI tree. At each stage, handlers can inhibit further propagation. Examples: connect_key_press_event, connect_draw. Continue is much less common, it is used with glib channels and idle callbacks. Examples: idle_add, attach.

And how would you expect the syntax to look like?

#[signal(ret = gtk::Inhibit(false))]

That's for gtk-rs - I can't implement a foreign trait for a foreign type...

Well yes, that change must be made upstream (or on a local copy for now)

You lost me.

Yeah, sorry. Let me try again. For example, I have a callback like this, where next and previous are some actions with their own callback on connect_activate:

carousel.connect_key_press_event(
    clone!(@weak next, @weak previous => @default-panic, move |_carousel, event| {
        use gdk::keys::constants;
        match event.get_keyval() {
            constants::Right | constants::KP_Right | constants::space | constants::KP_Space => {
                next.activate(None);
                gtk::Inhibit(true)
            },
            constants::Left | constants::KP_Left | constants::BackSpace => {
                previous.activate(None);
                gtk::Inhibit(true)
            }
            _ => gtk::Inhibit(false)
        }
    }),
);

In this case, the return value is dependent on the input but not on the current state. I can't use WoAB for handling this signal, but I can move as much as possible of it into another signal that has no return value and thus can be bridged by WoAB. I then need a mechanism to trigger that signal from within my key-press-event callback. Moreover, I need to connect the activate signal from my action as well; I don't think there is a way for it at the moment?

idanarye commented 3 years ago

I think gtk::Inhibit is a gtk-rs construct used to enforce some type strictness. The GTK key-press-event signal is using raw boolean (gboolean actually)

Had gtk::Inhibit implemented ToValue this would not be a problem, but since it doesn't, to support #[signal(ret = gtk::Inhibit(false))] I'd have to either make the macro recognize gtk::Inhibit and extract its internal value, or make ret support only gtk::Inhibit. Which may not be so bad - if we can assume all signals either return no value or return a inhibitness (which seems to be the case - but I would like to make sure this really is the rule). But in this case - wouldn't it be better to make the syntax #[signal(inhibit = false)]?

Regarding the dynamic return value - I was stuck on this problem because I was convincing myself that the syntax should be part of the signal enum and #[derive(woab::BuilderSignal)], and then it came to me - why not pass a closure to

factories.app.build().signals_inhibit(|signal| {
    match signal {
        AppSignal::KeyPressEvent(_, event) => {
            if custom_code_that_decide_if_should_inhibit(event) {
                Some(gtk::Inhibit(true)) // yea yea I know old meme
            } else {
                Some(gtk::Inhibit(false))
        }
        }
        _ => None // for most signals that don't need dynamic return types
    }
}).actor(|_, widgets| AppActor {
    widgets,
})?;

Of course, we'd still keep #[signal(inhibit = false)] to make things simpler for most signals that should not require this custom behavior.

The benefit of this style is that if we need some state we can pass a Rc<RefCell<...>> to the closure and pass another reference to the actor to allow some communication between them.

piegamesde commented 3 years ago

Can you show me the code?

I'll try to debug and minimize it first.

The crash happens in combination with my workaround for #3, so it can safely be ignored. Works as intended :tada:

piegamesde commented 3 years ago

Now that I think of it: would it be possible to add the inhibitor as a new method on the BuilderSignals trait? One could then implement the trait manually and override the default implementation that always returns None. The only gotcha is that I don't know if it is allowed to partially implement the trait multiple times (some methods in the proc macro, some methods manually in another impl).