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

Allow specifying class struct to glib_wrapper!() macro #234

Merged
merged 3 commits into from Nov 6, 2017

Conversation

Projects
None yet
4 participants
@federicomenaquintero
Contributor

federicomenaquintero commented Sep 14, 2017

This goes along with gtk-rs/gir#459

I'm not 100% sure if this works yet, as my gtk module doesn't build (but glib builds). Consider it as a proof of concept for now.

federicomenaquintero added some commits Sep 14, 2017

glib_wrapper!() - Specify ffi class in Object<ffi:Name, ffi:ClassName>
This is an API break for users of the glib_wrapper!() macro.

Now in the Wrapper trait we keep two associated types:

pub trait Wrapper {
    type GlibType: 'static;
    type GlibClassType: 'static;   // new one
}

This keeps the types of the instance struct and the class struct
together, so we can access them as needed.  We will need to access
both in the code generator for GObject boilerplate.

This is for #233
Allow specifying non-derivable classes in glib_wrapper!()
As before, we can have

    pub struct Foo(Object<ffi::Foo>);

in the macro call; this now means that there is no class struct
available.  To specify class structs,

    pub struct Foo(Object<ffi::Foo, ffi::FooClass>);
Show outdated Hide outdated src/wrapper.rs Outdated
Clarify the documentation for glib_wrapper!()
We now have examples of each use case.
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Sep 22, 2017

Member

While this PR still not uses GlibClassType I vote for merge it as is to fix block for future regens.
@GuillaumeGomez, @sdroege what you think about it?

Member

EPashkin commented Sep 22, 2017

While this PR still not uses GlibClassType I vote for merge it as is to fix block for future regens.
@GuillaumeGomez, @sdroege what you think about it?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Sep 22, 2017

Member

I would like it if there was a commit that makes glib_wrapper! also emit code like the following

#[repr(C)]
pub struct FooClass(ffi::BarFooClass);

impl IsA<ParentOfFooClass> for FooClass { }
impl IsA<ParentOfParentParentOfFooClass> for FooClass { }

impl Foo {
    pub fn get_class(&self) -> &FooClass {
        unsafe {
            let stash = self.to_glib_none();
            let ptr: ffi::Foo = stash.0;
            let class = *(ptr as *const *const ffi::BarFooClass);
            &*(class as *const FooClass)
        }
}

This would lead the way to having immutable class methods later (g_object_class_list_properties(), gst_element_class_get_pad_template(), etc). Same for mutable ones (to be called in class_init / base_init) would be something that could be added too then, but would only be useful for myself and the gnome-class macro :)

Member

sdroege commented Sep 22, 2017

I would like it if there was a commit that makes glib_wrapper! also emit code like the following

#[repr(C)]
pub struct FooClass(ffi::BarFooClass);

impl IsA<ParentOfFooClass> for FooClass { }
impl IsA<ParentOfParentParentOfFooClass> for FooClass { }

impl Foo {
    pub fn get_class(&self) -> &FooClass {
        unsafe {
            let stash = self.to_glib_none();
            let ptr: ffi::Foo = stash.0;
            let class = *(ptr as *const *const ffi::BarFooClass);
            &*(class as *const FooClass)
        }
}

This would lead the way to having immutable class methods later (g_object_class_list_properties(), gst_element_class_get_pad_template(), etc). Same for mutable ones (to be called in class_init / base_init) would be something that could be added too then, but would only be useful for myself and the gnome-class macro :)

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Sep 22, 2017

Member

Good idea

Member

EPashkin commented Sep 22, 2017

Good idea

@federicomenaquintero

This comment has been minimized.

Show comment
Hide comment
@federicomenaquintero

federicomenaquintero Nov 6, 2017

Contributor

Could we get this merged and postpone Sebastian's idea until we decide on the following?

As far as I can tell, generating pub struct FooClass requires something like the concat_idents!() macro, which is a nightly-only feature.

It may be possible to also have an internal procedural macro inside glib-rs itself that generates the FooClass identifier.

This is a bit more work than I'd like to put in just right now... maybe we can do this during the hackfest in Berlin?

Contributor

federicomenaquintero commented Nov 6, 2017

Could we get this merged and postpone Sebastian's idea until we decide on the following?

As far as I can tell, generating pub struct FooClass requires something like the concat_idents!() macro, which is a nightly-only feature.

It may be possible to also have an internal procedural macro inside glib-rs itself that generates the FooClass identifier.

This is a bit more work than I'd like to put in just right now... maybe we can do this during the hackfest in Berlin?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Nov 6, 2017

Member

👍 for merge this and related gir's PR as is.
This will need full Regen again, but all CI IMHO will pass.

Member

EPashkin commented Nov 6, 2017

👍 for merge this and related gir's PR as is.
This will need full Regen again, but all CI IMHO will pass.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Nov 6, 2017

Member

Fine by me. Thanks!

Member

GuillaumeGomez commented Nov 6, 2017

Fine by me. Thanks!

@GuillaumeGomez GuillaumeGomez merged commit a51dc67 into gtk-rs:master Nov 6, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@federicomenaquintero

This comment has been minimized.

Show comment
Hide comment
@federicomenaquintero

federicomenaquintero Nov 7, 2017

Contributor

Thank you!

Contributor

federicomenaquintero commented Nov 7, 2017

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment