Merge lp:~larsu/gsettings-qt/lp1503693 into lp:gsettings-qt

Proposed by Lars Karlitski on 2015-10-29
Status: Merged
Approved by: Albert Astals Cid on 2015-11-04
Approved revision: 75
Merged at revision: 74
Proposed branch: lp:~larsu/gsettings-qt/lp1503693
Merge into: lp:gsettings-qt
Diff against target: 49 lines (+16/-2)
2 files modified
GSettings/gsettings-qml.cpp (+13/-1)
tests/tst_GSettings.qml (+3/-1)
To merge this branch: bzr merge lp:~larsu/gsettings-qt/lp1503693
Reviewer Review Type Date Requested Status
Albert Astals Cid (community) 2015-10-29 Approve on 2015-11-04
Review via email: mp+276190@code.launchpad.net

Commit message

qgsettings-qml: explicitly emit changed events

We're working around QTBUG-32859 by not dispatching changed signals
directly. This leads to two issues:

1. Changed signals are not emitted by GSettingsQml, because it only
emits those when the value is different from its cached one, which is
already updated by the time the queued signal arrives.

2. Calling schema.reset() from qml and immediately fetching the value
after returns the old value, because the value was only updated when the
signal arrives.

Fix these by explicitly emitting the changed signal when setting the
value from qml (the later one is then ignored) and setting the cached
value when calling reset().

Also, remove changesSpy from test_reset(), because that now counts
changes correctly and includes the ones from previous tests.

This patch can be reverted once the Qt bug is fixed.

Description of the change

qgsettings-qml: explicitly emit changed events

To post a comment you must log in.
Albert Astals Cid (aacid) wrote :

any reason you removed the signalspy? i think it's good checking that reset does actually trigger the signal too, proves that qml will re-evaluate things that depend on it, something like

    changesSpy.clear();
    settings.schema.reset('testInteger');
    tryCompare(changesSpy, "count", 1);

What do you think?

review: Needs Information
lp:~larsu/gsettings-qt/lp1503693 updated on 2015-11-04
75. By Lars Karlitski on 2015-11-04

qml tests: put changesSpy back

To check that the signal gets emitted when a key is reset.

Lars Karlitski (larsu) wrote :

> any reason you removed the signalspy? i think it's good checking that reset
> does actually trigger the signal too, proves that qml will re-evaluate things
> that depend on it, something like
>
> changesSpy.clear();
> settings.schema.reset('testInteger');
> tryCompare(changesSpy, "count", 1);
>
> What do you think?

Good point. I removed it because it was counting the emitted signals for all tests. Didn't think about looking at the docs to find something like "clear". Thanks!

Albert Astals Cid (aacid) wrote :

It's non ideal but since fixing that bug in qt is much harder than this, let's approve this and hope they fix the bug in qt at some point.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'GSettings/gsettings-qml.cpp'
2--- GSettings/gsettings-qml.cpp 2015-06-02 10:10:03 +0000
3+++ GSettings/gsettings-qml.cpp 2015-11-04 15:29:45 +0000
4@@ -104,8 +104,13 @@
5 {
6 GSettingsQml *parent = (GSettingsQml *) this->parent();
7
8- if (parent->priv->settings != NULL)
9+ if (parent->priv->settings != NULL) {
10 parent->priv->settings->reset(key);
11+
12+ // make sure this object gets the new value immediately and not on the
13+ // next main loop iteration (see updateValue)
14+ parent->settingChanged(key);
15+ }
16 }
17
18 GSettingsQml::GSettingsQml(QObject *parent): QQmlPropertyMap(this, parent)
19@@ -161,6 +166,13 @@
20 return QVariant();
21
22 if (priv->settings->trySet(key, value)) {
23+ // due to QTBUG-32859, QGSettings doesn't dispatch its changed signal
24+ // directly, but on a new main loop iteration. At that point, this
25+ // object already has the new value set and doesn't emit its own
26+ // changed signal (see ::settingChanged). Emit it here so that it is
27+ // sent even when the setting is changed from qml.
28+ Q_EMIT(changed(key, value));
29+
30 return value;
31 }
32 else {
33
34=== modified file 'tests/tst_GSettings.qml'
35--- tests/tst_GSettings.qml 2015-06-02 10:10:03 +0000
36+++ tests/tst_GSettings.qml 2015-11-04 15:29:45 +0000
37@@ -95,9 +95,11 @@
38
39 function test_reset() {
40 settings.testInteger = 4;
41+
42+ changesSpy.clear();
43 settings.schema.reset('testInteger');
44- tryCompare(changesSpy, "count", 1); // this merely exists only to connect to the "changed" signal; resetting changes is async in gsettings
45 compare(settings.testInteger, 42);
46+ tryCompare(changesSpy, "count", 1);
47 }
48
49 function test_invalid_schema() {

Subscribers

People subscribed via source and target branches

to all changes: