After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 684882 - Gsettings should spaw a warning when binding against a low_underscored_property
Gsettings should spaw a warning when binding against a low_underscored_property
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gsettings
2.34.x
Other Linux
: Normal normal
: ---
Assigned To: Allison Karlitskaya (desrt)
gtkdev
Depends on:
Blocks:
 
 
Reported: 2012-09-26 14:47 UTC by Didier Roche
Modified: 2012-10-15 23:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
g_settings_bind: use canonical property name (4.48 KB, patch)
2012-09-26 16:48 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Didier Roche 2012-09-26 14:47:57 UTC
Small snippet of vala code:

constructor:
            this.gp_settings = new GLib.Settings ("com.canonical.Unity.Lenses");
            this.gp_settings.bind (REMOTE_CONTENT_KEY, this, "remote_content_search", SettingsBindFlags.DEFAULT);


        public RemoteContent remote_content_search { get; set; default = RemoteContent.ALL; }


and I try: remote_content_search = RemoteContent.NONE;


The remote_content_search vala property (corresponding to remote-content-search) is correctly refreshed when the gsettings key changed. However, setting it doesn't change the gsettings value.
After some discussion with Ryan, the cause seems to be that gsettings is watching for notify::remote_content_search and not notify::remote-content-search (but still translates correctly the other way around). It should at least spaw a warning to help the developer to use the - format.
Comment 1 Allison Karlitskaya (desrt) 2012-09-26 15:39:08 UTC
We're looking up the GParamSpec for the GObject property anyway -- we should just use the copy of the property name from there to append to "notify::" instead of using the string passed from the user.

As much as I'd like to punish the user for typing the wrong string, it's actually easier to make it just-work...
Comment 2 Allison Karlitskaya (desrt) 2012-09-26 16:48:56 UTC
Created attachment 225229 [details] [review]
g_settings_bind: use canonical property name

We were using the user-passed value of the @property argument for
several purposes in g_settings_bind(): error messages, binding
uniqueness (ie: one-binding-per-property-per-object) and most
importantly, connecting to the detailed notify:: signal.

The user may pass a string like "property_name" when the property's
canonical name is "property-name".  g_object_class_find_property() will
find the property under these circumstances, but a connection to
"notify::property_name" will not notice notifies emitted for
"property-name".

We can solve this by using the user's string to perform the lookup and
then using pspec->name for everything after that.
Comment 3 Colin Walters 2012-09-26 17:09:13 UTC
Review of attachment 225229 [details] [review]:

Code looks right.

Could use a test, but don't block on it.

Also, would it make sense to fix GObject so notify:: did the right thing?

::: mkinstalldirs
@@ +82,3 @@
       exec mkdir -p -- "$@"
     else
+      # On NextStep and OpenStep, the `mkdir' command does not

Spurious change?
Comment 4 Allison Karlitskaya (desrt) 2012-09-26 18:40:39 UTC
Review of attachment 225229 [details] [review]:

I don't think we can fix GObject with respect to this....  signal details are handled on the basis of quarks, which treat "-" and "_" differently.

If we were to 'fix' GObject, I would suggest we did it by removing the canonicalisation in property names (ie: _ is different from -).  A good first step in that direction may be to warn about any property names that are changed as a result of the canonicalisation process....

::: mkinstalldirs
@@ +82,3 @@
       exec mkdir -p -- "$@"
     else
+      # On NextStep and OpenStep, the `mkdir' command does not

Weird.  I guess automake copies this file in... why do we have it under version control?
Comment 5 Matthias Clasen 2012-09-27 02:36:25 UTC
(In reply to comment #4)

> ::: mkinstalldirs
> @@ +82,3 @@
>        exec mkdir -p -- "$@"
>      else
> +      # On NextStep and OpenStep, the `mkdir' command does not
> 
> Weird.  I guess automake copies this file in... why do we have it under version
> control?

Because it is used by glib-gettextize.
Comment 6 Didier Roche 2012-09-27 07:52:14 UTC
Tested Ryan's patch and if I change - by _, the property, in addition to be refreshed by get, is now effectively writing to gsettings with set. Thanks!
Comment 7 Matthias Clasen 2012-09-30 18:29:43 UTC
Review of attachment 225229 [details] [review]:

Please commit without the mkinstalldirs change
Comment 8 Matthias Clasen 2012-10-15 23:28:21 UTC
Attachment 225229 [details] pushed as 1a20d56 - g_settings_bind: use canonical property name