glib: Decouple ObjectInterface impl from interface class struct #1384

felinira · 2024-05-04T16:23:49Z

This makes ObjectInterface and ObjectSubclass more similar to each other and allows for cleaner separation between the unsafe ffi code and rust implementation.

There is some bikeshedding left to be done: InterfaceClassStruct is quite clumsy, but reusing ClassStruct is not possible, as it is tailored towards object subclasses. We could call it InterfaceStruct to align it with IsClass ->IsInterface and similar traits that omit Class. Then we should also name ObjectInterface::Class ObjectInterface::Interface I suppose.

Do we need something like glib::subclass::basic::ClassStruct for InterfaceClassStruct? I would say we don't, as most interfaces define vfuncs anyway.

Closes #1382

sdroege · 2024-05-04T20:06:57Z

InterfaceClassStruct is quite clumsy, but reusing ClassStruct is not possible, as it is tailored towards object subclasses. We could call it InterfaceStruct to align it with IsClass ->IsInterface and similar traits that omit Class. Then we should also name ObjectInterface::Class ObjectInterface::Interface I suppose.

Yes I think that makes sense, both. Having the word class in the context of interfaces is just confusing, even if some GObject API calls it like that (g_type_class_peek() etc)

Do we need something like glib::subclass::basic::ClassStruct for InterfaceClassStruct? I would say we don't, as most interfaces define vfuncs anyway.

I think that would be nice, especially as it's just a bit of copy&paste. It is probably not useful often, but if you do a signals/properties-only interface then this would make it a bit less code to write (there are some such interfaces!).

glib-macros/tests/object_subclass_dynamic.rs

sdroege · 2024-05-04T20:12:25Z

glib/src/object.rs

@@ -1317,6 +1317,15 @@ macro_rules! glib_object_wrapper {
        );
    };

+    (@object_interface [$($attr:meta)*] $visibility:vis $name:ident $(<$($generic:ident $(: $bound:tt $(+ $bound2:tt)*)?),+>)?, $iface:ty,


There's probably one branch of the macro that is not in use anymore now?

I don't think so. There are three branch arms from top to bottom:

The short @interface one is called by

gtk-rs-core/glib/src/wrapper.rs

Lines 416 to 431 in d849f56

// Interface

(

$(#[$attr:meta])*

$visibility:vis struct $name:ident $(<$($generic:ident $(: $bound:tt $(+ $bound2:tt)*)?),+>)? (Interface<$ffi_name:ty $(, $ffi_class_name:ty)?>) $(@requires $($requires:path),+)?;

match fn {

type_ => || $get_type_expr:expr,

}

) => {

$crate::glib_object_wrapper!(

@interface [$($attr)*] $visibility $name $(<$($generic $(: $bound $(+ $bound2)*)?),+>)?, *mut std::os::raw::c_void, $ffi_name,

$( @ffi_class $ffi_class_name ,)?

@type_ $get_type_expr,

@requires [$( $($requires),+ )?]

);

};

The new @object_interface one is called by

gtk-rs-core/glib/src/wrapper.rs

Lines 433 to 443 in d849f56

// ObjectInterface

(

$(#[$attr:meta])*

$visibility:vis struct $name:ident $(<$($generic:ident $(: $bound:tt $(+ $bound2:tt)*)?),+>)? (ObjectInterface<$iface_name:ty>) $(@requires $($requires:path),+)?;

) => {

$crate::glib_object_wrapper!(

@object_interface [$($attr)*] $visibility $name $(<$($generic $(: $bound $(+ $bound2)*)?),+>)?, $iface_name,

@type_ $crate::translate::IntoGlib::into_glib(<$iface_name as $crate::subclass::interface::ObjectInterfaceType>::type_()),

@requires [$( $($requires),+ )?]

);

};

The full implementation is being subsequently used be the former two macros. Also empirically, deleting either of the branches causes errors somewhere. 😅

Mh ok! This macro grew too much :)

sdroege · 2024-05-04T20:14:25Z

glib/src/subclass/types.rs

@@ -305,22 +323,22 @@ unsafe extern "C" fn interface_init<T: ObjectSubclass, A: IsImplementable<T>>(
 ) where
    <A as ObjectType>::GlibClassType: Copy,
 {
-    let iface = &mut *(iface as *mut crate::Interface<A>);
+    let klass = &mut *(iface as *mut crate::Interface<A>);


OOC, could we use g_type_class_peek_parent() here instead of making our own copy?

Since derived classes hold a reference count on their parent classes as long as they are instantiated, the returned class will always exist.

Always is a very strong assertion, and we do the same for class_init, so I don't see why not.

Ah but peek_parent isn't right.

This code copies the type data of the interface into the type data of the class that implements the interface. https://docs.gtk.org/gobject/concepts.html#interface-initialization

We get a pointer to the interface class type in this method. It will be valid as long as instances of the interface exist. So we should be able to just take this pointer and use it in the TypeData.

But this is our version of the interface struct, with the changes applied by our type. Not the default/parent one, which is currently saved away

sdroege

Looks good to me but I only took a short look now :) I'll check again tomorrow or Monday.

felinira · 2024-05-04T20:59:58Z

Do we need something like glib::subclass::basic::ClassStruct for InterfaceClassStruct? I would say we don't, as most interfaces define vfuncs anyway.

I think that would be nice, especially as it's just a bit of copy&paste. It is probably not useful often, but if you do a signals/properties-only interface then this would make it a bit less code to write (there are some such interfaces!).

Alright, that apparently also needs an addition to ObjectInterfaceType then to define a GlibType and GlibClassType?

sdroege · 2024-05-04T21:50:59Z

Alright, that apparently also needs an addition to ObjectInterfaceType then to define a GlibType and GlibClassType?

Also a good thing to make more consistent between interfaces and classes I guess :) GlibInterfaceType though.

For classes I used those associated types elsewhere in the past too so it might also become useful for something else.

felinira · 2024-05-06T14:54:31Z

Maybe it makes sense to handle this separately. Interfaces can also inherit from other interfaces, so we can't just assume GTypeInterface. We'd also need access to the public type, so I guess ObjectInterface also needs a Type and ParentType. This is all a bit much to add in one go IMO.

This makes ObjectInterface and ObjectSubclass more similar to each other and allows for cleaner separation between unsafe ffi code and application implementation.

sdroege · 2024-05-06T15:26:49Z

Interfaces can also inherit from other interfaces, so we can't just assume GTypeInterface

Is that so? 🤯 I thought that worked differently via the prerequisites.

I'm fine with keeping this for a separate PR in any case. Don't have to do everything at once.

felinira · 2024-05-06T17:54:12Z

Is that so? 🤯 I thought that worked differently via the prerequisites.

You are right. I stared at GType for too long.

sdroege · 2024-05-06T18:01:27Z

But now I'm wondering if that could actually be made to work 🤣

sdroege · 2024-05-08T08:02:10Z

@felinira Should we get this in then or do you want to change more as part of this PR? The boxed copy of the struct and the simple generic interface struct can also be done separately, don't really have to entangle it all and this here is IMHO complete and good as it is.

felinira · 2024-05-08T13:44:35Z

@felinira Should we get this in then or do you want to change more as part of this PR? The boxed copy of the struct and the simple generic interface struct can also be done separately, don't really have to entangle it all and this here is IMHO complete and good as it is.

I won't mind getting this in now. I opened #1389 to track the remaining bits.

sdroege · 2024-05-08T13:59:11Z

Let's do that then :)

sdroege requested review from sdroege and bilelmoussaoui May 4, 2024 20:04

sdroege reviewed May 4, 2024

View reviewed changes

glib-macros/tests/object_subclass_dynamic.rs Outdated Show resolved Hide resolved

sdroege reviewed May 4, 2024

View reviewed changes

felinira force-pushed the object-interface-struct branch 2 times, most recently from 1eacb86 to a431d87 Compare May 4, 2024 20:56

glib: Decouple ObjectInterface impl from interface class struct

60ffeec

This makes ObjectInterface and ObjectSubclass more similar to each other and allows for cleaner separation between unsafe ffi code and application implementation.

felinira force-pushed the object-interface-struct branch from a431d87 to 60ffeec Compare May 6, 2024 15:57

felinira mentioned this pull request May 8, 2024

[FEATURE REQUEST] Feature parity between interfaces and objects #1389

Open

sdroege merged commit 839fc12 into gtk-rs:master May 8, 2024
48 checks passed

sdroege mentioned this pull request May 9, 2024

examples: Add example for custom class structs and virtual methods #1378

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

glib: Decouple ObjectInterface impl from interface class struct #1384

glib: Decouple ObjectInterface impl from interface class struct #1384

felinira commented May 4, 2024

sdroege commented May 4, 2024

sdroege May 4, 2024

felinira May 4, 2024

sdroege May 4, 2024

sdroege May 4, 2024

felinira May 6, 2024

felinira May 6, 2024

sdroege May 6, 2024

sdroege left a comment

felinira commented May 4, 2024

sdroege commented May 4, 2024

felinira commented May 6, 2024

sdroege commented May 6, 2024

felinira commented May 6, 2024

sdroege commented May 6, 2024

sdroege commented May 8, 2024

felinira commented May 8, 2024

sdroege commented May 8, 2024

	// Interface
	(
	$(#[$attr:meta])*
	$visibility:vis struct $name:ident $(<$($generic:ident $(: $bound:tt $(+ $bound2:tt)*)?),+>)? (Interface<$ffi_name:ty $(, $ffi_class_name:ty)?>) $(@requires $($requires:path),+)?;

	match fn {
	type_ => \|\| $get_type_expr:expr,
	}
	) => {
	$crate::glib_object_wrapper!(
	@interface [$($attr)] $visibility $name $(<$($generic $(: $bound $(+ $bound2))?),+>)?, *mut std::os::raw::c_void, $ffi_name,
	$( @ffi_class $ffi_class_name ,)?
	@type_ $get_type_expr,
	@requires [$( $($requires),+ )?]
	);
	};

	// ObjectInterface
	(
	$(#[$attr:meta])*
	$visibility:vis struct $name:ident $(<$($generic:ident $(: $bound:tt $(+ $bound2:tt)*)?),+>)? (ObjectInterface<$iface_name:ty>) $(@requires $($requires:path),+)?;
	) => {
	$crate::glib_object_wrapper!(
	@object_interface [$($attr)] $visibility $name $(<$($generic $(: $bound $(+ $bound2))?),+>)?, $iface_name,
	@type_ $crate::translate::IntoGlib::into_glib(<$iface_name as $crate::subclass::interface::ObjectInterfaceType>::type_()),
	@requires [$( $($requires),+ )?]
	);
	};

glib: Decouple ObjectInterface impl from interface class struct #1384

glib: Decouple ObjectInterface impl from interface class struct #1384

Conversation

felinira commented May 4, 2024

sdroege commented May 4, 2024

sdroege May 4, 2024

Choose a reason for hiding this comment

felinira May 4, 2024

Choose a reason for hiding this comment

sdroege May 4, 2024

Choose a reason for hiding this comment

sdroege May 4, 2024

Choose a reason for hiding this comment

felinira May 6, 2024

Choose a reason for hiding this comment

felinira May 6, 2024

Choose a reason for hiding this comment

sdroege May 6, 2024

Choose a reason for hiding this comment

sdroege left a comment

Choose a reason for hiding this comment

felinira commented May 4, 2024

sdroege commented May 4, 2024

felinira commented May 6, 2024

sdroege commented May 6, 2024

felinira commented May 6, 2024

sdroege commented May 6, 2024

sdroege commented May 8, 2024

felinira commented May 8, 2024

sdroege commented May 8, 2024