Skip to content
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

Add glib::signals! macro #1577

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

PJungkamp
Copy link
Contributor

@PJungkamp PJungkamp commented Nov 22, 2024

This is my initial implementation of the ideas from #214 (comment) and currently only creates an implementation of a DerivedObjectSignals trait modeled after the DerivedObjectProperties used by the Properties macro. It needs some cleanup (a couple of unused elements/fields/imports) and it's also missing the generator for the connect_ and emit_ functions.

The basics seem to work. But I'm now wondering how exactly signal class handlers work. Is the class handler function invoked if none of run_first, run_last or run_cleanup are specified?

#[glib::derived_properties]
impl ObjectImpl for Author {
fn signals() -> &'static [Signal] {
static SIGNALS: OnceLock<Vec<Signal>> = OnceLock::new();
SIGNALS.get_or_init(|| vec![Signal::builder("awarded").build()])
Self::derived_signals()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to add a derived_signals macro as well, not much work and would make the migration to use the future signals macro more appealing

@bilelmoussaoui
Copy link
Member

missing the generator for the connect_ and emit_ functions.

For the connect_ functions, you would have to use ObjectExt::connect_closure and create a glib::closure!, pass the params the signal function signature got to the created closure. A local variant of the connect_ function should also be generated using glib::closure_local!

Something that I haven't seen any note about is the signal details, not sure how to handle it.

impl Author {

#[signal]
fn awarded(&self) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be possible to make

Suggested change
fn awarded(&self) {}
fn awarded(&self);

and that means that the signal has no class handler, and

Suggested change
fn awarded(&self) {}
fn awarded(&self) { println!("stuff"); }

which means that it has a class handler with the body of the function.

In the first case, the macro would have to remove the whole function after expansion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I almost never use class handlers in my signals so I would appreciate something like that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A function without a block isn't valid rust syntax outside of an extern block. I don't want to add some invalid Syntax that would be removed by the macro.

It's also way more complicated to parse this. I would have to write a custom parser based on syn::ItemImpl that accepts these "invalid" functions. And it confuses other parsing code for other macros or rustfmt.

I can add and parse a toplevel pseudo-macro, meaning something like this would be ok:

#[glib::signals(wrapper_type = super::FooObject)]
impl FooObject {
    signals! {
        fn awarded() -> i32;
    }
}

This pseudo-macro is valid Rust Syntax and should work with rustfmt and other macros.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of using an impl Object, the usage could be trait ObjectSignals making it possible to provide a default impl?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default implementations can only use methods provided by the trait's required subtraits and itself.

The class handler implementations would have to reside in an impl ObjectSignals for FooObject block.

I don't really see how I'd map a trait to a definition of signals, as I don't want the macro to modify the trait in ways that make it hard for a developer to understand what the trait will actually look like.

The problem I see is that a semicolon instead of a block in a trait definition means that you have to provide the class handler in your implementation of that trait, while you want it to mean that there is no class handler.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trait itself wouldn't be used. You can then impl T for SomeObject and forward the calls when the trait has an impl, no?

Anyways, zbus is using something similar for it zbus::proxy! macro.

Copy link
Contributor Author

@PJungkamp PJungkamp Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an open issue around #[zbus::proxy] eating the trait definition.
I don't want to introduce these weird macro-related edge cases into glib.

What do you think about this?

#[glib::signals(wrapper_type = super::FooObject)]
trait FooObjectSignals {
    fn simple_signal(&self);
    
    fn complex_signal_without_class_handler(&self) -> i32;
    
    #[signal(class_handler, run_first]
    fn complex_signal_with_class_handler(&self, param: i32) -> i32;
}

impl FooObjectSignals for FooObject {
    fn complex_signal_with_class_handler(&self, param: i32) -> i32 {
        10
    }
}

I would make the macro produce something like:

trait FooObjectSignals: glib::subclass::types::ObjectSubclass<Type = super::FooObject> {
    // These functions have to be removed from the trait since they doesn't have a class handler one could implement:
    //     simple_signal(&self); 
    //     complex_signal_without_class_handler(&self) -> i32;
    // Providing a function body for signals without class handlers is forbidden. 
    
    // This function is allowed to have a default implementation but does not need to have one.    
    fn complex_signal_with_class_handler(&self) -> i32;
    
    #[doc(hidden)]
    fn __derived_signals() {
        static SIGNALS: std::sync::OnceLock<[glib::subclass::signal::Signal; 3]> = std::sync::OnceLock::new();
        SIGNALS.get_or_init(|| [
            glib::subclass::signal::Signal::builder("simple_signal").build(),
            glib::subclass::signal::Signal::builder("complex_signal_without_class_handler")
                .param_types([<i32 as glib::types::StaticType>::static_type()])
                .return_type::<i32>()
                .build(),
            glib::subclass::signal::Signal::builder("complex_signal_with_class_handler")
                .param_types([<i32 as glib::types::StaticType>::static_type()])
                .return_type::<i32>()
                .class_handler(|params| {
                    Some(<i32 as glib::value::ToValue>::to_value(
                        &Self::complex_signal_with_class_handler(
                            glib::subclass::types::ObjectSubclassExt::from_obj(&params[0].get::<Self::Type>().unwrap()),
                            params[1].get::<i32>().unwrap(),
                        )
                    )
                })
                .build(),
        ])
    }
}

A #[glib::derived_signals] macro could then be used to insert the __derived_signals function call into the ObjectImpl::signals function.

#[glib::derived_signals]
impl ObjectImpl for FooObject {}

@sdroege
Copy link
Member

sdroege commented Nov 24, 2024

For the connect_ functions, you would have to use ObjectExt::connect_closure and create a glib::closure!, pass the params the signal function signature got to the created closure. A local variant of the connect_ function should also be generated using glib::closure_local!

This should only be done for non-Sync objects though. I think this will need to be configurable, can't really auto-detect that.

@sdroege
Copy link
Member

sdroege commented Nov 24, 2024

But I'm now wondering how exactly signal class handlers work. Is the class handler function invoked if none of run_first, run_last or run_cleanup are specified?

It's not called at all AFAICS (see signal_emit_unlocked_R() in glib/gobject/gsignal.c).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants