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 761381 - gtk_settings_reset_property does not always reset to correct value
gtk_settings_reset_property does not always reset to correct value
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: .General
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2016-02-01 00:47 UTC by Alberts Muktupāvels
Modified: 2016-02-04 09:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gtksettings: correctly reset property (5.79 KB, patch)
2016-02-01 00:47 UTC, Alberts Muktupāvels
none Details | Review
gtksettings: correctly reset property (v2) (3.50 KB, patch)
2016-02-01 18:46 UTC, Alberts Muktupāvels
none Details | Review
gtksettings: reset property to correct default value (2.22 KB, patch)
2016-02-01 20:53 UTC, Alberts Muktupāvels
committed Details | Review
gtksettings: fix critical warning (1.56 KB, patch)
2016-02-03 13:58 UTC, Alberts Muktupāvels
committed Details | Review

Description Alberts Muktupāvels 2016-02-01 00:47:55 UTC
Created attachment 320164 [details] [review]
gtksettings: correctly reset property

Currently gtk_settings_reset_property resets property to default value of pspec, but it is not always correct default value... Default values can be changed with settings.ini files.

Example with gnome-terminal:
1) User enables Global Dark Theme from Tweak Tool.
2) Theme variant is selected - default. In this case gnome-terminal will call gtk_settings_reset_property that will reset dark theme to FALSE. And now gnome-terminal use light theme variant instead of dark.
Comment 1 Matthias Clasen 2016-02-01 17:40:16 UTC
Review of attachment 320164 [details] [review]:

It is a bit unfortunate to reload multiple key files for a single key. It also introduces a subtle change of semantics: We're not monitoring these files, but if you reset a key, you'll get the newer value - but only for that one key.
Comment 2 Alberts Muktupāvels 2016-02-01 17:56:45 UTC
(In reply to Matthias Clasen from comment #1)
> Review of attachment 320164 [details] [review] [review]:
> 
> It is a bit unfortunate to reload multiple key files for a single key. It
> also introduces a subtle change of semantics: We're not monitoring these
> files, but if you reset a key, you'll get the newer value - but only for
> that one key.

And how about this:
Store property values (from .ini files) in hash table and then when resetting property get value form hash table. If there is no property in hash table then reset to pspec default value?

Has there been any discussion about possibility to monitor settings.ini files?
Comment 3 Alberts Muktupāvels 2016-02-01 18:46:59 UTC
Created attachment 320210 [details] [review]
gtksettings: correctly reset property (v2)
Comment 4 Matthias Clasen 2016-02-01 20:07:52 UTC
The idea is good. One change I would propose it to attach the overridden default value to the param specs with g_param_spec_set_qdata instead of maintaining a separate hash table.
Comment 5 Alberts Muktupāvels 2016-02-01 20:53:27 UTC
Created attachment 320228 [details] [review]
gtksettings: reset property to correct default value
Comment 6 Matthias Clasen 2016-02-02 15:44:58 UTC
Review of attachment 320228 [details] [review]:

Looks good now, thanks
Comment 7 Matthias Clasen 2016-02-02 15:45:09 UTC
Review of attachment 320228 [details] [review]:

Looks good now, thanks
Comment 8 Alberts Muktupāvels 2016-02-03 13:58:17 UTC
Created attachment 320340 [details] [review]
gtksettings: fix critical warning

Commit 5186aeb90f52c941a2642b04ebfb54e9d8f8ea11 introduced critical
warning - g_value_copy: assertion 'g_value_type_compatible (...)' failed.

P.S. Why boolean is created as G_TYPE_LONG in gtk_settings_load_from_key_file?
Comment 9 Matthias Clasen 2016-02-03 16:40:00 UTC
I think it is worth trying to just use set_boolean. Looking through the history, this is very old code from 2001, and it may predate boolean support in GValue.
Comment 10 Alberts Muktupāvels 2016-02-03 18:07:53 UTC
I think that it is better to stick with patch that I created...

Changing boolean from G_TYPE_LONG to G_TYPE_BOOLEAN will not be enough. Same problem will be with G_TYPE_INT/UINT that is also created as G_TYPE_LONG, same problem with string that is created as G_TYPE_GSTRING. All that will case same critical warning when resetting property. Only G_TYPE_DOUBLE is created with correct type.
Comment 11 Matthias Clasen 2016-02-04 09:36:48 UTC
Review of attachment 320340 [details] [review]:

Lets stick with this for now, then