GNOME Bugzilla – Bug 723394
const parameter to GtkPopover gtk_popover_set_pointing_to
Last modified: 2015-05-18 08:35:57 UTC
I think that in order to make this method const-correct the rect parameter needs to be declared as const. Sorry if this is not intended to be this way.
Created attachment 267756 [details] [review] const-correct gtk_popover_set_pointing_to()
*** Bug 723396 has been marked as a duplicate of this bug. ***
Comment on attachment 267756 [details] [review] const-correct gtk_popover_set_pointing_to() as per IRC discussion, maybe using GdkRectangle instead of cairo_rectangle_t also fits best, it would be great if you could update the patch, otherwise I'll get at that
Created attachment 268102 [details] [review] Use GdkRectangle instead of cairo_rectangle_int_t
This patch replaces cairo_rectangle_int_t with GdkRectangle, and makes gtk_popover_set_pointing_to const-correct.
Comment on attachment 268102 [details] [review] Use GdkRectangle instead of cairo_rectangle_int_t looks good!
I pushed this: https://git.gnome.org/browse/gtk+/commit/?id=552c29b488ecd7bcc3303dd5514ce6dbfff04437 Conflicts meant that I ended up recreating the patch, so I guess it's possible that it's slightly different.
Why wasn't CAIRO_GOBJECT_TYPE_RECTANGLE_INT changed to GDK_TYPE_RECTANGLE in g_object_class_install_property (object_class, PROP_POINTING_TO, g_param_spec_boxed ("pointing-to", P_("Pointing to"), P_("Rectangle the bubble window points to"), CAIRO_GOBJECT_TYPE_RECTANGLE_INT, GTK_PARAM_READWRITE)); Was it an oversight, or was it deliberately not changed?
I guess I just missed it.
Created attachment 301490 [details] [review] patch: GtkPopover:pointing-to: GdkRectangle instead of cairo_rectangle_int_t Other people also missed it. It's not done in the patch in comment 4. The attached patch changes the type of the pointing-to property. Can such a patch be pushed now, or is it an API break? It can affect people that use g_object_get/set_property(). The patch won't be a problem for gtkmm. On the contrary. This property has not yet been wrapped in gtkmm, probably because of the inconsistency between GdkRectangle in gtk_popover_get/set_pointing_to() and cairo_rectangle_int_t in the property.
> It can affect people that use g_object_get/set_property(). Could it have worked before? A change is generally not a break if the API was unusable before.
Yes, sorry, I totally missed it. I do not remember if I did any testing with this property in gtkmm; probably not.
I've copied gtk+/demos/gtk-demo/popover.c, changed the copy, and tested g_object_get/set_property(). 1. g_value_init(&value, CAIRO_GOBJECT_TYPE_RECTANGLE_INT); g_value_set_boxed(&value, &rect); g_object_set_property(G_OBJECT(popover), "pointing-to", &value); g_object_get_property(G_OBJECT(popover), "pointing-to", &value); 1.1 Without my patch: No warning. 1.2 With my patch: (example:2893): GLib-GObject-WARNING **: unable to set property 'pointing-to' of type 'GdkRectangle' from value of type 'CairoRectangleInt' (example:2893): GLib-GObject-WARNING **: g_object_get_property: can't retrieve property 'pointing-to' of type 'GdkRectangle' as value of type 'CairoRectangleInt' 2. g_value_init(&value, GDK_TYPE_RECTANGLE); g_value_set_boxed(&value, &rect); g_object_set_property(G_OBJECT(popover), "pointing-to", &value); g_object_get_property(G_OBJECT(popover), "pointing-to", &value); 2.1 Without my patch: (example:2870): GLib-GObject-WARNING **: unable to set property 'pointing-to' of type 'CairoRectangleInt' from value of type 'GdkRectangle' (example:2870): GLib-GObject-WARNING **: g_object_get_property: can't retrieve property 'pointing-to' of type 'CairoRectangleInt' as value of type 'GdkRectangle' 2.2 With my patch: No warning. 3. There are no warnings, if you add void transform_rect(const GValue* src_value, GValue* dest_value) { g_value_set_boxed(dest_value, g_value_get_boxed(src_value)); } g_value_register_transform_func(CAIRO_GOBJECT_TYPE_RECTANGLE_INT, GDK_TYPE_RECTANGLE, transform_rect); g_value_register_transform_func(GDK_TYPE_RECTANGLE, CAIRO_GOBJECT_TYPE_RECTANGLE_INT, transform_rect);
Always reopen bugs when attaching new stuff, please.
(In reply to Kjell Ahlstedt from comment #13) > I've copied gtk+/demos/gtk-demo/popover.c, changed the copy, and tested > g_object_get/set_property(). > > 1. > g_value_init(&value, CAIRO_GOBJECT_TYPE_RECTANGLE_INT); > g_value_set_boxed(&value, &rect); > g_object_set_property(G_OBJECT(popover), "pointing-to", &value); > g_object_get_property(G_OBJECT(popover), "pointing-to", &value); > > 1.1 Without my patch: No warning. > > 1.2 With my patch: > (example:2893): GLib-GObject-WARNING **: unable to set property > 'pointing-to' of type 'GdkRectangle' from value of type 'CairoRectangleInt' > (example:2893): GLib-GObject-WARNING **: g_object_get_property: can't > retrieve property 'pointing-to' of type 'GdkRectangle' as value of type > 'CairoRectangleInt' > > 2. > g_value_init(&value, GDK_TYPE_RECTANGLE); > g_value_set_boxed(&value, &rect); > g_object_set_property(G_OBJECT(popover), "pointing-to", &value); > g_object_get_property(G_OBJECT(popover), "pointing-to", &value); > > 2.1 Without my patch: > (example:2870): GLib-GObject-WARNING **: unable to set property > 'pointing-to' of type 'CairoRectangleInt' from value of type 'GdkRectangle' > (example:2870): GLib-GObject-WARNING **: g_object_get_property: can't > retrieve property 'pointing-to' of type 'CairoRectangleInt' as value of type > 'GdkRectangle' > > 2.2 With my patch: No warning. > > 3. There are no warnings, if you add > > void transform_rect(const GValue* src_value, GValue* dest_value) > { > g_value_set_boxed(dest_value, g_value_get_boxed(src_value)); > } > > g_value_register_transform_func(CAIRO_GOBJECT_TYPE_RECTANGLE_INT, > GDK_TYPE_RECTANGLE, > transform_rect); > g_value_register_transform_func(GDK_TYPE_RECTANGLE, > CAIRO_GOBJECT_TYPE_RECTANGLE_INT, > transform_rect); I think the transform func is probably what we'll have to go with if we want to fix this. Would suggest to register in gdk_pre_parse() or so.
(In reply to Matthias Clasen from comment #15) > I think the transform func is probably what we'll have to go with if we want > to fix this. Would suggest to register in gdk_pre_parse() or so. The description of G_DEFINE_BOXED_TYPE_WITH_CODE() says Similar to G_DEFINE_BOXED_TYPE(), but allows to insert custom code into the type_name_get_type() function, e.g. to register value transformations with g_value_register_transform_func(). Isn't that a good alternative to inserting g_value_register_transform_func() in gdk_pre_parse()? When I replace G_DEFINE_BOXED_TYPE() in gdkrectangle.c with G_DEFINE_BOXED_TYPE_WITH_CODE (GdkRectangle, gdk_rectangle, gdk_rectangle_copy, g_free, gdk_rectangle_register_value_transform_funcs ()) it compiles without problem, but unfortunately the gtk+ build halts at GISCAN Gdk-3.0.gir It looks like gobject-introspection is waiting indefinitely for something. Is this worth investigating further, or do you prefer to have the transformation functions registered in gdk_pre_parse()?
sounds worth investigating, thanks for pointing it out
Created attachment 301975 [details] [review] patch: GtkPopover:pointing-to: GdkRectangle instead of cairo_rectangle_int_t patch: GtkPopover:pointing-to: GdkRectangle instead of cairo_rectangle_int_t It's not gobject-introspection's fault. I used GDK_TYPE_RECTANGLE in the registration function, called from G_DEFINE_BOXED_TYPE_WITH_CODE(). That's not possible. A comment in the patch explains what happens. I wouldn't mind if the description of G_DEFINE_BOXED_TYPE_WITH_CODE() mentioned this restriction.
Review of attachment 301975 [details] [review]: Looks good to me, thanks! Could you file a glib bug about the registration function docs ?
Review of attachment 301975 [details] [review]: I have pushed https://git.gnome.org/browse/gtk+/commit/?id=2495edc9fdd604fa9f052b4003a492d2b7230d37 and filed glib bug 748228.
This change broke access to positioning popovers from Gjs. It introduces a difference between what gets reported by the introspection data (Cairo.RectangleInt) and the GObject class (GdkRectangle). Meaning Gjs cannot tell these 2 types are actually the same thing. Yes, sure we could check that the types are transformable, but does anyone actually dares adding one more case to this lovely 396 lines function : https://git.gnome.org/browse/gjs/tree/gi/value.cpp#n238 ? Would it be possible to revert this change and submit another one for fixing the const correctness, if this is what this change is about?
The commit in comment 20 is not about const correctness, it's about type correctness. The bug title has become misleading. I wanted the pointing-to property to have the same type (GdkRectangle) as used in gtk_popover_[set,get]_pointing_to(). I suppose gobject-introspection is involved in your problem. Unfortunately I know very little about it. I mostly work with the C++ bindings glibmm and gtkmm. I hope someone else can tell if your problem can reasonably easily be fixed, or if we have to revert the change in gdkrectangle.c and gtkpopover.c. I suppose that if this change breaks Gjs, it might also break other code. From where do you get the introspection data (Cairo.RectangleInt)? The type of GtkPopover:pointing-to was changed from cairo_rectangle_int_t to GdkRectangle. Is it possible that you just don't use updated introspection data?
Any appearance of GdkRectangle in introspection data is automatically replaced by https://git.gnome.org/browse/gobject-introspection/tree/girepository/girepository.c#n819
Opened https://bugzilla.gnome.org/show_bug.cgi?id=748832 and https://bugzilla.gnome.org/show_bug.cgi?id=748833 to resolve this.
(In reply to Lionel Landwerlin from comment #21) > Meaning Gjs cannot tell these 2 types are actually the same thing. This is a GJS issue. > Yes, sure we could check that the types are transformable, but does anyone > actually dares adding one more case to this lovely 396 lines function : > https://git.gnome.org/browse/gjs/tree/gi/value.cpp#n238 ? The fact that the function is 396 lines long is pretty much immaterial, if it's not doing the right thing — and the right thing is always to check if the value type when dealing with properties is transformable to or from the property type.
(In reply to Lionel Landwerlin from comment #24) > Opened https://bugzilla.gnome.org/show_bug.cgi?id=748832 and > https://bugzilla.gnome.org/show_bug.cgi?id=748833 to resolve this. Regardless of these two issues, which should be fixed, GJS needs a bug open to call g_value_transform() when dealing with instance/boxed types. A bunch of perfectly valid GObject-related code depends on transformable types anyway.
Lionel, can I close this bug? Or do you still think that the commit in comment 20 ought to be reverted?
The commit in https://bugzilla.gnome.org/show_bug.cgi?id=748832 hasn't landed yet, so the situation is still unresolved :/ I guess we can close this bug as there is a solution that doesn't involve reverting.
Both bug 748832 and bug 748833 have been fixed, meaning there is no need to revert the commit in comment 20. Instead I revert this bug to Fixed status.