Add proc macro for binding methods to action maps

[andy128k]

Continuation of gtk-rs/gtk3-rs#266
Closes #16

[GuillaumeGomez]

Before going any further: is it really worth it to create a whole new crate for this? Shouldn't we put it into glib-macros instead?

Also: please add error message tests for your macro (like there are in ./glib/clone_tests/clone_compilation_errors.py).

[bilelmoussaoui]

Before going any further: is it really worth it to create a whole new crate for this? Shouldn't we put it into glib-macros instead?

fyi, the actions are part of gio not glib

[GuillaumeGomez]

We can rename "glib-macros" into "gtk-rs-macros" or something along the line if you prefer? I'd simply prefer to avoid creating too many repositories.

[sdroege]

fyi, the actions are part of gio not glib

Don't we have a macro for GResource things somewhere? That would also be part of some kind of gio-macros crate then

[bilelmoussaoui]

fyi, the actions are part of gio not glib

Don't we have a macro for GResource things somewhere? That would also be part of some kind of gio-macros crate then

Indeed we do

[sdroege]

Ah but that's not a proc macro so irrelevant here.

We could put this into the glib-macros crate here, doesn't seem like much of a problem. gio already indirectly depends on it anyway, and glib-macros wouldn't have to depend on gio. And technically gio is a part of glib anyway.

[andy128k]

Moving this to glib-macros will make a circular dependency. Doc-tests depend on gio.

[sdroege]

On Sun, 2021-06-06 at 03:30 -0700, Andrey Kutejko wrote: Moving this to glib-macros will make a circular dependency. Doc-tests depend on gio.

Well that decides it then :)

[Hofer-Julian]

Is something blocking this PR at the moment?

[sdroege]

Is something blocking this PR at the moment?

Someone who knows well about actions to review if the way how the macro exposes them makes sense, and someone to review if adding this macro causes us composibility problems in the future.

[andy128k]

@Hofer-Julian You may try this macro with my crate.

[AaronErhardt]

I really like this solution. It has two weak points though, IMO.

First, using functions means that capturing variables is impossible. This makes this solution a lot less attractive for using it in apps built on top of Relm4.
Also, using actions for more than just one action widget isn't really considered in this approach. Of course you can still add actions to other actionable widget if you know their name but that isn't type safe.

My approach is more flexible and works without proc-macros but has some possible weak spots, as well. Maybe we can get the best out of both wolds 🤔

[andy128k]

@AaronErhardt

First, using functions means that capturing variables is impossible. This makes this solution a lot less attractive for using it in apps built on top of Relm4.

I never tried Relm (or Relm4), so cannot comment on this. But my macro is designed to mimic OOP methods (if you derive your widget) or "methods" (if your widget is just a composition or a newtype-wrapper). So, it captures self and that should be enough.

Also, using actions for more than just one action widget isn't really considered in this approach. Of course you can still add actions to other actionable widget if you know their name but that isn't type safe.

Isn't this a regular approach in Gtk-world? You place strings with action names (or action names and target values) here and there in a source code and .ui files, and they just work.

---

Regarding type safety of actions. Once I gave a try to such approach and defined all actions of my app as enums. I actually did not find it useful and got rid of it later. Maybe in a world of Relm(4) they behave better, I don't know.

---

It seems both of us agree on one thing: action has only one callback handler :) This simplifies its role to just a fancy alias to a callback handler. So, you may pass either a string or a type-safe value to widgets instead of an actual closure.

[jf2048]

@andy128k I'd like to see this updated with the latest bits from awesome-gtk (async stuff, lint fixes) and then I can review it, I've found this to be really useful

[jf2048]

Maybe put a note in here about the caveats with async actions? Especially as it relates to returning state changes, it seems like most of the time a stateful action shouldn't be async. You can put a note similar to this one I put in the template callbacks macro: https://github.com/gtk-rs/gtk4-rs/blob/86c1548cc2a6fa05c4285802f0607b374f3bec0f/gtk4-macros/src/lib.rs#L122-L131

[andy128k]

Why? Such actions work as expected. Returning a value is just a fancy way to invoke set_state.

[jf2048]

Right, but it is possible with async actions to have two or more invocations of the action get spawned. If a later future resolves quicker than an earlier one, that would end up calling set_state with a stale value. In a GTK widget that would be like pressing a button twice and seeing a widget update with the new value and the suddenly switching back to an old state.

To be totally free of races in stateful actions, it seems the app would have to do something like:

• Check the current state at the end of the async action, return None if it does not match the input state, or
• If the order is important, generate a unique ID at the beginning of the action and push it on a stack (a heap pointer would do), take it off the stack at the end and return None if it was not at the top
• Try to disable the action at the beginning of the async action, return None immediately if it's already disabled, then re-enable it at the end

[jf2048]

Instead of returning a Result and using to_compile_error, you can catch the error, pass it to proc_macro_error::emit_error and then generate an empty register method. I made the gtk4 macros do something like this and it works really well to reduce the error message if you have a nightly compiler.

[jf2048]

Also I think we may also want to accumulate more of these errors with combine instead of returning early from this function?

[jf2048]

A couple more things:

• I didn't bother tagging all of them because there's so many, but every usage of gio and glib inside a quote block should be fully qualified using proc-macro-crate. crate_ident_new can be copied from glib-macros. Then it can be #crate_ident for gio and #crate_ident::glib for glib.
• Does the #[allow(clippy)] bits actually do anything here? I thought those didn't have an effect inside macros but I could be wrong.

[jf2048]

Do we want to require an #[action] attribute on the method in order to make it an action? Without that attribute the macro would skip over that function. That's what I did with gtk::template_callbacks but not sure if it makes sense here?

[andy128k]

There are multiple examples of actions with and without an attribute in tests.

[jf2048]

Let me modify the example to show what I mean. Would it help to be like this:

#[gio::actions]
impl MyApplication {
    #[action]
    fn action1(&self) { /* this method gets added with add_action */ }
    fn my_method(&self) { /* not an action, just an ordinary method */ }
}

Then action1 becomes an action but my_method does not. Does that make sense? It would allow for putting multiple macros on the same block, like for a widget:

#[gio::actions]
#[gtk::template_callbacks]
impl MyWidget {
    #[action]
    fn action1(&self) { /* ... */ }
    #[template_callback]
    fn callback1(&self) { /* ... */ }
}

What do you think? Or does it make sense to enforce a separate impl block for each macro? I'm not sure.

[andy128k]

My opinion is that it is better to keep such blocks separated. Such macros may modify signatures of functions, generate new ones and step on each others feet any other way.

[jf2048]

Sounds good to me

[sdroege]

@bilelmoussaoui As you worked on actions API, what do you think of this considering the existing helpers you added for making this easier?

[andy128k]

Current API is still very verbose and repetitive. I gave it a try and that's how it looks like just for two actions.

            window
                .add_action_entries([
                    gio::ActionEntry::builder("find")
                        .activate(move |this: &Self::Type, _, _| {
                            let this = this.clone();
                            glib::MainContext::default().spawn_local(async move {
                                this.find().await;
                            });
                        })
                        .build(),
                    gio::ActionEntry::builder("copy")
                        .activate(move |this: &Self::Type, _, _| this.copy())
                        .build(),
                ])
                .unwrap();