add generated action interface #10

vhdirk · 2018-06-05T04:52:44Z

Let's start off with the action interface, since that one is used by application at some point.
This generated file is not at all usable yet, but let's see where we get from here.

sdroege · 2018-06-05T05:54:02Z

gio-subclass/src/action.rs

+    iface_data: glib_ffi::gpointer
+) {
+    callback_guard!();
+    let action_iface = &mut *(iface as *mut ffi::);


This is missing the C interface struct type here after ffi::

sdroege · 2018-06-05T05:56:29Z

gio-subclass/src/action.rs

+
+unsafe extern "C" fn action_get_type<T: ObjectType>(
+    type_: glib_ffi::GType
+) -> glib::Type {


This function is not needed for interfaces, what you basically have here is a trampoline calling a get_type function on the interface... but there is none on GAction

sdroege · 2018-06-05T05:58:54Z

gio-subclass/src/action.rs

+    let interface_static = &*(ptr as *const T::InstanceStructType);
+    let imp = instance.get_impl();
+    let imp = (*(*interface_static).imp_static).get_impl(imp);
+    imp.get_state_type()


Like signal trampolines, this needs all the conversion stuff for the return value (from_glib_full in this case) and also the parameters (none here). You also want to pass the &gio::Action via &from_glib_borrow(ptr) to this and all other functions

sdroege · 2018-06-05T06:00:08Z

gio-subclass/src/action.rs

+    let interface_static = &*(ptr as *const T::InstanceStructType);
+    let imp = instance.get_impl();
+    let imp = (*(*interface_static).imp_static).get_impl(imp);
+    imp.change_state()


Here you want to pass both ptr and value to the function, which should work the same as in signal trampolines

sdroege · 2018-06-05T06:01:11Z

gio-subclass/src/action.rs

+
+pub trait ActionImpl: AnyImpl + 'static {
+
+    fn activate(&self, action: &T, parameter: Option<&glib::Variant>);


The &T here should be a &gio::Action AFAIU

True. I was thinking for a moment that we could make actionImpl generic over T? But i'm not sure if that has any benefits to it

That has the problem that you can't use Box<ActionImpl> anymore but have to provide the type parameter everywhere, or not?

Not sure. I can't even remember why I wanted to make it generic

Let's not do that for now then :) It can always be dynamically casted if needed

sdroege · 2018-06-05T06:01:53Z

Looks generally very good, the main thing missing here at this point seems to be the type conversion between C and Rust, which should be the same as in the signal trampolines

vhdirk · 2018-06-06T17:51:02Z

Aside from missing imports and so on, the largest issue remains those trampolines. I've been looking at the signals implementation, but I can only find stuff marked as TODO :)

sdroege · 2018-06-07T08:55:33Z

I've been looking at the signals implementation, but I can only find stuff marked as TODO :)

There's https://github.com/gtk-rs/gir/blob/master/src/codegen/trampoline.rs, specifically the trampoline_call_func function. trampoline_call_parameters is converting all the parameters from C to Rust, analysis.ret.trampoline_to_glib converts the return value back to C.

vhdirk · 2018-06-07T14:35:25Z

Thanks. I'm not sure how correct this is, but it seems like it could work

sdroege · 2018-06-07T14:42:29Z

gio-subclass/src/action.rs

+    let imp = instance.get_impl();
+    let imp = (*(*interface_static).imp_static).get_impl(imp);
+    let wrap: T = from_glib_borrow(instance);
+    imp.activate(&wrap, &from_glib_none(action), /*Unknown conversion*/parameter)


This could be &from_glib_borrow(action) (and IIRC would be for signals). Not sure why it falls apart on the GVariant, in the function just below it works :)

Generally this should do &from_glib_borrow for all shared/boxed/object types (if transfer none) like it would in signal trampolines. There's probably a code path that has to be enabled for your case too here

See e.g. https://github.com/sdroege/gstreamer-rs/blob/85ca3b94a34d0236314aac900d5926c81ba73d99/gstreamer/src/auto/element.rs#L535-L540

Actually, all of the 'action' conversions here are incorrect. That's what 'wrap' is

sdroege · 2018-06-07T14:45:26Z

gio-subclass/src/action.rs

+    let imp = instance.get_impl();
+    let imp = (*(*interface_static).imp_static).get_impl(imp);
+    let wrap: T = from_glib_borrow(instance);
+    imp.get_name(&wrap, &from_glib_none(action))/*Not checked*/.to_glib_none().0


The return value is unsafe but that's because the C API is broken, this needs a manual implementation.

You need to return a string here that is valid for the whole lifetime of the underlying action object

@federicomenaquintero things like this are btw examples why I think we will need an override system somewhere between gnome-class and the .gir files, and why I believe the trampolines and some glue infrastructure should live in the gtk/etc bindings for gnome-class to use.

I have some more examples in GStreamer :) We should talk about this at the next hackfest in more detail

sdroege · 2018-06-07T14:45:42Z

gio-subclass/src/action.rs

+    let imp = instance.get_impl();
+    let imp = (*(*interface_static).imp_static).get_impl(imp);
+    let wrap: T = from_glib_borrow(instance);
+    imp.get_parameter_type(&wrap, &from_glib_none(action))/*Not checked*/.to_glib_none().0


Same as for the name

sdroege · 2018-06-07T14:46:47Z

gio-subclass/src/action.rs

+    let imp = instance.get_impl();
+    let imp = (*(*interface_static).imp_static).get_impl(imp);
+    let wrap: T = from_glib_borrow(instance);
+    imp.get_state(&wrap, &from_glib_none(action)).to_glib_full()


This return value could use an optimization but not important right now

let ret = imp.get_state(...); let ptr = ret.to_glib_none().0; mem::forget(ret); ptr

sdroege · 2018-06-07T14:47:03Z

gio-subclass/src/action.rs

+    let imp = instance.get_impl();
+    let imp = (*(*interface_static).imp_static).get_impl(imp);
+    let wrap: T = from_glib_borrow(instance);
+    imp.get_state_type(&wrap, &from_glib_none(action))/*Not checked*/.to_glib_none().0


Same problem as for name

vhdirk · 2018-06-07T15:24:14Z

As far as the gir is concerned, I don't think I can manage to fix the c-api broken-ness. So either I fix that manually, or we come up with a plan to pull implementations from a separate file.
Even though I like the ambition of the latter, I'd like to have something to work with. Who knows what else needs fixing, so I guess using the gir to generate a skeleton and going from there is more feasible

sdroege · 2018-06-08T06:46:43Z

So either I fix that manually, or we come up with a plan to pull implementations from a separate file.

Generally I was thinking separate files should work fine, e.g. marking a vfunc as ignore in the toml file and then having one file per vfunc that defines the 3 parts of it: 1) the trait function signature, 2) the chaining up part, 3) the trampoline. And then during code generation, those parts are spliced into the output.

vhdirk · 2018-06-08T11:38:51Z

Do you have any proposition regarding the syntax of that file? I'd like to make it as simple as possible to know what content to splice where. Ideally, the gir should not know about what's in that file, only which block to insert where.

sdroege · 2018-06-08T11:55:09Z

Do you have any proposition regarding the syntax of that file? I'd like to make it as simple as possible to know what content to splice where. Ideally, the gir should not know about what's in that file, only which block to insert where.

Yes I agree, ideally we wouldn't need to parse it at all. So maybe just separate files with the actual content that is inserted in the relevant places? manual/action_vfuncname_trampoline.custom, etc?

vhdirk · 2018-06-08T12:40:45Z

Something like this? (see gir for implementatin details)

sdroege · 2018-06-08T13:40:45Z

Yes, exactly that :)

sdroege · 2018-06-09T19:50:58Z

diff --git a/gio-subclass/src/action.rs b/gio-subclass/src/action.rs
index 7542553..9a6a927 100644
--- a/gio-subclass/src/action.rs
+++ b/gio-subclass/src/action.rs
@@ -78,9 +78,9 @@ unsafe extern "C" fn action_activate<T: ObjectType>
     let param = if parameter.is_null(){
         None
     }else{
-        Some(&from_glib_none(parameter))
+        Some(from_glib_none(parameter))
     };
-    imp.activate(&wrap, param)
+    imp.activate(&wrap, param.as_ref())
 }
 
 unsafe extern "C" fn action_change_state<T: ObjectType>
@@ -206,7 +206,7 @@ unsafe extern "C" fn action_init<T: ObjectType>(
     iface_data: glib_ffi::gpointer
 ) {
     callback_guard!();
-    let action_iface = &mut *(iface as *mut gio_ffi::GAction);
+    let action_iface = &mut *(iface as *mut gio_ffi::GActionInterface);
     let iface_type = (*(iface as *const gobject_ffi::GTypeInterface)).g_type;
     let type_ = (*(iface as *const gobject_ffi::GTypeInterface)).g_instance_type;
     let klass = &mut *(gobject_ffi::g_type_class_ref(type_) as *mut ClassStruct<T>);

This is required to make it compile. Wrong type is used for the interface, and some lifetime issues with a variable

vhdirk · 2018-06-09T20:08:26Z

Thanks!

vhdirk · 2018-06-09T20:15:39Z

So while this thing now compiles, I have no idea how to use it. I'd guess we need SimpleAction or so in order to test this?

sdroege · 2018-06-10T06:01:10Z

No, SimpleAction already implements the GAction interface. We'd need a new GObject and then can have it implement the interface. I'll write a compiling-but-not-working example for that later (because I don't know how GAction is supposed to be used properly) :)

sdroege · 2018-06-10T13:27:44Z

You can find an example that compiles here. Let me know if you have any questions about the code, and also note the function I've added to action.rs

sdroege · 2018-06-13T05:28:58Z

Were you able to write a subclass of GAction that is actually doing something meaningful?

vhdirk · 2018-06-13T05:51:20Z

No. Not yet. I don't know what to do with that one either :)

sdroege · 2018-06-13T05:54:47Z

Ah, I thought you started with GAction because that's something you actually needed :) I guess look at the implementation of GSimpleAction in gio then

This still panics when using the getter on a Boxed VariantType though

vhdirk · 2018-07-04T07:55:05Z

I've regenerated one last time. It now includes some let rs_ret statements. That's because I've been tailoring the gir to support writing back output parameters to pointers. I'll get a PR using that specifically up soon.

sdroege · 2018-07-04T09:14:16Z

Ok, go for it then :)

vhdirk · 2018-07-04T09:37:55Z

Ready for merge?

sdroege · 2018-07-04T10:16:15Z

Yes, but please don't forget to look at the property flag stuff afterwards :)

vhdirk · 2018-07-04T10:21:29Z

I'll do that soon. I'll need it anyway.

sdroege reviewed Jun 5, 2018

View reviewed changes

vhdirk force-pushed the gaction branch from db96087 to 4ffbf7e Compare June 7, 2018 14:36

sdroege reviewed Jun 7, 2018

View reviewed changes

vhdirk force-pushed the gaction branch from 795a4f6 to 7840149 Compare June 8, 2018 07:20

vhdirk added 22 commits July 3, 2018 14:28

rebase on master

6c05000

wip: proper test for gaction

4ceca27

add gaction to glib_wrapper list

70ffb58

wip: beef up gaction test

2a306dc

This still panics when using the getter on a Boxed VariantType though

fix simpleaction unit test

d40cea4

extend simpleaction unit test some more

7f2258d

wip: simpleaction test

aac248a

wip: simpleaction test

318448a

invert did_run assertion

f16ac62

invert get_property(enabled) assertion

c4ee724

wip: ownership of pointers

081094a

fix argument for activate signal

ae4f36b

regenerate

6555dfc

is this a good way to implement those free functions?

ba0ac33

second try for destroy function

058d458

regenerate

d9aedf1

regenerate

ca2a3ab

regenerate

254a53a

Fix lib after removal of 'free' module

961a40b

remove new GParamSpec properties for now

95ced4c

regenerate

d976bfb

regenerate

500da22

vhdirk force-pushed the gaction branch from 18bfb7b to 500da22 Compare July 3, 2018 12:28

regenerate

0bc5673

vhdirk merged commit bc0d24f into gtk-rs:master Jul 4, 2018


		pub trait ActionImpl: AnyImpl + 'static {

		fn activate(&self, action: &T, parameter: Option<&glib::Variant>);

add generated action interface #10

add generated action interface #10

Uh oh!

Conversation

vhdirk commented Jun 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdroege Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sdroege commented Jun 5, 2018

Uh oh!

vhdirk commented Jun 6, 2018

Uh oh!

sdroege commented Jun 7, 2018

Uh oh!

vhdirk commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdroege Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

vhdirk commented Jun 7, 2018

Uh oh!

sdroege commented Jun 8, 2018

Uh oh!

vhdirk commented Jun 8, 2018

Uh oh!

sdroege commented Jun 8, 2018

Uh oh!

vhdirk commented Jun 8, 2018

Uh oh!

sdroege commented Jun 8, 2018

Uh oh!

sdroege commented Jun 9, 2018

Uh oh!

vhdirk commented Jun 9, 2018

Uh oh!

vhdirk commented Jun 9, 2018

Uh oh!

sdroege commented Jun 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdroege commented Jun 10, 2018

Uh oh!

sdroege commented Jun 13, 2018

sdroege Jun 5, 2018 •

edited

Loading

sdroege Jun 7, 2018 •

edited

Loading

sdroege commented Jun 10, 2018 •

edited

Loading