Allow implementing a GAction interface #1640

andy128k · 2025-01-27T10:05:23Z

gio/src/subclass/action.rs

andy128k · 2025-01-27T20:08:51Z

Another issue I have is that GAction implies that an implementor had some properties.

(process:527349): GLib-GObject-CRITICAL **: 20:51:28.369: Object class ExampleRenamedAction doesn't implement property 'state-type' from interface 'GAction'

(process:527349): GLib-GObject-CRITICAL **: 20:51:28.369: Object class ExampleRenamedAction doesn't implement property 'state' from interface 'GAction'

(process:527349): GLib-GObject-CRITICAL **: 20:51:28.369: Object class ExampleRenamedAction doesn't implement property 'parameter-type' from interface 'GAction'

(process:527349): GLib-GObject-CRITICAL **: 20:51:28.369: Object class ExampleRenamedAction doesn't implement property 'enabled' from interface 'GAction'

I have no clue what would be an idiomatic way to use g_object_class_override_property in Rust.

bilelmoussaoui · 2025-01-28T07:40:33Z

Another issue I have is that GAction implies that an implementor had some properties.

(process:527349): GLib-GObject-CRITICAL **: 20:51:28.369: Object class ExampleRenamedAction doesn't implement property 'state-type' from interface 'GAction'

(process:527349): GLib-GObject-CRITICAL **: 20:51:28.369: Object class ExampleRenamedAction doesn't implement property 'state' from interface 'GAction'

(process:527349): GLib-GObject-CRITICAL **: 20:51:28.369: Object class ExampleRenamedAction doesn't implement property 'parameter-type' from interface 'GAction'

(process:527349): GLib-GObject-CRITICAL **: 20:51:28.369: Object class ExampleRenamedAction doesn't implement property 'enabled' from interface 'GAction'

I have no clue what would be an idiomatic way to use g_object_class_override_property in Rust.

GtkEditable provides a method to automatically add the properties for you along with the get_property and set_property. Either the example implementing a custom Action would have to do that manually or you could add helpers that would do it automatically for you, see the EditableImpl in the gtk4-rs repo.

bilelmoussaoui · 2025-01-28T09:36:46Z

examples/gio_action_impl/action.rs

+
+    #[derive(glib::Properties, Default)]
+    #[properties(wrapper_type = super::RenamedAction)]
+    pub struct RenamedAction {


The properties macro allows overriding properties by the way

I gave it a try but received an error

(process:718176): GLib-GObject-CRITICAL **: 22:59:07.010: When installing property: type 'ExampleRenamedAction' already has a property named 'name'

I guess it's because of the sequence of initialization. When I define a class and implement an interface then class installs properties first and interface fails to install them.

The way how you do it now should work or not?

It does not work. I guess It works as expected in the cases like

class A implements IFace class B extends A implements IFace { override property x of IFace }

where initialization sequence is: A, IFace, B. So, when B is initializing IFace is already there and could be overriden.

but does not work for

class B implements IFace { override property x of IFace }

where initialization sequence is: B, IFace. So, B has nothing to override yet.

You didn't find a solution to this yet, right? This probably should be solved independent of this PR (please create an issue with all you remember)

andy128k · 2025-01-28T22:49:45Z

@bilelmoussaoui Thanks for pointing a direction! Unfortunately Gio does not provide helpers similar to gtk_editable_*_delegate_*, so I needed to replicate C code myself.

I am not sure if I fully understand the subject but this line seems wrong. Gtk's doc uses NUM_PROPERTIES as a second parameter which is usually not an amount of properties but amount + 1 (props are numbered from 1). gtk_editable_install_properties does not add 1.

examples/gio_action_impl/action.rs

gio/src/subclass/action.rs

sdroege · 2025-01-29T08:28:50Z

gio/src/subclass/action.rs

+            glib::ffi::g_free(properties as *mut _);
+            let first_prop = first_prop.assume_init() + 1;
+
+            set_first_prop(instance_type, first_prop as usize);


That's a nice hack for the properties :)

gio/src/subclass/action.rs

sdroege · 2025-01-29T08:32:41Z

gio/src/subclass/action.rs

+        _pspec: &glib::ParamSpec,
+    ) -> Option<glib::Value> {
+        let type_: glib::Type = self.obj().type_();
+        let property = ActionProperty::from_type(type_, prop_id)?;


If you pass in Self::type_() here then you don't need to do all the parent type walking inside from_type() and always have directly the correct type

That's what I've found in GtkEditable's code.

@bilelmoussaoui Why does it do things so complicated, do you remember? :)

sdroege · 2025-01-29T08:35:25Z

gio/src/subclass/action.rs

+    let instance = &*(actionptr as *mut T::Instance);
+    let imp = instance.imp();
+
+    if let Some(parameter_type) = imp.parameter_type() {


This should probably be moved into the set_qdata() call. Now you'd get inconsistent behaviour if the function returns Some on the first call and None afterwards

It's not directly forbidden by Gio's docs, but I think it could be insane if the type of a parameter would change during the action's lifetime.

A similar assumption is made in ActionGroup.

My personal approach here would be to either only call the function once to don't give the user a chance to do it wrong, or otherwise at least to add an assertion :)

I've changed this and other places (name, parameter_type and state_type) in the way that the function is called once.

gio/src/subclass/action.rs

bilelmoussaoui · 2025-02-12T10:49:04Z

gio/src/subclass/action.rs

+        })
+    }
+
+    fn delegate_set_property(


good call of adding this even if it just returns false; it would allow adding some attribute to glib::derived_properties to automatically use the delegate_get_property / delegate_set_property calls, simplifying things a bit.

bilelmoussaoui · 2025-04-26T09:23:31Z

what is left to be done here btw?

andy128k · 2025-05-09T22:38:58Z

what is left to be done here btw?

I'd say its ready, he-he. But I have a concern about a potential off-by-one error (see above) and waited for your judgement.

bilelmoussaoui · 2025-05-22T12:17:13Z

It looks good to me as is. Any future improvements can be done in the future without blocking this forever.

I will wait for an ack from @sdroege, otherwise i will merge in few days if nothing.

sdroege · 2025-05-23T10:00:06Z

examples/gio_action_impl/action.rs

+        }
+
+        fn set_property(&self, id: usize, value: &glib::Value, pspec: &glib::ParamSpec) {
+            if !self.delegate_set_property(id, value, pspec) {


If multiple interfaces like this are implemented, the method name would probably conflict. Should we write the interface name in the method name (delegate_action_set_property()), or change the example to specify the trait (<Self as ActionImplExt>::delegate_set_property(self, id, value, pspec))?

So far there are only Actions and Editable.i rather keep it this way until we find more cases like this

You'd already have this problem then if you implement both interfaces in the same type :)

There is 0 use case for that. You would rather implement GActionMap instead

Sure, but if we want to solve this problem later we need to change API in even more places so it seems useful to figure this out from the beginning. I'm not going to block this PR on this though, just don't tell me in a few years that I didn't warn you :P

My goal is to basically make the derived_properties macro automatically generate those bits for you. I will send a patch for it once this PR lands

Sounds good

sdroege · 2025-05-23T10:01:46Z

gio/src/subclass/action.rs

+                .expect("no parent \"activate\" implementation");
+            func(
+                self.obj().unsafe_cast_ref::<Action>().to_glib_none().0,
+                parameter.to_glib_none().0,


As this is to_glib_none(), maybe the parameter should be a reference (same for others above)

gio/src/subclass/action.rs

sdroege · 2025-05-23T10:05:31Z

gio/src/subclass/action.rs

+    let parameter: Option<glib::Variant> = from_glib_none(parameterptr);
+
+    imp.activate(parameter);


Suggested change

let parameter: Option<glib::Variant> = from_glib_none(parameterptr);

imp.activate(parameter);

let parameter: Option<glib::Variant> = from_glib_borrow(parameterptr);

imp.activate(parameter.as_ref());

or so

sdroege · 2025-05-23T10:07:54Z

gio/src/subclass/action.rs

+            if let Some(first_prop) = get_first_prop(type_) {
+                break ActionProperty::from_id(first_prop, id);
+            }
+            type_ = type_.parent()?;


Is this actually correct to look at all the parents, can you add a comment why?
Shouldn't this just look at the just the type of T?

Maybe a test with multiple levels of GAction inheritance would be useful to make sure this works and continues working

sdroege · 2025-05-23T10:19:10Z

gio/src/subclass/action.rs

+                .get_name
+                .expect("no parent \"get_name\" implementation");
+            let ret = func(self.obj().unsafe_cast_ref::<Action>().to_glib_none().0);
+            from_glib_none(ret)


This is rather suboptimal as it copies strings all the time whenever the name is retrieved. This should be possible to make a &glib::GStr or not?

Co-authored-by: Sebastian Dröge <[email protected]>

sdroege reviewed Jan 27, 2025

View reviewed changes

gio/src/subclass/action.rs Outdated Show resolved Hide resolved

gio/src/subclass/action.rs Outdated Show resolved Hide resolved

andy128k force-pushed the subclass-action branch from 1c5636b to c4de546 Compare January 27, 2025 19:52

bilelmoussaoui reviewed Jan 28, 2025

View reviewed changes

andy128k force-pushed the subclass-action branch from c4de546 to 59cbb3a Compare January 28, 2025 22:30

andy128k force-pushed the subclass-action branch from 59cbb3a to 482d8ef Compare January 28, 2025 22:54

sdroege reviewed Jan 29, 2025

View reviewed changes

examples/gio_action_impl/action.rs Outdated Show resolved Hide resolved

sdroege reviewed Jan 29, 2025

View reviewed changes

gio/src/subclass/action.rs Outdated Show resolved Hide resolved

sdroege reviewed Jan 29, 2025

View reviewed changes

gio/src/subclass/action.rs Outdated Show resolved Hide resolved

sdroege reviewed Jan 29, 2025

View reviewed changes

gio/src/subclass/action.rs Outdated Show resolved Hide resolved

andy128k force-pushed the subclass-action branch from bef560a to ecae6f6 Compare January 29, 2025 08:45

Allow implementing a GAction interface

066dd78

andy128k force-pushed the subclass-action branch from ecae6f6 to 066dd78 Compare January 29, 2025 19:04

andy128k marked this pull request as ready for review January 30, 2025 11:25

bilelmoussaoui reviewed Feb 12, 2025

View reviewed changes

bilelmoussaoui previously approved these changes May 22, 2025

View reviewed changes

sdroege reviewed May 23, 2025

View reviewed changes

gio/src/subclass/action.rs Show resolved Hide resolved

sdroege reviewed May 23, 2025

View reviewed changes

gio/src/subclass/action.rs Outdated Show resolved Hide resolved

sdroege reviewed May 23, 2025

View reviewed changes

gio/src/subclass/action.rs Show resolved Hide resolved

sdroege reviewed May 23, 2025

View reviewed changes

Update gio/src/subclass/action.rs

a039057

Co-authored-by: Sebastian Dröge <[email protected]>

andy128k dismissed bilelmoussaoui’s stale review via a039057 June 1, 2025 22:58

		let parameter: Option<glib::Variant> = from_glib_none(parameterptr);

		imp.activate(parameter);

Uh oh!

Allow implementing a GAction interface #1640

Are you sure you want to change the base?

Allow implementing a GAction interface #1640

Uh oh!

Conversation

andy128k commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andy128k commented Jan 27, 2025

Uh oh!

bilelmoussaoui commented Jan 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy128k commented Jan 28, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bilelmoussaoui commented Apr 26, 2025

Uh oh!

andy128k commented May 9, 2025

Uh oh!

bilelmoussaoui commented May 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy128k commented Jan 27, 2025 •

edited

Loading