GNOME Bugzilla – Bug 709865
Add boxing to GParameter
Last modified: 2018-01-10 20:33:19 UTC
Created attachment 256949 [details] Reproduces the reported issue When executing the attached example, assignment fails with: TypeError: cannot set a structure which has no well-defined ownership transfer rules. Simon says boxing the values would do the trick, I guess he'll better placed than I am to comment.
Created attachment 258887 [details] [review] Box GParameter to make it introspectable This seems to make creating a PeasExtensionSet from pygobject possible using the GParameter base API. Your test will of course still fail because setting the values directly won't work.
Moving to GLib. The patch looks fine to me but I am not a GLib reviewer.
Is there a real usecase that actually requires GParameter to be boxed? Maybe libpeas could stop having it on its public API? This seems a bit ... contorted.
The other use case seems to be Farstream as Oliver notes in bug 669535.
My use case in Farstream is that I want to be able to pass arbitrary construct only parameters to an object which comes from a plug-in. So quite similar to what libpeas does I suppose. My current workaround is to have an extra API with a GHashTable of string->GValue which is even uglier.
This also prevents calling g_object_new() with GParameters in Python, which I would love to use in libpeas' Python plugin loader (now partially written in Python). It seems odd that boxing this type is undesired, libpeas is using it for the intended purpose of setting properties and was chosen because GObject uses it the same way. I wouldn't call using the type for the intended purpose "contorted" but instead an attempt for consistency. I had also hoped that potential overrides in the bindings would be a possibility and shared between all users of the API.
If the API uses GParameter, then bindings users would have to write something like: Foo.Whatever ([ new GObject.Parameter ('height', 5), new GObject.Parameter ('visible', true), new GObject.Parameter ('parent', someWidget) ]) But if you have it take a GHashTable which is annotated (element-type utf8 GLib.Value), then they can just do: Foo.Whatever ({ height: 5, visible: true, parent: someWidget }) because bindings should already know how to make that conversion.
Except the point of this bug is that the API already exists, the GHashTable would require an extra round trip by libpeas to turn it into GParameters anyways (required for g_object_new) and the GHashTable isn't typesafe which is nice. The fact is that people are trying to use GParameter for the the intended purpose and the same one GObject itself promotes. Thus, it should thus be usable by bindings. Also, changing libpeas to use a GHashTable isn't going to fix GObject.Object.new() in PyGObject. Which would rather useful from the standpoint of libpeas and implementing more of the plugin loader in Python. Honestly, I don't really care if the usage from the bindings looks like hell. I can always ship overrides to clean it up or the bindings could automatically go from a dictionary to GParameter. But at least there they can check that everything has the correct types.
(In reply to Garrett Regier from comment #8) > The fact is that people are trying to use GParameter for the the intended > purpose and the same one GObject itself promotes. I don't think GObject "promotes" it. Yes, the API is there, and no, it has not been formally deprecated, but I don't think anyone considers it an example of the right way to do things any more. Eg, no other API in glib has ever used GParameter; even ones like g_object_set() which are effectively identical to g_object_newv().
I'd really like a g_object_setv() API that is usable from bindings, so that we'd have a way to "atomically" set a number of properties, and only apply the result in the dispatch_properties_changed() vfunc. In Farstream, I ended up adding a GHashTable variant of the API, but it's quite ugly for the reasons Garrett mentionned.
(In reply to Dan Winship from comment #9) > (In reply to Garrett Regier from comment #8) > > The fact is that people are trying to use GParameter for the the intended > > purpose and the same one GObject itself promotes. > > I don't think GObject "promotes" it. Yes, the API is there, and no, it has > not been formally deprecated, but I don't think anyone considers it an > example of the right way to do things any more. Eg, no other API in glib has > ever used GParameter; even ones like g_object_set() which are effectively > identical to g_object_newv(). g_object_set() doesn't use GParameter because it is convenient API for C users. g_object_set_property() also doesn't use it because it only works on a single GParameter, however, interestingly enough the two arguments it takes are actually the same two fields in GParameter. Also, there is in fact other API in glib that does use GParameter, the following is list of various places thave I've found GParameter. Do not that in general it only makes sense when dealing with multiple GObject properties and the construction of GObjects. g_initable_newv(): https://developer.gnome.org/gio/stable/GInitable.html#g-initable-newv g_async_initable_newv_async(): https://developer.gnome.org/gio/stable/GAsyncInitable.html#g-async-initable-newv-async GtkBuilder uses it internally for creating the object and setting various properties after construction: https://git.gnome.org/browse/gtk+/tree/gtk/gtkbuilder.c#n706 https://git.gnome.org/browse/gtk+/tree/gtk/gtkbuilder.c#n745 PyGObject uses it for constructing objects: https://git.gnome.org/browse/pygobject/tree/gi/gobjectmodule.c#n977 https://git.gnome.org/browse/pygobject/tree/gi/gobjectmodule.c#n1367 In fact PyGObject has a helper function to turn a dict into an array of GParameters, which means that it should be pretty easy to add an override for APIs that take GParameter arrays: https://git.gnome.org/browse/pygobject/tree/gi/pygobject.c#n1192 libgit2-glib uses has to go through the GParameter dance when using the object factory: https://git.gnome.org/browse/libgit2-glib/tree/libgit2-glib/ggit-object-factory.c#n237 GIMP has a utility function for constructing a GParameter array: https://git.gnome.org/browse/gimp/tree/app/core/gimp-utils.h#n41 Which is then used to construct an object using various properties via a GParameter array: https://git.gnome.org/browse/gimp/tree/app/core/gimpimage-undo.c#n387 https://git.gnome.org/browse/gimp/tree/app/gui/gui-vtable.c#n503 https://git.gnome.org/browse/gimp/tree/libgimpconfig/gimpconfig-iface.c#n150 All of these places have the same thing in common, they are constructing a GObject and potentially specifying multiple properties during construction and sometimes after. While this kind of usage is certainly a niche, it is still there and has a defined reason for its usage.
(In reply to Olivier Crête from comment #10) > I'd really like a g_object_setv() API that is usable from bindings, so that > we'd have a way to "atomically" set a number of properties, and only apply > the result in the dispatch_properties_changed() vfunc. You can achieve the same effect by first calling g_object_freeze_notify()[1], setting each property individually and then calling g_object_thaw_notify(). This will prevent the dispatch_properties_changed() vfunc from being called[2]. Also if you are using PyGObject, there is a special override for freeze_notify()/thaw_notify()[3] that allows it to be use with the 'with' statement. 1. https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#g-object-freeze-notify 2. https://git.gnome.org/browse/glib/tree/gobject/gobject.c#n1126 3. https://git.gnome.org/browse/pygobject/tree/gi/overrides/GObject.py#n605
I don't think any of the GLib maintainers want to see GParameter gain wider exposure.
(In reply to Matthias Clasen from comment #13) > I don't think any of the GLib maintainers want to see GParameter gain wider > exposure. Right. OK, so I think the "recommended" way for these APIs to work then would be to have a g_object_new()-like API for C users, and a GHashTable(utf8,GValue)-based API for bindings users, with a rename-to annotation to the name of the C version. (This possibly implies that we should have hash-table-based versions of these existing APIs, but I think we expect that GObject, GInitable, etc, will be wrapped manually anyway, so it doesn't really matter.) Or maybe we could have some way of annotating g_object_set(), g_object_new(), g_initable_new() directly.
From another point of view, bindings try do as much of the work as possible in the language being bound. Because GParameter is not wrapped it is required to manually write wrappers for that API. By forcing the API to be manually wrapped it is enforcing the need to wrap API and in turn more and more of the API is wrapped manually. A major goal for mature language bindings is to only manually wrap GIRepository and the marshalling functions. For instance, PyGObject has working on reducing the need to manually wrap various parts of the glib/GObject API. This is just another instance of that issue. --- If GParameter will not be wrapped then I request another API by which a GObject can be created using a non-variadic function and continue being type safe. All instance of GParameter use I've found have all involved this usage and the API exposed by them is a reflection of this. In the end I would like to keep the libpeas API consistent with GObject and avoid adding a type which is basically a GBoxed duplicate of GParameter.
(In reply to Garrett Regier from comment #11) > (In reply to Dan Winship from comment #9) > > (In reply to Garrett Regier from comment #8) > > > The fact is that people are trying to use GParameter for the the intended > > > purpose and the same one GObject itself promotes. > > > > I don't think GObject "promotes" it. Yes, the API is there, and no, it has > > not been formally deprecated, but I don't think anyone considers it an > > example of the right way to do things any more. Eg, no other API in glib has > > ever used GParameter; even ones like g_object_set() which are effectively > > identical to g_object_newv(). > > g_object_set() doesn't use GParameter because it is convenient API for C > users. g_object_set_property() also doesn't use it because it only works on > a single GParameter No, it does not use it because GParameter is a useless type that sadly survived when the type system landed in GLib. Originally, GParameter was meant to be used for newv(), setv(), and getv() variants; that would at least have had more sense than the current oddity. The vector-based set() and get() never materialized, and newv() was not fixed to drop the needless boxing of the name/value pairs. > however, interestingly enough the two arguments it takes are actually > the same two fields in GParameter. > > Also, there is in fact other API in glib that does use GParameter, the > following is list of various places thave I've found GParameter. Do not that > in general it only makes sense when dealing with multiple GObject properties > and the construction of GObjects. You're constructing your argument backwards. The only reason these other projects are using GParameter is because it's the only existing way to handle this stuff, not because it's convenient — even from a C developer perspective GParameter is just a *bad* *type*. It's an auxiliary type that does not match any other vector-based API we have because we realized how bad it is after its introduction and after the API was set in stone. Every other vector-based variant that takes a key/value pair uses two vectors of the same length, and a length argument — see GtkListStore, the Clutter animation API, and countless of other examples. The proper solution is, as you identified it in comment #15, to add a g_object_setv(), g_object_getv(), and a g_object_newv2() (or whatever name we choose) that uses the same pattern that was consolidated through the years, i.e.: gpointer g_object_newv2 (GType gtype, guint n_properties, const char *names[], const GValue values[]); After that, we will need to deprecate GParameter and its only user in the GObject code base. (In reply to Dan Winship from comment #14) > (In reply to Matthias Clasen from comment #13) > > I don't think any of the GLib maintainers want to see GParameter gain wider > > exposure. > > Right. OK, so I think the "recommended" way for these APIs to work then > would be to have a g_object_new()-like API for C users, and a > GHashTable(utf8,GValue)-based API for bindings users, with a rename-to > annotation to the name of the C version. We don't really need an hash table variant of the API. Language bindings can cope with name/value arrays of the same length perfectly fine.
(In reply to Emmanuele Bassi (:ebassi) from comment #16) > > Right. OK, so I think the "recommended" way for these APIs to work then > > would be to have a g_object_new()-like API for C users, and a > > GHashTable(utf8,GValue)-based API for bindings users, with a rename-to > > annotation to the name of the C version. > > We don't really need an hash table variant of the API. Language bindings can > cope with name/value arrays of the same length perfectly fine. That's awkward for callers though; they have to manually match everything up, and re-sync the lists if they remove an item, etc. And in the case of gjs at least, object constructors already work as though they take a GHashTable(utf8,GValue), so new APIs written that way would therefore be familiar-looking.
(In reply to Dan Winship from comment #17) > And in the case of gjs at least, object constructors already work as though > they take a GHashTable(utf8,GValue), so new APIs written that way would > therefore be familiar-looking. Creating GObjects is already slow. Do we want to add the creation and destruction of a GHashTable to the overhead from bindings (which involves a second managed hashtable and various marshaling)? And lose type safety from the compiler standpoint at the same time? I know it's not ideal, but some improperly written objects in the wild are dependent on property ordering. Not ideal, but they exist in third party code and we shouldn't break it. So we'd have to be careful to iterate the hashtable in order or entry. Is that possible today with GHashTable? > That's awkward for callers though; they have to manually match everything > up, and re-sync the lists if they remove an item, etc. If Emmanuele's example took an array of GValue* instead of GValueArray, you could do a trick where you use a GValueArray internally for storing values and just update an indirection array for the pointers to values. That should simplify the "remove an item" case, but I don't actually know where that happens in the wild.
(In reply to Dan Winship from comment #17) > (In reply to Emmanuele Bassi (:ebassi) from comment #16) > > > Right. OK, so I think the "recommended" way for these APIs to work then > > > would be to have a g_object_new()-like API for C users, and a > > > GHashTable(utf8,GValue)-based API for bindings users, with a rename-to > > > annotation to the name of the C version. > > > > We don't really need an hash table variant of the API. Language bindings can > > cope with name/value arrays of the same length perfectly fine. > > That's awkward for callers though; they have to manually match everything > up, and re-sync the lists if they remove an item, etc. That's a non-issue: language bindings construct the C arguments when calling the function. Before that, they can use their own data structures. C-level API can use GArray to construct the arrays of strings and GValues. > And in the case of gjs at least, object constructors already work as though > they take a GHashTable(utf8,GValue), so new APIs written that way would > therefore be familiar-looking. That would kill the ordering of properties, which is guaranteed (and we had to fix GObject to guarantee as well).
Created attachment 344281 [details] [review] Deprecate GParameter. Add g_object_newv2, g_object_getv and g_object_getv. Please, review it.
Created attachment 344282 [details] [review] tests: Add test for gobject properties for g_object_newv/setv/getv Review it. Depends on "344281: Deprecate GParameter. Add g_object_newv2, g_object_getv and g_object_getv."
Created attachment 344284 [details] [review] [pygobject] Remove GParameter references This is for PyGOBject and depends on 344281: Deprecate GParameter. Add g_object_newv2, g_object_getv and g_object_getv.
Review of attachment 344281 [details] [review]: ::: gio/gasyncinitable.c @@ +364,3 @@ * * Since: 2.22 + * Deprecated: 2.52: Use g_object_new() followed by g_async_initable_new_async() instead. Add some reasoning as to why. ::: gio/ginitable.c @@ +169,3 @@ * * Since: 2.22 + * Deprecated: 2.52: Use g_object_new() followed by g_initable_init() instead. Add some reasoning as to why. ::: gobject/gobject.c @@ +1886,3 @@ +/** + * g_object_newv2: (rename-to g_object_new) + * @object_type: the type id of the #GObject subtype to instantiate Nitpick: s/type id/type ID/ @@ +1893,3 @@ + * Creates a new instance of a #GObject subtype and sets its properties. + * + * Construction parameters (see #G_PARAM_CONSTRUCT, #G_PARAM_CONSTRUCT_ONLY) IIRC the gtk-doc syntax for enum members is `%G_PARAM_CONSTRUCT`, not `#G_PARAM_CONSTRUCT`. https://developer.gnome.org/gtk-doc-manual/unstable/documenting_syntax.html.en#documenting_syntax @@ +1897,3 @@ + * + * Returns: (type GObject.Object) (transfer full): a new instance of + * @object_type Needs a ‘Since: $version’ line. @@ +1905,3 @@ + const GValue values[]) +{ + GObjectClass *class, *unref_class = 0; `unref_class = NULL` @@ +1966,3 @@ * @object_type + * + * Deprecated: 2.52: Use g_object_newv2() instead. Would be good to include some reasoning about why it should be used. Also, I think the (skip) (rename-to) and Deprecated annotation changes should probably be done in a separate commit, since they’re not really integral to introducing the new methods. @@ +2163,3 @@ + const char *property_name) +{ + if (!pspec) As a separate commit, all these conditions could be annotated with G_UNLIKELY() for a tiny speedup. @@ +2193,3 @@ + * @n_properties: the number of properties + * @names: (array length=n_properties): an array of strings + * @values: (array length=n_properties): an array of #GValue set to #G_VALUE_INIT Presumably they should be set to the value to be passed into the object, not `G_VALUE_INIT`? @@ +2195,3 @@ + * @values: (array length=n_properties): an array of #GValue set to #G_VALUE_INIT + * + * Sets properties on an object. As with my comment below on g_object_getv(), this documentation needs more detail and a ‘Since: $version’ line. @@ +2214,3 @@ + { + if (names[n] == NULL || !G_IS_VALUE (&values[n])) + break; What’s the purpose of this check? We have n_properties. @@ +2218,3 @@ + pspec = g_param_spec_pool_lookup (pspec_pool, + names[n], + G_OBJECT_TYPE (object), `G_OBJECT_TYPE (object)` could be factored out of the loop for a tiny speedup. @@ +2221,3 @@ + TRUE); + + if (g_object_set_is_valid_property (object, pspec, names[n])) Would be nice if this condition was inverted and you used `break` if it failed, so it’s symmetric with the implementation in g_object_getv(). @@ +2291,3 @@ +static gboolean +g_object_get_is_valid_property (GObject *object, It would be clearer if you factored out the `*is_valid_property()` functions like this in a separate commit first. @@ +2295,3 @@ + const char *property_name) +{ + if (!pspec) As a separate commit, this (and the check below) could have a `G_UNLIKELY()` added for a potential slight speedup. @@ +2319,3 @@ + * @n_properties: the number of properties + * @names: (array length=n_properties): an array of strings + * @values: (array length=n_properties): an array of #GValue set to #G_VALUE_INIT Do they have to be set to `G_VALUE_INIT`? You clear them to 0 with memset() below. @@ +2322,3 @@ + * + * Gets properties of an object. + * Documentation needs a bit more detail about how to use the function. For example, @names and @values have to be the same length. What happens if @names includes an invalid property name? What happens if @n_properties is 0? Needs a ‘Since: $version’ line too. (See: https://developer.gnome.org/gtk-doc-manual/unstable/documenting_symbols.html.en#documenting_symbols.) @@ +2341,3 @@ + pspec = g_param_spec_pool_lookup (pspec_pool, + names[n], + G_OBJECT_TYPE (object), You could cache the result of `G_OBJECT_TYPE (object)` for a tiny performance improvement. ::: gobject/gparam.h @@ +270,3 @@ * to hand parameter name/value pairs to g_object_newv(). + * + * Deprecated: 2.52: This type will be removed. Add some reasoning as to why.
Review of attachment 344282 [details] [review]: This looks good. A few other tests which could be added: - Calling each of the functions with (n_properties == 0) to check they’re a no-op (or check the fast path on g_object_newv2()). - Calling each of the functions with invalid property names/types/flags (basically, one test for each of the branches in g_object_*_is_valid_property()) to check the error handling. See g_test_trap_subprocess(). - Test calling g_object_getv() with initialised and uninitialised GValues, since both are allowed. - Check GObject::notify emissions as a result of calling g_object_setv(). ::: gobject/tests/properties.c @@ +399,3 @@ + g_assert (g_strcmp0 (g_value_get_string (&values2[3]), "where you live") == 0); + + g_object_unref (test_obj); The GValues all need to be unset here, otherwise they leak. Would be good to run this test under valgrind quickly to double-check there are no other leaks.
Review of attachment 344284 [details] [review]: I’m not a pygobject maintainer, so this is only a cursory review with a few comments. ::: gi/gobjectmodule.c @@ +1348,3 @@ GObjectClass *class; + guint n_props = 0, i; + const gchar **prop_names; Initialise this to `NULL` or the failure path from `pygobject_prepare_construct_properties()` will call g_free() on it while it’s uninitialised. ::: gi/pygobject-object.c @@ +1226,3 @@ while (PyDict_Next(kwargs, &pos, &key, &value)) { GParamSpec *pspec; + (*prop_names)[*n_props] = PYGLIB_PyUnicode_AsString(key); Would probably be clearer to assign `(*prop_names)[*n_props]` to a local variable. @@ +1257,3 @@ + guint n_props = 0, i; + const gchar **prop_names; + GValue *values; Initialise these to `NULL` otherwise they will be uninitialised when g_free() is called on them in the error path from `pygobject_prepare_construct_properties()`. (Actually, pygobject_prepare_construct_properties() initialises them to NULL unconditionally (even on the error path), but I don’t think that’s a safe thing to assume in general.)
Review of attachment 258887 [details] [review]: It's pretty clear we're not going to do this.
Review of attachment 344281 [details] [review]: Thanks for your patch! Please, split off the patch into at least these separate commits: 1. commit that adds g_object_newv() replacement 2. commit that adds g_object_setv() and g_object_getv() 3. commit that deprecates the g_object_newv()/GParameter users in GIO 3. commit that deprecates g_object_newv() 4. commit that deprecates GParameter This allows easier incremental reviews. ::: gio/gasyncinitable.h @@ +96,3 @@ const gchar *first_property_name, ...); +GLIB_DEPRECATED_IN_2_52 Use: GLIB_DEPRECATED_IN_2_52_FOR(g_object_newv2 and g_async_initable_init()) ::: gio/ginitable.h @@ +82,3 @@ ...); + +GLIB_DEPRECATED_IN_2_52 Use: GLIB_DEPRECATED_IN_2_52_FOR(g_object_newv2 and g_initable_init) ::: gobject/gobject.c @@ +1846,3 @@ + +static gboolean +g_object_new_is_valid_property (GType object_type, This function ought to be inlined. @@ +1853,3 @@ +{ + gint i; + const char *name, Coding style: G_UNLIKELY/G_LIKELY go inside paretheses, not outside — i.e. if (G_UNLIKELY (!pspec)) Also, NULL comparisons should be explicit: if ( G_UNLIKELY (pspec != NULL))) @@ +1870,3 @@ + if (pspec->flags & (G_PARAM_CONSTRUCT | G_PARAM_CONSTRUCT_ONLY)) + { + G_STRFUNC, g_type_name (object_type), name); Coding style: please put curly braces around the for() body: for (i = 0; i < n_params; i++) { if (…) … } @@ +1886,3 @@ +/** + * g_object_newv2: (rename-to g_object_new) + G_STRFUNC, pspec->name, g_type_name (object_type)); "the type of the subtype" is a bit redundant. @object_type: the object type to instantiate @@ +1889,3 @@ + * @n_properties: the number of properties + * @names: (array length=n_properties): an array of strings + } @names: an array of property names @values: an array of property values We already know the types of @names and @values in the API reference, there's no need to repeat them. @@ +1891,3 @@ + * @values: (array length=n_properties): an array of #GValue + * + "and sets its properties using the provided arrays." @@ +1918,3 @@ + class = unref_class = g_type_class_ref (object_type); + + return TRUE; Please, invert the check. if (n_properties == 0) { … return; } and just declare `GObjectConstructParam *params` and `int i, count` at the beginning of the function. @@ +1921,3 @@ + { + gint n, count = 0; + This is a case of using C99 VLAs, which is not going to work everywhere (e.g. on MSVC). I think we're going to need to use `g_newa()`: GObjectConstructParam *params; params = g_newa (GObjectConstructParam, n_properties); @@ +1954,2 @@ /** + * g_object_newv: (skip) You don't need `(skip)` if you're using `(rename-to)`. @@ +2001,3 @@ pspec = g_param_spec_pool_lookup (pspec_pool, parameters[i].name, object_type, TRUE); + if (!g_object_new_is_valid_property (object_type, pspec, parameters[i].name, + cparams, j)) Coding style: don't break the line. @@ +2071,3 @@ + if (!g_object_new_is_valid_property (object_type, pspec, name, params, + n_params)) Coding style: same as above. ::: gobject/gobject.h @@ +422,3 @@ ...); +GLIB_AVAILABLE_IN_2_52 +gpointer g_object_newv2 (GType object_type, I'd probably call it: g_object_new_with_properties() because g_object_newv2() is kind of lame.
Created attachment 344398 [details] [review] gobject: Add helper functions to handle warnings in g_object_new/set/get Review it, please. Tell me if I missed something.
Created attachment 344399 [details] [review] gobject: Add g_object_new_with_properties Review it, please.
Created attachment 344400 [details] [review] gobject: Add g_object_setv and g_object_getv functions
Created attachment 344401 [details] [review] tests: Add test for gobject properties for g_object_newv/setv/getv
Created attachment 344402 [details] [review] gobject: Deprecate GParameter
Created attachment 344403 [details] [review] gio: Deprecate GParameter-related functions
Created attachment 344404 [details] [review] gobject: Deprecate g_object_newv
Created attachment 344405 [details] [review] [pygobject] Remove GParameter references
Review my patches, please. I have done what ebassi and Philip Withnall told me to do. Tell me if I forgot something. ... I have a doubt, btw. I have tried this ---------------------------------------------- fabian@fabian-ubuntu:~/Documents/.trash$ cat main2.py import gi gi.require_version('Gtk', '4.0') from gi.repository import GObject from gi.repository import Gtk obj = GObject.Object.new_with_properties(Gtk.Button, 1, ["label"], ["text"]) fabian@fabian-ubuntu:~/Documents/.trash$ jhbuild run python3 main2.py Gtk-Message: Failed to load module "unity-gtk-module" Gtk-Message: Failed to load module "canberra-gtk-module" Gtk-Message: Failed to load module "canberra-gtk-module" Traceback (most recent call last):
+ Trace 237085
obj = GObject.Object.new_with_properties(Gtk.Button, 1, ["label"], ["text"])
fabian@fabian-ubuntu:~/Documents/.trash$ ---------------------------------------------- Although, this is not the typical way to instantiate in Python, I don't know why it is expecting a string. I have tried to add to g_object_new_with_properties function the annotation (element-type gchar*), (element-type utf8)... I have tried to change "const gchar *names[]" to "const gchar **names", but I see this in all these cases I get: "TypeError: Must be string, not list". Any idea, why?
Review of attachment 344398 [details] [review]: ::: gobject/gobject.c @@ +1861,3 @@ + } + + if G_UNLIKELY (~pspec->flags & G_PARAM_WRITABLE) Missing brackets around the if-condition. @@ +1868,3 @@ + } + + if (pspec->flags & (G_PARAM_CONSTRUCT | G_PARAM_CONSTRUCT_ONLY)) You could add G_UNLIKELY to all of these tests, really. @@ +2092,3 @@ + const char *property_name) +{ + if (!pspec) Explicit NULL comparisons please: `if (pspec == NULL)`. This could also use G_UNLIKELY(). @@ +2097,3 @@ + G_STRFUNC, + G_OBJECT_TYPE_NAME (object), + property_name); Indentation seems to have been broken here.
Review of attachment 344399 [details] [review]: Your commit message could do with a bit of explanation about why this function is being added, to save people having to open up and read the entire bug report if they’re browsing through the git logs. ::: gobject/gobject.c @@ +1887,3 @@ + +/** + * g_object_new_with_properties: (rename-to g_object_new) Is this (rename-to) annotation not going to conflict with the `g_object_newv: (rename-to g_object_new)` immediately below it? Maybe add this annotation in the “Deprecate g_object_newv” patch instead. @@ +1891,3 @@ + * @n_properties: the number of properties + * @names: (array length=n_properties): an array of property names + * @values: (array length=n_properties): an array of property values Emmanuele already commented about this: drop the “an array of”, since that’s explicit in the argument’s type. @@ +1893,3 @@ + * @values: (array length=n_properties): an array of property values + * + * Creates a new instance of a #GObject subtype and sets its properties using the provided arrays. You should specify how the arrays are used: both arrays must have exactly n_properties elements, and the names and values correspond by index. @@ +1924,3 @@ + if (n_properties > 0) + { + gint n, count = 0; Use guint: these should never be negative. Also, I’d use `i` rather than `n`, since `n` is typically used for the *n*umber of things (i.e. a total). @@ +1952,3 @@ + object = g_object_new_internal (class, NULL, 0); + + if (unref_class) Explicit NULL comparisons please: `if (unref_class != NULL)`. If someone makes a review comment like this, please apply it uniformly across the whole patch (or patch series), not just in the one location they pointed out. :-)
Review of attachment 344400 [details] [review]: ::: gobject/gobject.c @@ +2195,3 @@ + * @n_properties: the number of properties + * @names: (array length=n_properties): an array of property names + * @values: (array length=n_properties): an array of initialized values Same comment as from previous reviews: drop “an array of”. @@ +2199,3 @@ + * Sets @n_properties properties for an @object. + * Set properties will be taken from @values. In the case of an invalid + * property, the rest of them will not be considered. > In the case of an invalid property, the rest of them will not be considered. That makes it sound like invalid properties are accepted, but they’re not: a warning will be emitted, and it’s a programming error for someone to pass an invalid property into this function. I’m not sure it’s useful to say that properties after an invalid one will not be considered; I think you should say something like “All properties must be valid; warnings will be emitted and undefined behaviour may result if invalid properties are passed in.” @@ +2209,3 @@ + const GValue values[]) +{ + guint n; Same comment as in the review above: I think you should use `i` here rather than `n`. @@ +2216,3 @@ + g_return_if_fail (G_IS_OBJECT (object)); + + g_object_ref (object); You could wrap all of this in `if (n_properties > 0)` to avoid touching the reference count in that case. @@ +2218,3 @@ + g_object_ref (object); + obj_type = G_OBJECT_TYPE (object); + nqueue = g_object_notify_queue_freeze (object, FALSE); If (n_properties == 1), I think we could avoid calling g_object_notify_queue_[freeze|thaw](), which would save some locks/unlocks. @@ +2222,3 @@ + { + if (names[n] == NULL || !G_IS_VALUE (&values[n])) + break; Shouldn’t this warn, just like g_object_set_is_valid_property() does? It’s a programming error if the caller passes in a names or values array which doesn’t match n_properties. @@ +2331,3 @@ + * Gets @n_properties properties for an @object. + * Obtained properties will be set to @values. In the case of an invalid + * property, the rest of them will not be considered. Same comments apply to this documentation comment as to the one above. @@ +2332,3 @@ + * Obtained properties will be set to @values. In the case of an invalid + * property, the rest of them will not be considered. + * Missing a `Since: 2.52` line. @@ +2336,3 @@ +void +g_object_getv (GObject *object, + const guint n_properties, `const guint`?? @@ +2340,3 @@ + GValue values[]) +{ + guint n; Use `i` rather than `n`. @@ +2345,3 @@ + + g_return_if_fail (G_IS_OBJECT (object)); + g_object_ref (object); As above, the bulk of this function could be wrapped in `if (n_properties > 0)` to avoid touching the object’s reference count in that corner case. @@ -2441,3 @@ - G_STRFUNC, - G_OBJECT_TYPE_NAME (object), - property_name); Shouldn’t all of this validity checking stuff have been factored out into g_object_set_is_valid_property() in the commit which introduced that function? ::: gobject/gobject.h @@ +461,3 @@ +GLIB_AVAILABLE_IN_2_52 +void g_object_getv (GObject *object, + const guint n_properties, `const guint` should just be `guint`.
Review of attachment 344401 [details] [review]: You haven’t applied any of the suggestions from comment #24.
Review of attachment 344402 [details] [review]: ::: gobject/gparam.h @@ +270,3 @@ + * + * Deprecated: 2.52: This type will be removed in a future version. + * Do not use it. It is not supported by bindings. It might be clearer to rephrase the final sentence as “It was removed because it was not introspectible.”?
Review of attachment 344403 [details] [review]: ::: gio/gasyncinitable.h @@ +96,3 @@ const gchar *first_property_name, ...); +GLIB_DEPRECATED_IN_2_52_FOR(g_object_new_with_properties and g_async_initable_init_async) I always thought the replacement-API-name here was supposed to be a single symbol — but GLIB_DEPRECATED_IN_*_FOR() boils down to G_DEPRECATED_FOR(), which stringifies its argument and uses it in an error message (rather than passing it to the compiler as a structured annotation), so this is probably OK. If G_DEPRECATED_FOR() changes in future, we can always change this.
Review of attachment 344404 [details] [review]: As with the rest of the commit messages in this patch series, the commit message here could be expanded to summarise why this change is necessary; rather than forcing people to read the entire bug report.
Review of attachment 344405 [details] [review]: I can’t see any problems with this, but it should be reviewed by a pygobject maintainer.
(In reply to César Fabián Orccón Chipana from comment #36) > Review my patches, please. I have done what ebassi and Philip Withnall told > me to do. Tell me if I forgot something. > ... > > I have a doubt, btw. I have tried this > *snip* What does the entry for g_object_new_with_properties() in the GObject-2.0.gir you’re using look like?
Created attachment 346660 [details] [review] gobject: Add helper functions to handle warnings in g_object_new/set/get
Created attachment 346661 [details] [review] gobject: Add g_object_new_with_properties
Created attachment 346662 [details] [review] gobject: Add g_object_setv and g_object_getv functions
Created attachment 346663 [details] [review] tests: Add test for gobject properties for g_object_newv/setv/getv
Created attachment 346664 [details] [review] gobject: Deprecate GParameter
Created attachment 346665 [details] [review] gio: Deprecate GParameter-related functions
Created attachment 346666 [details] [review] gobject: Deprecate g_object_newv
(In reply to Philip Withnall from comment #45) > (In reply to César Fabián Orccón Chipana from comment #36) > > Review my patches, please. I have done what ebassi and Philip Withnall told > > me to do. Tell me if I forgot something. > > ... > > > > I have a doubt, btw. I have tried this > > *snip* > > What does the entry for g_object_new_with_properties() in the > GObject-2.0.gir you’re using look like? <function name="new_with_properties" c:identifier="g_object_new_with_properties"> <return-value transfer-ownership="none" nullable="1"> <type name="gpointer" c:type="gpointer"/> </return-value> <parameters> <parameter name="object_type" transfer-ownership="none"> <type name="GType" c:type="GType"/> </parameter> <parameter name="n_properties" transfer-ownership="none"> <type name="guint" c:type="guint"/> </parameter> <parameter name="names" transfer-ownership="none"> <type name="utf8" c:type="const char*"/> </parameter> <parameter name="values" transfer-ownership="none"> <type name="Value" c:type="const GValue"/> </parameter> </parameters> </function>
Review of attachment 346660 [details] [review]: ::: gobject/gobject.c @@ +2108,3 @@ + return FALSE; + } + else if G_UNLIKELY (((pspec->flags & G_PARAM_CONSTRUCT_ONLY) && !object_in_construction (object))) Missing brackets around the if-condition here.
Review of attachment 346661 [details] [review]: ::: gobject/gobject.c @@ +1894,3 @@ + * + * Creates a new instance of a #GObject subtype and sets its properties using + * the provided arrays. Both arrays must have exactly n_properties elements, and Nitpick: if you use ‘@n_properties’ rather than ‘n_properties’, gtk-doc will format the parameter to make it more distinguishable from the surrounding text. @@ +1921,3 @@ + class = g_type_class_peek_static (object_type); + + if (!class) if (class == NULL) @@ +1930,3 @@ + + params = g_newa (GObjectConstructParam, n_properties); + for (i = 0; i < n_properties; i++) { GLib style is for the braces ({}) to be on a separate line. i.e. for (i = 0; i < n_properties; i++) { … } rather than for (i = 0; i < n_properties; i++) { … } Sorry about all this nitpicking, but we need to enforce a consistent coding style, otherwise the code base will become a pain to read.
Review of attachment 346662 [details] [review]: ::: gobject/gobject.c @@ +2224,3 @@ + g_object_ref (object); + obj_type = G_OBJECT_TYPE (object); + nqueue = g_object_notify_queue_freeze (object, FALSE); I’ll repeat my review comment from the previous review: > If (n_properties == 1), I think we could avoid calling g_object_notify_queue_[freeze|thaw](), which would save some locks/unlocks. @@ +2227,3 @@ + for (i = 0; i < n_properties; i++) + { + if (names[i] == NULL || !G_IS_VALUE (&values[i])) The code is checking `!G_IS_VALUE (&values[i])` twice: once here, and once in the check immediately below. Should the copy in this if-statement be removed? @@ +2341,3 @@ + * @n_properties: the number of properties + * @names: (array length=n_properties) (nullable): an array of property names + * @values: (array length=n_properties) (nullable): an array of values Drop “an array of”, as you have done for the other bits of documentation. Please check all across your patches when fixing review comments. It’s very boring to have to make the same review comment several times. :-( `git grep` is useful for this.
Review of attachment 346663 [details] [review]: Some minor comments, but overall I like these new tests. Thanks. :-) ::: gobject/tests/properties.c @@ +363,3 @@ + const char *prop_names[4] = { "foo", "bar", "baz", "quux" }; + GValue values_out[4] = { G_VALUE_INIT }; + gint i; guint i @@ +389,3 @@ + GValue values_in[4] = { G_VALUE_INIT }; + GValue values_out[4] = { G_VALUE_INIT }; + gint i; guint i @@ +450,3 @@ + test_obj = g_object_new_with_properties (test_object_get_type (), 1, + invalid_prop_names, values_in); + Spurious blank line before the `}`. @@ +457,3 @@ +} + + Spurious blank line (i.e. the code only needs one blank line between functions). @@ +500,3 @@ + GValue values_out_initialized[4] = { G_VALUE_INIT }; + GValue values_out_uninitialized[4] = { G_VALUE_INIT }; + gint i; guint i @@ +541,3 @@ + GValue values_in[3] = { G_VALUE_INIT }; + Notifys n; + gint i; guint i
Review of attachment 346664 [details] [review]: This looks OK to me. OK to commit once the rest of the patches are approved.
Review of attachment 346665 [details] [review]: This looks OK to me. OK to commit once the rest of the patches are approved.
Review of attachment 346666 [details] [review]: Shouldn’t you re-add the `(rename-to g_object_new)` annotation to g_object_new_with_properties() in this commit?
(In reply to César Fabián Orccón Chipana from comment #53) > (In reply to Philip Withnall from comment #45) > > (In reply to César Fabián Orccón Chipana from comment #36) > > > Review my patches, please. I have done what ebassi and Philip Withnall told > > > me to do. Tell me if I forgot something. > > > ... > > > > > > I have a doubt, btw. I have tried this > > > *snip* > > > > What does the entry for g_object_new_with_properties() in the > > GObject-2.0.gir you’re using look like? > … > <parameter name="names" transfer-ownership="none"> > <type name="utf8" c:type="const char*"/> > </parameter> > <parameter name="values" transfer-ownership="none"> > <type name="Value" c:type="const GValue"/> > </parameter> … These look wrong — neither of them are declared as an <array>. Try changing their declarations from + const char *names[], + const GValue values[]) to + const char **names, + const GValue *values) and see if that triggers different behaviour in g-ir-scanner.
(In reply to Philip Withnall from comment #57) > Review of attachment 346662 [details] [review] [review]: > > ::: gobject/gobject.c > @@ +2224,3 @@ > + g_object_ref (object); > + obj_type = G_OBJECT_TYPE (object); > + nqueue = g_object_notify_queue_freeze (object, FALSE); > > I’ll repeat my review comment from the previous review: > > If (n_properties == 1), I think we could avoid calling g_object_notify_queue_[freeze|thaw](), which would save some locks/unlocks. Taking a second look at this, I’m wrong. g_object_notify_queue_thaw() is necessary to emit the notify signal at all — nothing else in g_object_set_property() triggers it. I think I was getting confused between g_object_thaw_notify() and g_object_notify_queue_thaw(). :-)
Created attachment 347710 [details] [review] gobject: Add helper functions to handle warnings in g_object_new/set/get g_object_new_is_valid_property g_object_get_is_valid_property g_object_set_is_valid_property
Created attachment 347713 [details] [review] gobject: Add g_object_new_with_properties g_object_new_with_properties is an alternative to g_object_newv. The last one, takes an array of GParameter. However, GParameter is a rarely used type and this type is not introspectible, so it will not work properly in bindings.
Created attachment 347714 [details] [review] gobject: Add g_object_setv and g_object_getv functions
Created attachment 347715 [details] [review] gobject: Deprecate g_object_newv g_object_newv uses a GParameter as argument. Since GParameter is deprecated due to this type is not introspectible, g_object_newv is deprecated now.
Created attachment 347716 [details] [review] tests: Add test for gobject properties for g_object_newv/setv/getv
(In reply to Philip Withnall from comment #62) > > These look wrong — neither of them are declared as an <array>. Try changing > their declarations from > + const char *names[], > + const GValue values[]) > to > + const char **names, > + const GValue *values) > and see if that triggers different behaviour in g-ir-scanner. <function name="new_with_properties" c:identifier="g_object_new_with_properties"> <return-value transfer-ownership="none" nullable="1"> <type name="gpointer" c:type="gpointer"/> </return-value> <parameters> <parameter name="object_type" transfer-ownership="none"> <type name="GType" c:type="GType"/> </parameter> <parameter name="n_properties" transfer-ownership="none"> <type name="guint" c:type="guint"/> </parameter> <parameter name="names" transfer-ownership="none"> <type name="utf8" c:type="const char*"/> </parameter> <parameter name="values" transfer-ownership="none"> <type name="Value" c:type="const GValue"/> </parameter> </parameters> </function> I am not sure if this is a bug in gobject-introspection. But I use a similar prototype in one patch I am about to purpose in libpeas in https://github.com/cfoch/libpeas/commit/9dfa1d698f5e5bcc8fad5c6157d0d7eb32e71640#diff-fece5e7cf7302c46327f6fc997ea11f8R657 and the .gir file is generated as expected <constructor name="new_with_properties" c:identifier="peas_extension_set_new_with_properties" version="1.20.0"> <doc xml:space="preserve">Create a new #PeasExtensionSet for the @exten_type extension type. If @engine is %NULL, then the default engine will be used. See peas_extension_set_new() for more information.</doc> <return-value transfer-ownership="full"> <doc xml:space="preserve">a new instance of #PeasExtensionSet.</doc> <type name="ExtensionSet" c:type="PeasExtensionSet*"/> </return-value> <parameters> <parameter name="engine" transfer-ownership="none" nullable="1" allow-none="1"> <doc xml:space="preserve">A #PeasEngine, or %NULL.</doc> <type name="Engine" c:type="PeasEngine*"/> </parameter> <parameter name="exten_type" transfer-ownership="none"> <doc xml:space="preserve">the extension #GType.</doc> <type name="GType" c:type="GType"/> </parameter> <parameter name="n_properties" transfer-ownership="none"> <doc xml:space="preserve">the length of the @prop_names and @prop_values array.</doc> <type name="guint" c:type="guint"/> </parameter> <parameter name="prop_names" transfer-ownership="none"> <doc xml:space="preserve">an array of property names.</doc> <array length="2" zero-terminated="0" c:type="gchar*"> <type name="utf8" c:type="gchar"/> </array> </parameter> <parameter name="prop_values" transfer-ownership="none"> <doc xml:space="preserve">an array of property values.</doc> <array length="2" zero-terminated="0" c:type="GValue"> <type name="GObject.Value" c:type="GValue"/> </array> </parameter> </parameters> </constructor>
Created attachment 347720 [details] [review] gobject: Add g_object_new_with_properties g_object_new_with_properties is an alternative to g_object_newv. The last one, takes an array of GParameter. However, GParameter is a rarely used type and this type is not introspectible, so it will not work properly in bindings. https://bugzilla.gnome.org/show_bug.cgi?id=709865
(In reply to César Fabián Orccón Chipana from comment #53) > (In reply to Philip Withnall from comment #45) > > > > What does the entry for g_object_new_with_properties() in the > > GObject-2.0.gir you’re using look like? > > <function name="new_with_properties" > c:identifier="g_object_new_with_properties"> > <return-value transfer-ownership="none" nullable="1"> > <type name="gpointer" c:type="gpointer"/> > </return-value> > <parameters> > <parameter name="object_type" transfer-ownership="none"> > <type name="GType" c:type="GType"/> > </parameter> > <parameter name="n_properties" transfer-ownership="none"> > <type name="guint" c:type="guint"/> > </parameter> > <parameter name="names" transfer-ownership="none"> > <type name="utf8" c:type="const char*"/> > </parameter> > <parameter name="values" transfer-ownership="none"> > <type name="Value" c:type="const GValue"/> > </parameter> > </parameters> > </function> I think you need to update the GLib annotations in gobject-instropection (gir/*.c files) using gobject-introspection/misc/update-glib-annotations.py and build gobject-introspection again to get the proper gir file.
Review of attachment 347710 [details] [review]: Looks good to me. Emmanuele, do you have any final comments?
Review of attachment 347714 [details] [review]: Looks good to me. Emmanuele, any other comments?
Review of attachment 347715 [details] [review]: Accepting as before.
As much as I'd like this to be committed, are we sure that one day before code freeze is a good time to land API that will have to stick around forever?
Review of attachment 347710 [details] [review]: ::: gobject/gobject.c @@ +1778,3 @@ GObject *object; + if (G_UNLIKELY (CLASS_HAS_CUSTOM_CONSTRUCTOR (class))) This is an extraneous chunk; it should go in a separate commit, even though it's correct. @@ +1873,3 @@ + { + if (params[i].pspec == pspec) + return FALSE; Coding style: too much indentation.
Review of attachment 347714 [details] [review]: ::: gobject/gobject.c @@ +2522,3 @@ const GValue *value) { + g_object_setv (object, 1, &property_name, value); I'm not overly fond of this, but I guess it's okay.
Review of attachment 347720 [details] [review]: ::: gobject/gobject.c @@ +1942,3 @@ + params[count].value = g_newa (GValue, 1); + memset (params[count].value, 0, sizeof (GValue)); + g_value_init (params[count].value, G_VALUE_TYPE (&values[count])); Shouldn’t this use &values[i]? i is the counter for names/values; count is the counter for params. @@ +1944,3 @@ + g_value_init (params[count].value, G_VALUE_TYPE (&values[count])); + + g_value_copy (&values[count], params[count].value); Similarly, shouldn’t this be &values[i]?
Review of attachment 347715 [details] [review]: Okay, with a minor documentation fix that should go in before pushing. ::: gobject/gobject.c @@ +1975,3 @@ * @object_type + * + * Deprecated: 2.52: Use g_object_new_with_properties() instead, because #GParameter is now This should be something like: Deprecated: 2.52: Use g_object_new_with_properties() instead. The deprecation of GParameter is going to be linked by the GParameter argument.
Review of attachment 347716 [details] [review]: Looks good to me. Emmanuele, any final comments?
Review of attachment 346664 [details] [review]: Okay, with a minor documentation nitpick that should be fixed before pushing. ::: gobject/gparam.h @@ +269,3 @@ * to hand parameter name/value pairs to g_object_newv(). + * + * Deprecated: 2.52: This type will be removed in a future version. Deprecation implies removal. Also, there are no plans of bumping API in GLib for the time being. This should just read: Deprecated: 2.52: This type is not introspectable.
Review of attachment 346665 [details] [review]: Okay, with minor documentation nitpicks that should be fixed before pushing. ::: gio/gasyncinitable.c @@ +364,3 @@ * * Since: 2.22 + * Deprecated: 2.52: Use g_object_new_with_properties() followed by g_async_initable_init_async() There's no point in mentioning that "now GParameter is deprecated". Deprecated: 2.52: Use g_object_new_with_properties() and g_async_initiable_init_async() instead. ::: gio/ginitable.c @@ +170,3 @@ * Since: 2.22 + * Deprecated: 2.52: Use g_object_new_with_properties() followed by g_initable_init() instead, + * because #GParameter is now deprecated. See #GParameter for more information. There's no point in mentioning that "now GParameter is deprecated". Deprecated: 2.52: Use g_object_new_with_properties() and g_initiable_init() instead.
Review of attachment 347716 [details] [review]: Okay.
Review of attachment 347720 [details] [review]: ::: gobject/gobject.c @@ +1935,3 @@ + pspec = g_param_spec_pool_lookup (pspec_pool, names[i], object_type, TRUE); + if (!g_object_new_is_valid_property (object_type, pspec, names[i], + const GValue values[]) Coding style: there's no need to break the line, here.
Review of attachment 347710 [details] [review]: ::: gobject/gobject.c @@ +1857,3 @@ + g_critical ("%s: object class '%s' has no property named '%s'", + G_STRFUNC, g_type_name (object_type), name); + int n_params) I don't think this comment is relevant. @@ +1871,3 @@ + { + for (i = 0; i < n_params; i++) + /* Can't continue because arg list will be out of sync. */ Brackets can be omitted here. @@ +2097,3 @@ + G_STRFUNC, + G_OBJECT_TYPE_NAME (object), +{ Maybe you could join this g_warning call in two lines, similar to what you did in g_object_new_is_valid_property. Same in few other places. @@ +2100,3 @@ + return FALSE; + } + { You could omit the "else if"s here and below. Similar to g_object_new_is_valid_property. @@ +2188,3 @@ + return FALSE; + } + { You could omit the "else if"s here. Similar to g_object_new_is_valid_property.
Review of attachment 347715 [details] [review]: I think this commit should go AFTER "gobject: Add g_object_new_with_properties"
Created attachment 348122 [details] [review] gobject: Add helper functions to handle warnings in g_object_new/set/get g_object_new_is_valid_property g_object_get_is_valid_property g_object_set_is_valid_property
Created attachment 348123 [details] [review] gobject: Add g_object_new_with_properties g_object_new_with_properties is an alternative to g_object_newv. The last one, takes an array of GParameter. However, GParameter is a rarely used type and this type is not introspectible, so it will not work properly in bindings.
Created attachment 348124 [details] [review] gobject: Add g_object_setv and g_object_getv functions
Created attachment 348125 [details] [review] tests: Add test for gobject properties for g_object_newv/setv/getv
Created attachment 348126 [details] [review] gobject: Deprecate GParameter GParameter is a rarely used type and not introspectible.
Created attachment 348127 [details] [review] gio: Deprecate GParameter-related functions
Created attachment 348128 [details] [review] gobject: Deprecate g_object_newv g_object_newv uses a GParameter as argument. Since GParameter is deprecated due to this type is not introspectible, g_object_newv is deprecated now.
Review of attachment 348122 [details] [review]: ++
Review of attachment 348123 [details] [review]: ++
Review of attachment 348124 [details] [review]: ++
Review of attachment 348125 [details] [review]: ++
Review of attachment 348126 [details] [review]: ++
Review of attachment 348127 [details] [review]: ++
Review of attachment 348128 [details] [review]: ++
Pushed with some minor fixes (unaddressed review comment from comment #81; some warning fixes) and a couple of follow-up commits (fix some more warnings about using deprecated API; update the Since and Deprecated versions). Thanks for working on this. I’ll leave the bug open for handling the pygobject patch. Attachment 348122 [details] pushed as 19e81de - gobject: Add helper functions to handle warnings in g_object_new/set/get Attachment 348123 [details] pushed as 26b211e - gobject: Add g_object_new_with_properties Attachment 348124 [details] pushed as c6d373b - gobject: Add g_object_setv and g_object_getv functions Attachment 348125 [details] pushed as 942edbc - tests: Add test for gobject properties for g_object_newv/setv/getv Attachment 348126 [details] pushed as 2646c21 - gobject: Deprecate GParameter Attachment 348127 [details] pushed as 3151da1 - gio: Deprecate GParameter-related functions Attachment 348128 [details] pushed as e2c3b7f - gobject: Deprecate g_object_newv
(In reply to Philip Withnall from comment #101) > Thanks for working on this. I’ll leave the bug open for handling the > pygobject patch. Although that might proceed faster if it was in the pygobject product. Moving.
*** Bug 338024 has been marked as a duplicate of this bug. ***
Created attachment 363974 [details] [review] Remove GParameter references Rebased on master and added a glib version requirement bump. Since this changes the glib requirement from 2.38 to 2.54 I'm inclined to push this to the next cycle as it doesn't seem to change any functionality. But I don't feel strongly about this..
Review of attachment 363974 [details] [review]: A commit message explaining why the change is needed/desired would be nice (even if the only reason is getting rid of GParameter)
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/pygobject/issues/54.