add support of module types #1169

fbrouille · 2023-08-11T18:04:08Z

This PR aims to add support of module types and interfaces. See https://docs.gtk.org/gobject/class.TypeModule.html
The new macros module_object_subclass and module_object_interface allow to register types and interfaces that are part of a module (e.g. a shared library on linux). The macros mimic object_subclass and object_interface but generate code that support registration when the module (TypeModule) is loaded, following the behavior defined in Glib doc. However it is possible to postpone the registration at first use of the type (like for object_subclass) by explicitly setting the macro attribute lazy_registration = true.
Some examples in glib-macros/tests/test.rs, glib/src/subclass/mod.rs and glib/src/subclass/type_module.rs

sdroege · 2023-08-14T07:22:10Z

This would fix #55, or would something still be missing?

glib-macros/src/lib.rs

sdroege · 2023-08-14T07:25:04Z

glib/src/gobject/auto/type_module.rs

+    }
+
+    #[doc(alias = "g_type_module_register_enum")]
+    fn register_enum(&self, name: &str, const_static_values: &EnumValue) -> crate::types::Type {


The add() / register() API here, would it make more sense to hide that behind a macro? E.g. the glib::Enum macro with some additional configuration?

yes you are right. Honestly I did not do it because I don't need it in my project. I will try to add it...

sdroege · 2023-08-14T07:25:38Z

glib/src/gobject/auto/type_plugin.rs

+
+pub trait TypePluginExt: IsA<TypePlugin> + sealed::Sealed + 'static {
+    #[doc(alias = "g_type_plugin_complete_interface_info")]
+    fn complete_interface_info(


And also this maybe as part of a macro instead of proper public API?

This PR focuses more on TypeModule than on TypePlugin. I added TypePlugin and TypePluginExt only because GTypeModule implements GTypePlugin in Glib and I generated the code from GIR files.
I tried to respect the behavior of the TypeModule (according to Glib documentation) in the new macros and i suppose TypePlugin has a different behavior (e.g. it does not define load and unload virtual functions) so I don't really now how to deal with it... And I don't need that since GTypeModule already implements it.

What I mean is that this PR adds quite a bit of public API that should probably never be called by user code but only by the macro, so maybe it can be moved directly into the macro instead and call the FFI API there.

TypePlugin / TypeModule` and their ext traits with the (useful) public API should still exists of course.

I'm currently working on a new implementation of the macros to support TypePlugin as well. I have to finalize it (fix comments and add tests) but I hope I will push a new commit next week. However I won't have so much time for the 2 next weeks, so I will do my best. Stay tuned...

here it is (see 22ab846).

I have changed implementation of macros in order to use static dispatch and avoid dynamic dispatch (see 0fe707d)

sdroege · 2023-08-14T07:27:16Z

glib/src/gobject/interface_info.rs

+wrapper! {
+    #[derive(Debug)]
+    #[doc(alias = "GInterfaceInfo")]
+    pub struct InterfaceInfo(BoxedInline<gobject_ffi::GInterfaceInfo>);


As this has no GType assigned and also otherwise no memory management functions, maybe this should be implemented fully manually? There's no point in ever cloning these, for example, is there? So it could just be a & reference returned from the functions

(same for the other two types here)

Do you have some examples to help me understand how to do it ?

You'd define a

#[repr(transparent)] pub struct InterfaceInfo(gobject_ffi::GInterfaceInfo);

and then can do things like

let ptr: *const gobject_ffi::GInterfaceInfo; let info: &InterfaceInfo = &*(ptr as *const InterfaceInfo);

As there's no memory management associated with these structs in C, inventing that on the bindings side is going to be problematic.

New commit c484822 to change the implementation.
As TypePlugin and TypeModule are generated by gir tool, they require InterfaceInfo, TypeInfo and TypeValueTable to implement function from_glib_ptr_borrow_mut and trait ToGlibPtr.
I don't know how to tell gir to not use these functions.
Therefore I decided to implement the missing functions and missing trait (ToGlibPtr) and also implement other complementary traits (IntoGlib, FromGlib, ToGlibPtrMut, FromGlibPtrNone) which imo is more coherent.

You can't have ToGlibPtr::to_glib_full() for these and also not FromGlibPtrNone and IntoGlib and FromGlib. Those implementations conceptually make no sense.

For the time being you will have to implement the functions with these types manually to get the correct behaviour (borrowing).

This is very basic infrastructure so it's quite expected that gir won't do the right thing here and manual bindings are needed in many places.

I have removed implementation of traits and I have made manual bindings so now code of TypePluginand TypeModule is manual and not generated anymore. See commit d631c13

glib/src/subclass/type_module.rs

sdroege · 2023-08-14T07:31:29Z

glib/src/subclass/type_plugin.rs

+    TypeValueTable,
+};
+
+pub trait TypePluginImpl: ObjectImpl + TypePluginImplExt {


Can you add a minimal test for this too?

i will try, but again I added TypePlugin types only because I wanted to respect how GTypeModule is implemented in Glib (by implementing the GTypePugin interface)

done (see 22ab846).

sdroege

Generally looks good to me but I don't really know the dynamic type API of GObject :)

sdroege

Seems mostly fine to me now. If this is the best API design for this will have to be figured out by someone actually using it, but it works for now apparently :)

sdroege · 2023-09-04T08:19:16Z

glib/src/gobject/type_info.rs

+#[derive(Debug)]
+#[doc(alias = "GTypeInfo")]
+#[repr(transparent)]
+pub struct TypeInfo(pub gobject_ffi::GTypeInfo);


Suggested change

pub struct TypeInfo(pub gobject_ffi::GTypeInfo);

pub struct TypeInfo(pub(crate) gobject_ffi::GTypeInfo);

Maybe, plus an as_ptr() function that returns the underlying FFI pointer?

Done (see f1eac51). Also added from_glib_ptr_borrow_mut.

sdroege · 2023-09-04T08:20:36Z

glib/src/subclass/type_plugin.rs

+    fn parent_complete_type_info(
+        &self,
+        type_: Type,
+        info: &mut TypeInfo,


Why does this have to be mutable? There's nothing we (can) change in it, is there?

yes, according to Glib documentation GObject.TypePlugin.complete_type_info implementations of the interface must fill info.

How would they fill that currently?

complete_type_info is usually called by the Glib type system. A rust implementation of TypePlugin could be something like:

pub struct MyPlugin { my_type_type_info: Cell<glib::TypeInfo>, ... } impl TypePluginImpl for MyPlugin { .... fn complete_type_info(&self, type_: glib::Type, info: &mut glib::TypeInfo, value_table: &mut glib::TypeValueTable) { match self.my_type_type_info.get() { Some(my_type_info) => *info = my_type_info, _ => panic!("unexpected"), } } ... } impl MyPlugin { fn register_dynamic_type(&self, parent_type: glib::Type, type_name: &str, type_info: &glib::TypeInfo, flags: glib::TypeFlags) -> glib::Type { let type_ = unsafe { glib::Type::from_glib(glib::gobject_ffi::g_type_register_dynamic(parent_type.into_glib(), type_name.as_ptr(), self.obj().upcast_ref::<glib::TypePlugin>().as_ptr(), flags.into_glib())) }; if type_.is_valid() { self.my_type_type_info.set(Some(*type_info)); } type_ } }

that sounds like this vfunc should be returning (glib::TypeInfo, glib::TypeValueTable) then

My question was more how an implementation in Rust would even create or modify a TypeInfo right now. But I see, that makes sense for future extensibility then.

Is the TypeInfo passed in by GObject here already correctly initialized, or is the implementer supposed to return a new one? That would basically be what decides if the mutable references or the return tuple is the right choice.

I finally changed it to return a tuple (see 22ab846).

sdroege · 2023-09-04T08:21:08Z

glib/src/gobject/type_plugin.rs

+        &self,
+        g_type: crate::types::Type,
+        info: &mut TypeInfo,
+        value_table: &mut TypeValueTable,


Why are these two mutable? (also in the other places)

Same for value_table.
Also same for info within implementations of GObject.TypePlugin.complete_interface_info.

I finally changed it to return a tuple (see 22ab846).

sdroege · 2023-09-04T08:22:42Z

glib/src/gobject/type_value_table.rs

+#[derive(Debug)]
+#[doc(alias = "GTypeValueTable")]
+#[repr(transparent)]
+pub struct TypeValueTable(pub gobject_ffi::GTypeValueTable);


Suggested change

pub struct TypeValueTable(pub gobject_ffi::GTypeValueTable);

pub struct TypeValueTable(pub(crate) gobject_ffi::GTypeValueTable);

maybe

Done (see f1eac51). Also added as_ptrand from_glib_ptr_borrow_mut.

bilelmoussaoui · 2023-09-07T10:42:14Z

glib/src/gobject/interface_info.rs

@@ -3,4 +3,24 @@
 #[derive(Debug)]
 #[doc(alias = "GInterfaceInfo")]
 #[repr(transparent)]
-pub struct InterfaceInfo(pub gobject_ffi::GInterfaceInfo);
+pub struct InterfaceInfo(pub(crate) gobject_ffi::GInterfaceInfo);


Suggested change

pub struct InterfaceInfo(pub(crate) gobject_ffi::GInterfaceInfo);

pub struct InterfaceInfo(gobject_ffi::GInterfaceInfo);

as you would be from_glib_ptr_borrow_mut to construct them anyway, right?

No, some code needs to create instance of InterfaceInfo and similar types: see in glib/src/subclass/types.rs
Or maybe I can add a new method ?

Seems fine to me like this, but a constructor pub(crate) would also work and reduce the code a bit

bilelmoussaoui · 2023-10-18T16:52:02Z

glib/src/gobject/type_module.rs

+impl fmt::Display for TypeModule {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        f.write_str("TypeModule")
+    }
+}


Please remove all those Display implementations, we have dropped them in the main branch

bilelmoussaoui

looks good to me otherwise, thanks!

fbrouille · 2023-10-18T17:57:45Z

This PR does not support dynamic registration of glib::Enum and glib::Flags within a module or a plugin.
Let me know if I have to modify this PR (needs some work) or if you prefer me to create a new one.

fbrouille · 2023-10-26T19:29:47Z

fix some bugs

sdroege · 2023-10-27T06:38:21Z

Let me know if I have to modify this PR (needs some work) or if you prefer me to create a new one.

Seems fine in a separate PR.

sdroege · 2023-10-27T06:39:35Z

glib/src/subclass/type_plugin.rs

+                    let type_name = CString::new(type_name).unwrap();
+                    let mut g_type = gobject_ffi::g_type_from_name(type_name.as_ptr());


Suggested change

let type_name = CString::new(type_name).unwrap();

let mut g_type = gobject_ffi::g_type_from_name(type_name.as_ptr());

let mut g_type = gobject_ffi::g_type_from_name(type_name.to_glib_none().0);

or

Suggested change

let type_name = CString::new(type_name).unwrap();

let mut g_type = gobject_ffi::g_type_from_name(type_name.as_ptr());

let mut g_type = type_name.run_with_gstr(|type_name| gobject_ffi::g_type_from_name(type_name.as_ptr()));

also elsewhere

sdroege · 2023-10-27T06:42:46Z

glib/src/subclass/mod.rs

+//!         }
+//!
+//!         fn register_dynamic_type(&self, parent_type: glib::Type, type_name: &str, type_info: &glib::TypeInfo, flags: glib::TypeFlags) -> glib::Type {
+//!             let type_ = unsafe {


I wonder if there wouldn't be some more convenient (even if unsafe) API that wouldn't require user code to do FFI directly.

g_type_from_name() for example has safe wrappers, g_type_register_dynamic() should be possible to wrap in something a bit more convenient at least (taking non-FFI types, taking care of the C string conversion, ...)

yes indeed. I have added Type::get_plugin(), Type::register_dynamic() and Type::add_interface_dynamic() wrappers and code is now concise and clearer.

sdroege · 2023-10-27T06:43:55Z

Once this is ready, please also squash it all into fewer commits that are each one functional and useful change (i.e. in your case I guess a single commit)

fbrouille · 2023-10-28T23:37:54Z

About enums and flags, I have removed the piece of code that was introduced early with this PR (essentially due to the generation of code). It will be part of a separate PR.
All commits have been squashed and rebased.

sdroege

One last thing, otherwise this is IMHO ready. @bilelmoussaoui ?

sdroege · 2023-10-30T08:23:20Z

glib/src/types.rs

@@ -223,6 +223,48 @@ impl Type {
        }
    }

+    #[doc(alias = "g_type_get_plugin")]
+    pub fn get_plugin(self) -> Option<TypePlugin> {


Suggested change

pub fn get_plugin(self) -> Option<TypePlugin> {

pub fn plugin(self) -> Option<TypePlugin> {

bilelmoussaoui

otherwise lgtm, thanks

bilelmoussaoui · 2023-10-30T20:15:18Z

glib/src/subclass/interface.rs

@@ -3,25 +3,28 @@
 use std::{marker, mem};

 use super::{InitializingType, Signal};
-use crate::{prelude::*, translate::*, Object, ParamSpec, Type};
+use crate::{
+    gobject::traits::DynamicObjectRegisterExt, prelude::*, translate::*, Object, ParamSpec, Type,


Suggested change

gobject::traits::DynamicObjectRegisterExt, prelude::*, translate::*, Object, ParamSpec, Type,

prelude::*, translate::*, Object, ParamSpec, Type,

glib's prelude should include gobject's one

… types Signed-off-by: fbrouille <[email protected]>

fbrouille force-pushed the module_type branch 6 times, most recently from d08dff6 to 5266273 Compare August 12, 2023 10:59

bilelmoussaoui added the needs-backport PR needs backporting to the current stable branch label Aug 14, 2023

sdroege reviewed Aug 14, 2023

View reviewed changes

glib-macros/src/lib.rs Outdated Show resolved Hide resolved

sdroege reviewed Aug 14, 2023

View reviewed changes

glib/src/subclass/type_module.rs Show resolved Hide resolved

sdroege reviewed Aug 14, 2023

View reviewed changes

glib/src/subclass/type_module.rs Show resolved Hide resolved

sdroege reviewed Aug 14, 2023

View reviewed changes

fbrouille force-pushed the module_type branch from 4c029cf to d631c13 Compare August 30, 2023 10:09

sdroege reviewed Sep 4, 2023

View reviewed changes

bilelmoussaoui reviewed Sep 7, 2023

View reviewed changes

fbrouille force-pushed the module_type branch from f1eac51 to 7eb9cbb Compare October 4, 2023 21:04

bilelmoussaoui added the glib label Oct 10, 2023

fbrouille force-pushed the module_type branch from 7eb9cbb to 0fe707d Compare October 18, 2023 16:16

bilelmoussaoui reviewed Oct 18, 2023

View reviewed changes

bilelmoussaoui previously approved these changes Oct 18, 2023

View reviewed changes

fbrouille dismissed bilelmoussaoui’s stale review via ecfe5fe October 26, 2023 18:33

sdroege reviewed Oct 27, 2023

View reviewed changes

fbrouille force-pushed the module_type branch from ecfe5fe to d945d12 Compare October 28, 2023 23:02

sdroege reviewed Oct 30, 2023

View reviewed changes

fbrouille force-pushed the module_type branch from d945d12 to 35d7226 Compare October 30, 2023 20:13

bilelmoussaoui reviewed Oct 30, 2023

View reviewed changes

fbrouille force-pushed the module_type branch from 35d7226 to 7497a1b Compare October 30, 2023 20:26

add support of object subclasses and interfaces registered as dynamic…

6839fc4

… types Signed-off-by: fbrouille <[email protected]>

sdroege force-pushed the module_type branch from 7497a1b to 6839fc4 Compare October 31, 2023 07:18

sdroege approved these changes Oct 31, 2023

View reviewed changes

bilelmoussaoui approved these changes Oct 31, 2023

View reviewed changes

sdroege merged commit 2cb10ec into gtk-rs:master Oct 31, 2023
48 checks passed

sdroege mentioned this pull request Oct 31, 2023

Support for dynamic types #55

Closed

fbrouille deleted the module_type branch October 31, 2023 16:58

bilelmoussaoui removed the needs-backport PR needs backporting to the current stable branch label Feb 6, 2024

	pub struct TypeInfo(pub gobject_ffi::GTypeInfo);
	pub struct TypeInfo(pub(crate) gobject_ffi::GTypeInfo);

	pub struct TypeValueTable(pub gobject_ffi::GTypeValueTable);
	pub struct TypeValueTable(pub(crate) gobject_ffi::GTypeValueTable);

	pub struct InterfaceInfo(pub(crate) gobject_ffi::GInterfaceInfo);
	pub struct InterfaceInfo(gobject_ffi::GInterfaceInfo);

		let type_name = CString::new(type_name).unwrap();
		let mut g_type = gobject_ffi::g_type_from_name(type_name.as_ptr());

	let type_name = CString::new(type_name).unwrap();
	let mut g_type = gobject_ffi::g_type_from_name(type_name.as_ptr());
	let mut g_type = type_name.run_with_gstr(\|type_name\| gobject_ffi::g_type_from_name(type_name.as_ptr()));

	pub fn get_plugin(self) -> Option<TypePlugin> {
	pub fn plugin(self) -> Option<TypePlugin> {

	gobject::traits::DynamicObjectRegisterExt, prelude::, translate::, Object, ParamSpec, Type,
	prelude::, translate::, Object, ParamSpec, Type,

add support of module types #1169

add support of module types #1169

Conversation

fbrouille commented Aug 11, 2023 • edited Loading

sdroege commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbrouille Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbrouille Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

fbrouille Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

sdroege left a comment

Choose a reason for hiding this comment

sdroege left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbrouille Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbrouille Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bilelmoussaoui left a comment

Choose a reason for hiding this comment

fbrouille commented Oct 18, 2023

fbrouille commented Oct 26, 2023

sdroege commented Oct 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbrouille Oct 28, 2023 • edited Loading

Choose a reason for hiding this comment

sdroege commented Oct 27, 2023

fbrouille commented Oct 28, 2023

sdroege left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bilelmoussaoui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbrouille commented Aug 11, 2023 •

edited

Loading

fbrouille Oct 4, 2023 •

edited

Loading

fbrouille Aug 14, 2023 •

edited

Loading

fbrouille Oct 4, 2023 •

edited

Loading

fbrouille Oct 4, 2023 •

edited

Loading

fbrouille Oct 4, 2023 •

edited

Loading

fbrouille Oct 28, 2023 •

edited

Loading