add To/FromValue trait to VariantType #340

vhdirk · 2018-06-18T18:48:14Z

This PR enables wrapping a VariantTy(pe) in Value

GuillaumeGomez · 2018-06-18T18:49:16Z

sdroege

Looks good but incomplete. Please add both directions for both types :)

sdroege · 2018-06-18T19:41:23Z

src/variant_type.rs

+
+impl SetValue for VariantTy {
+    unsafe fn set_value(value: &mut Value, this: &Self) {
+        gobject_ffi::g_value_take_boxed(value.to_glib_none_mut().0, this.to_glib_none().0 as glib_ffi::gpointer)


Shouldn't this be set_boxed? The rust part takes a reference, not ownership of the variant ty

sdroege · 2018-06-18T19:43:15Z

src/variant_type.rs

@@ -2,6 +2,9 @@
 // See the COPYRIGHT file at the top-level directory of this distribution.
 // Licensed under the MIT license, see the LICENSE file or <http://opensource.org/licenses/MIT>

+use libc::{c_void};


Unneeded {}

sdroege · 2018-06-18T19:45:44Z

src/variant_type.rs

+
+impl SetValueOptional for VariantTy {
+    unsafe fn set_value_optional(value: &mut Value, this: Option<&Self>) {
+        gobject_ffi::g_value_take_boxed(value.to_glib_none_mut().0, this.to_glib_none().0 as glib_ffi::gpointer)


Is NULL a valid VariantTy?

Probably not. I guess NULL would be "*"?

Needs to be checked in the GLib code, in the implementation of the boxed<->variantty code. Will check tomorrow if nobody else is faster :)

It goes into g_variant_type_copy(), which says that it must not be NULL. So no SetValueOptional for both and you can implement FromValue for both

So needed: StaticType, FromValueOptional, FromValue and SetValue for both types

sdroege · 2018-06-19T05:31:10Z

src/variant_type.rs

+impl<'a> FromValueOptional<'a> for &'a VariantTy {
+    unsafe fn from_value_optional(value: &'a Value) -> Option<Self> {
+        let cvty = gobject_ffi::g_value_dup_boxed(value.to_glib_none().0) as *mut glib_ffi::GVariantType;
+        if cvty.is_null() {


assert for NULL here as it's not allowed

sdroege · 2018-06-19T05:32:05Z

src/variant_type.rs

+
+impl<'a> FromValueOptional<'a> for &'a VariantTy {
+    unsafe fn from_value_optional(value: &'a Value) -> Option<Self> {
+        let cvty = gobject_ffi::g_value_dup_boxed(value.to_glib_none().0) as *mut glib_ffi::GVariantType;


You're leaking the memory here, this has to be get_boxed()

sdroege · 2018-06-19T05:33:49Z

src/variant_type.rs

+
+impl<'a> FromValueOptional<'a> for VariantType {
+    unsafe fn from_value_optional(value: &'a Value) -> Option<Self> {
+        Option::<VariantType>::from_glib_full(gobject_ffi::g_value_dup_boxed(value.to_glib_none().0) as *mut glib_ffi::GVariantType)


This should be from_glib_none() and get_boxed().

See also this comment in the code from from_glib_full:

impl FromGlibPtrFull<*const glib_ffi::GVariantType> for VariantType { unsafe fn from_glib_full(ptr: *const glib_ffi::GVariantType) -> VariantType { // Don't assume ownership of a const pointer. // A transfer: full annotation on a `const GVariantType*` is likely a bug. VariantTy::from_ptr(ptr).to_owned() } }

vhdirk · 2018-06-20T16:38:25Z

How come a VariantType can't be null? This test seems to tell otherwise? https://gitlab.gnome.org/GNOME/glib/blob/master/gio/tests/actions.c

sdroege · 2018-06-20T16:56:10Z

How come a VariantType can't be null? This test seems to tell otherwise? https://gitlab.gnome.org/GNOME/glib/blob/master/gio/tests/actions.c

Inside GValue it is using g_variant_type_copy(), which will assert if NULL is passed:
https://gitlab.gnome.org/GNOME/glib/blob/b9aed426d198ccf27078263d982bb247c82ccaee/glib/gvarianttype.c#L330 and https://gitlab.gnome.org/GNOME/glib/blob/b9aed426d198ccf27078263d982bb247c82ccaee/glib/gvarianttype.c#L184-188

That might be a bug on the GLib side though. Do you need it to be NULL for anything?

sdroege · 2018-06-20T16:56:57Z

src/variant_type.rs

-            Some(VariantTy::from_ptr(cvty))
-        }
+        let cvty = gobject_ffi::g_value_get_boxed(value.to_glib_none().0) as *mut glib_ffi::GVariantType;
+        assert!(cvty.is_null());


This is missing a negation. You want to assert that it is not NULL

vhdirk · 2018-06-20T18:16:18Z

That might be a bug on the GLib side though. Do you need it to be NULL for anything?

Not necessarily. Only for aping SimpleAction

sdroege · 2018-06-20T18:23:18Z

That might be a bug on the GLib side though. Do you need it to be NULL for anything?

Not necessarily. Only for aping SimpleAction

How does it work in SimpleAction (why does it not run into these assertions?), what are the NULL types used for exactly?

vhdirk · 2018-06-20T18:28:51Z

The docs of SimpleAction state:

The type of the parameter that must be given when activating the action.

I've never used GAction before, so I'm not sure what that means. And my rust version of SimpleAction is still panicking somewhere without a clear trace...

vhdirk · 2018-06-20T18:30:15Z

More from the docs

Since GLib 2.40, if no handler is connected to this signal then the default behaviour for boolean-stated actions with a NULL parameter type is to toggle them via the “change-state” signal.

sdroege · 2018-06-20T18:32:22Z

That hints at a bug in GLib then, that clearly says that NULL types are expected behaviour... but running g_boxed_copy() on such GValues (as from the signals) would assert. Can you file a bug against GLib about that?

vhdirk · 2018-06-20T19:04:48Z

From what I can tell, it is not so much GValueType that is not nullable, rather the type string itself?

sdroege · 2018-06-21T05:40:51Z

From what I can tell, it is not so much GValueType that is not nullable, rather the type string itself?

I'm not sure what you mean with this. GVariantType seems to be possible to be NULL in various contexts, just not when stored in a GValue (but GSimpleAction seems to use it as such nonetheless).

vhdirk · 2018-06-21T06:39:47Z

Looking at the implementation of g_variant_type_copy, I think the documentation is wrong.

GVariantType *
g_variant_type_copy (const GVariantType *type)
{
  gsize length;
  gchar *new;

  g_return_val_if_fail (g_variant_type_check (type), NULL);

  <snip>
}

There's no assertion here. It will just return another null pointer when you provide it a null pointer.

sdroege · 2018-06-21T08:45:50Z

g_return_val_if_fail (g_variant_type_check (type), NULL);

This is an assertion (it will return FALSE for NULL)

vhdirk · 2018-06-21T08:56:36Z

Are you sure?

According to the docs:

#define             g_return_val_if_fail(expr,val)

It will return val if expr evaluates to FALSE.

So in this case, it will return NULL when g_variant_type_check(type) returns FALSE. Which is the case when type is NULL to begin with.

sdroege · 2018-06-21T09:07:34Z

It's only to be used as a guard against programming mistakes. It's an assertion, but not taking down your application unless yet set G_DEBUG=fatal_criticals. It causes a g_critical() warning, which is also printed on stderr.

vhdirk · 2018-06-21T09:30:33Z

Ah, I though that was just a shorthand. So yeah, this is a bug in glib then.

sdroege · 2018-06-21T09:35:29Z

Ah, I though that was just a shorthand. So yeah, this is a bug in glib then.

Lovely :) Not sure what we can do about that, need a workaround until a new GLib version arrives. But your code making it nullable is correct then, just GLib isn't correct.

sdroege · 2018-06-21T09:39:53Z

I would suggest to file a bug there and go ahead with your implementation. It will not cause unsafety, it only causes ugly warnings on the terminal and those are GLib's problem

sdroege · 2018-06-21T09:40:15Z

src/variant_type.rs

+    }
+}
+
+// impl SetValueOptional for VariantType {


You can include this one now :)

sdroege · 2018-06-21T10:24:52Z

Looks good to me, but please also link to the GLib bug report about this here for future reference

vhdirk · 2018-06-21T10:27:59Z

I'll create the glib bug report soon.

vhdirk · 2018-06-22T06:04:35Z

https://gitlab.gnome.org/GNOME/glib/issues/1427

sdroege · 2018-06-22T06:09:10Z

@GuillaumeGomez Let's get this in then?

GuillaumeGomez · 2018-06-22T08:14:42Z

Yes! Thanks @vhdirk!

add To/FromValue trait to VariantType

9fd66f7

sdroege reviewed Jun 18, 2018

View reviewed changes

dvhaeren added 2 commits June 18, 2018 21:55

fix ownership of conversion & minor cleanup

2c1876a

add FromValue conversion for VariantTy

030d2d0

sdroege reviewed Jun 19, 2018

View reviewed changes

fix memory issues

56fcf7e

sdroege reviewed Jun 20, 2018

View reviewed changes

invert assertion & add simple test for VariantTy

925ac35

sdroege reviewed Jun 21, 2018

View reviewed changes

src/variant_type.rs Outdated

}

}

// impl SetValueOptional for VariantType {

Copy link

Member

sdroege Jun 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can include this one now :)

varianttype can actually be NULL

4735425

vhdirk mentioned this pull request Jun 21, 2018

add generated action interface gtk-rs/gobject-subclass#10

Merged

GuillaumeGomez merged commit f016524 into gtk-rs:master Jun 22, 2018

Uh oh!

add To/FromValue trait to VariantType #340

add To/FromValue trait to VariantType #340

Uh oh!

Conversation

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

Uh oh!

Uh oh!

GuillaumeGomez commented Jun 18, 2018

Uh oh!

sdroege left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vhdirk commented Jun 20, 2018

Uh oh!

sdroege commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vhdirk commented Jun 20, 2018

Uh oh!

sdroege commented Jun 20, 2018

Uh oh!

vhdirk commented Jun 20, 2018

Uh oh!

vhdirk commented Jun 20, 2018

Uh oh!

sdroege commented Jun 20, 2018

Uh oh!

vhdirk commented Jun 20, 2018

Uh oh!

sdroege commented Jun 21, 2018

Uh oh!

vhdirk commented Jun 21, 2018

Uh oh!

sdroege commented Jun 21, 2018

Uh oh!

vhdirk commented Jun 21, 2018

Uh oh!

sdroege commented Jun 21, 2018

Uh oh!

vhdirk commented Jun 21, 2018

Uh oh!

sdroege commented Jun 21, 2018

Uh oh!

sdroege commented Jun 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdroege commented Jun 21, 2018

Uh oh!

vhdirk commented Jun 21, 2018

Uh oh!

vhdirk commented Jun 22, 2018

Uh oh!

sdroege commented Jun 22, 2018

Uh oh!

GuillaumeGomez commented Jun 22, 2018

Uh oh!

Uh oh!

vhdirk commented Jun 18, 2018 •

edited

Loading