Created attachment 267922 [details] [review]
GPropertyAction: avoid a critical message

g_settings_set_mapping() can return NULL in valid cases, but
g_variant_ref_sink() doesn't support NULL.

Created attachment 267923 [details] [review]
GSimpleActionGroup: avoid a critical message

Review of attachment 267923 [details] [review]:

The state change can never be to NULL -- actions must always either have a NULL state (which never changes) or have a state which will change between values of the same type, but never to NULL.

Therefore this patch doesn't make sense.

Review of attachment 267922 [details] [review]:

The interface of GAction is that a stateful action must always have a state of the expected type -- never NULL.  We therefore must always return a value of the correct type from this function.

Thank you for looking into this problem (which I agree is a problem).  The approach in the proposed patches isn't sufficient, though.

I think we should maybe figure out a way to either do the following:

  (a) apply some stricter checks for type compatibility on construction and
      possibly fail to construct it if we can't always guarantee it

  (b) return a GVariant with some sort of 'default value' in the case that we
      get NULL from the GSettings mapping

The critical messages occur with gedit master, so there is maybe something wrong on the gedit side.

I've just written the patches because I thought it was trivial. But I don't really plan to work on this.

on the gedit side this is triggered by

action = g_property_action_new ("set-visible-child", priv->stack, "visible-child-name");
g_action_map_add_action (G_ACTION_MAP (priv->action_group), G_ACTION (action));

in the menu-stack-switcher.

Could it be that it does not like the initial NULL value of the property?

Created attachment 269191 [details] [review]
patch

What about this instead?

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

Review of attachment 269191 [details] [review]:

This patch is evil... but less evil than the other one.

I'd still like to try to find a better solution -- probably on the gedit side.

The crux of this issue is that if this property is NULL then we cannot have a property action for it.  That probably boils down to us having to create/destroy the property action as the stack enters/leaves states where NULL is a possible value.  I guess that means we create the action when there are pages to switch and destroy it again when the last page is gone.  That even sort of makes sense logically...

Created attachment 269336 [details] [review]
gedit workaround

This is the gedit workaround of initting lazily.

It works, but we still get the criticals on shutdown.

Bug #724506 has a gtk patch to work around the other side of the issue.



This is serious engineering work, original designers of the Pisa tower would be proud

Review of attachment 269336 [details] [review]:

I'm happier with this. :)

I gave up and reimplemented the switcher not using GPropertyAction at all (since it also fits the visual design requirements better), so the gedit critical is fixed.

That said I still think this should be addressed in GProperyAction: we may like it or not, but a GParamSpec for strings can be NULL so string properties are always "ms" imho... I have a half done patch for the g_property_action_new_wit_type, but it didn't fix the issue so I did not pursue it, if needed I can attach it

-- 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/glib/issues/826.