I've started to work on this as one of the Gtk 3.0 tasks.

Created attachment 116648 [details] [review]
Initial (unfinished) patch

This patch implements an omnipotent _g_object_get_generic with careful type checking and ability to retrieve different kinds of property values. On top of that there are several public g_object_get_foo style functions there merely use _g_object_get_generic with a type name, so it can make sure we fail for example when trying to g_object_get_int (gtk_window_new(), "title") even though that property is not an integer.

The accessors don't cover all types yet. And there are no Since tags.

Some things I apparently missed on the first review:

+static
+_g_object_get_generic (gpointer     object,
+                       const gchar *property_name,
+                       const gchar *str_func,
+                       GType        gtype,
+                       void        *value)

The function should be "static void" and the leading _ is not needed.
It should be "gpointer value" not "void *value"

+  if (!G_IS_OBJECT (object))
+  {
+    g_return_if_fail_warning ("Glib-Object",
+                              str_func, "G_IS_OBJECT");
+    return;
+  }

Indentation is broken here.

Also, I guess the functions should check the value type, not
the param spec type. Or should they check both?

In any case, checking the type for (un)equality seems wrong,
the code should use g_type_is_a() so derived param specs
will work too.

Created attachment 116664 [details] [review]
Improved implementation

Thank you for the comments, I addressed the incomplete prototype and the wrong indentation.

> Also, I guess the functions should check the value type, not
> the param spec type. Or should they check both?

Not sure actually, I changed it now to check the value type. The main difference is that it looks better in the warnings, as far as I can say.

> In any case, checking the type for (un)equality seems wrong,
> the code should use g_type_is_a() so derived param specs
> will work too.

Good point, changed that.

ping, what's the status of this?

int64/uint64 would be great to have too, since they're used quite a bit in GStreamer.

Any chance we could get corresponding setters as well? (For better type-safety and to avoid the usual problems with vararg functions, esp. if there are int64 properties)

If you're using int64 and uint64 a lot I think we can include them, it's trivial to add them. I mainly intended to keep the patch small and handy for review. So I would suggest to do additional types in a separate patch.
Maybe g_object_get_generic should be public so that it's easy to provide type-safe accessors without including a function for every single type in Glib?

I hadn't considered setters, that seems like a natural fit. If I see a green flag for the getters that might well motivate me to provide a patch for setters :)

Some comments on the patch:

 - Shouldn't the return type of g_object_get_object() be GObject* instead of gpointer?  Or does that lead to all kinds of annoying casting warnings?  (Which is IIRC the reason why most GObject methods have a gpointer argument for the instance).
 
 - I agree with adding int64 and uint64.  In fact, I think that there should be a convenience getter for each supported type by GParamSpec.  gparamspecs.h contains this list, showing much more useful types that can be supported: int64, uint64, uchar, unichar, enum, etc.

 - I think there should be setters as well. (Mitch: wasn't that part of the original proposal?)

Created attachment 141952 [details] [review]
With setters, since and more types

(In reply to comment #8)
>  - Shouldn't the return type of g_object_get_object() be GObject* instead of
> gpointer?  Or does that lead to all kinds of annoying casting warnings?  (Which
> is IIRC the reason why most GObject methods have a gpointer argument for the
> instance).

You want to use MyType* there rather than GObject*, so asking for GObject* would lead to ugly casting.

>  - I agree with adding int64 and uint64.  In fact, I think that there should be
> a convenience getter for each supported type by GParamSpec.  gparamspecs.h
> contains this list, showing much more useful types that can be supported:
> int64, uint64, uchar, unichar, enum, etc.

uchar is no fundamental type, neither is uint32. We'd have to add that if you find that useful.

I take it you prefer a larger patch with everything, so I enhanced it:

- Added int64, uint64, enum, float, uchar, long, ulong, pointer accessors
- Added Since tags
- Added g_object_set_int, _double, _enum, _string, _object, _pointer, _flags, _enum

Kris, can this get a review please?

(In reply to comment #9)
> You want to use MyType* there rather than GObject*, so asking for GObject*
> would lead to ugly casting.

Agreed.

> uchar is no fundamental type, neither is uint32. We'd have to add that if you
> find that useful.

According to gobject/gtype.h UCHAR is a fundamental type.  uint32 is indeed not explicitly listed.  Nevertheless, I did not look at whether the type was fundamental but whether the type has a GParamSpec that supports it.  Since it is the GParamSpecs that decide which value types can be held as object properties which will be accessed using these accessors.


(In reply to comment #10)
> Kris, can this get a review please?

I've had my say earlier already, IMHO Mitch needs to review it now.  Possibly followed by a GLib maintainer if Mitch does not consider himself to be one ;)

The patch looks good to me, but as Kris said, it should include all
types which by default can live in GTypes and be object properties.

I miss unichar, param, value array and gtype.

Err ...which can live in GValues...

Created attachment 148809 [details] [review]
With GType, GParamSpec, unichar and updated Since tags

I updated the patch, it includes GType, GParamSpec and unichar now. And I updated the Since tags.

Do we actually want to add specific subtypes like GValueArray, which are simply boxed types? Then of course there is GString, GDate, GRegex and GValue as well. Presumably in that case we would want to include all of these.

(In reply to comment #14)
> Created an attachment (id=148809) [details] [review]
> With GType, GParamSpec, unichar and updated Since tags

Any idea who could review this?

I'm not sure I agree with the premise of this API extension.

Anywhere that we lose access to a object property because of sealing or 'priv'isation we should be adding a normal C API accessor function.  GObject properties are slow and, as noted, are annoying to use.  They also lack static type safety.

imho, GObject properties should only be used where programmatic access to the properties is required (things like GtkCellRenderer, object construction, introspection, etc).  Normal C programming should really prefer to say:

  x = some_object_get_foo (obj);

over:

  g_object_get (obj, "foo", &x);

or even:

  x = g_object_get_int (obj, "foo");


For this reason, I really doubt the usefulness of a patch designed to save typing in this way....

In GStreamer the main / only way to interact with plugins is via the GObject (and GStreamer) API. There are no header files or exported symbols other than a magic plugin struct, so accessor functions. are not possible. Run this for example:

 $ gst-inspect-0.10 playbin2

to get an idea. GObject seems the natural place for these accessors. I'm sure there are other use cases as well.

(In reply to comment #16)
> They also lack static type safety.

This is exactly what these accessors are intended to solve. You would be required to use _get_int if you need to use an integer property.

punting to 2.26

(In reply to comment #17)
>                      There are no header files or exported symbols other than a
> magic plugin struct, so accessor functions. are not possible.

Are there places where you're writing substantial amounts of C code that is directly interacting with those properties by a specific name?  How do you know that that name will be available?


(In reply to comment #18)
> This is exactly what these accessors are intended to solve. You would be
> required to use _get_int if you need to use an integer property.

what's to prevent me from using _get_int() with a property that's (say) a GObject?

(In reply to comment #20)
> (In reply to comment #18)
> > This is exactly what these accessors are intended to solve. You would be
> > required to use _get_int if you need to use an integer property.
> 
> what's to prevent me from using _get_int() with a property that's (say) a
> GObject?

Calling 'g_object_get_int (widget, "name")' will print a warning and return 0.

This is comparable to doing 'gtk_window_get_role (GTK_WINDOW (label))' which would print that label is a GtkLabel, not a GtkWindow and return NULL.

> How do you know that that name will be available?

Just like you know that GtkButton has a "clicked" signal that you can g_signal_connect() to: the property names and types and signals are part of the API/ABI and documented (exception: gst-plugins-bad, but that's irrelevant here).


> what's to prevent me from using _get_int() with a property that's (say) a GObject?

Nothing. That's a programming mistake, just like calling g_variant_get_boolean() on a GVariant that doesn't contain a boolean.


> punting to 2.26

Really? I think we're overthinking this a little..

(In reply to comment #21)
> Calling 'g_object_get_int (widget, "name")' will print a warning and return 0.

As I said, it turns what used to be a compile-time static check into a runtime check.  If that code path doesn't execute during your testing then you release buggy software.

> This is comparable to doing 'gtk_window_get_role (GTK_WINDOW (label))' which
> would print that label is a GtkLabel, not a GtkWindow and return NULL.

Yes.  We've always had this problem due to lack of proper upcasting in C.  Not much we can do about that except to avoid making the problem worse by expanding it to new domains...

(In reply to comment #22)
> Just like you know that GtkButton has a "clicked" signal that you can
> g_signal_connect() to: the property names and types and signals are part of the
> API/ABI and documented (exception: gst-plugins-bad, but that's irrelevant
> here).

I actually consider this to be a sore part of GObject, as well.  I'd prefer if we had a type-safe signal connection mechanism that didn't involve string names and use of G_CALLBACK() -- for exactly the same reasons.

Totally another discussion. :)

> Nothing. That's a programming mistake, just like calling
> g_variant_get_boolean() on a GVariant that doesn't contain a boolean.

That's a fair comparison except that when handed a GVariant* you're not sure what's inside of it.  When you're handed a GtkLabel* you are capable of being sure of two things:

  - gtk_label_get_text() will work on it
  - it will return a string

If I try to say:

  int x = gtk_label_get_txt (label);

then I get to find out about my mistakes.

If instead I try to go for this sort of feel:

  - g_object_get_int (label, "txt");

the compiler will fail to notice (a) it's a property that doesn't exist and (b) it's the wrong type.


We may as well have as much type safety as possible.


> > punting to 2.26
> 
> Really? I think we're overthinking this a little..

2.24.0 is happening *really* *really* soon.  It's basically a done deal at this point.  I punted a couple of my bugs, too -- for more trivial things.



I'm not saying that this API is totally useless.  My main concern is that having this API will encourage people to use it in inappropriate ways.  That's what I want to avoid.  People should always favour exposing proper C API accessors and using them instead of using this nice new GObject-properties API as a crutch.  Specifically, the claim in the initial report that we should use this as a replacement for structure member visibility is very much a bad idea.

*** Bug 597669 has been marked as a duplicate of this bug. ***

We decided at the hackfest that we don't want to do this.