Ubuntu Settings Components

Merge lp:~nick-dedekind/ubuntu-settings-components/fix-maximumWaitBufferInterval into lp:~registry/ubuntu-settings-components/trunk

  1. fix-maximumWaitBufferInterval
  2. Merge into trunk
Proposed by Nick Dedekind
Status: Merged
Approved by: Albert Astals Cid
Approved revision: 90
Merged at revision: 88
Proposed branch: lp:~nick-dedekind/ubuntu-settings-components/fix-maximumWaitBufferInterval
Merge into: lp:~registry/ubuntu-settings-components/trunk
Diff against target: 313 lines (+84/-57)
3 files modified
plugins/Ubuntu/Settings/Components/serverpropertysynchroniser.cpp (+60/-41)
plugins/Ubuntu/Settings/Components/serverpropertysynchroniser.h (+5/-2)
tests/qmltests/Components/tst_ServerPropertySynchroniser.qml (+19/-14)
To merge this branch: bzr merge lp:~nick-dedekind/ubuntu-settings-components/fix-maximumWaitBufferInterval
Reviewer Review Type Date Requested Status
Albert Astals Cid (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Registry Administrators Pending
Review via email: mp+257347@code.launchpad.net

Commit message

ServerPropertySynchorniser - Do not update to old server value when the control is no longer buffering.

Description of the change

Fix and test for autopilot failures:
https://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/2076/#showFailuresLink

Do not update to old server value when the control is no longer buffering.

 * Are there any related MPs required for this MP to build/function as expected? Please list.
https://code.launchpad.net/~nick-dedekind/unity8/fix-laggy-indicators-autopilot/+merge/258411

 * Did you perform an exploratory manual test run of your code change and any related functionality?
Yes

 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

 * If you changed the UI, has there been a design review?
N/A

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:88
http://jenkins.qa.ubuntu.com/job/ubuntu-settings-components-ci/109/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-settings-components-qmltests-vivid/28
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-settings-components-vivid-amd64-ci/16
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-settings-components-vivid-armhf-ci/16
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-settings-components-vivid-armhf-ci/16/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-settings-components-ci/109/rebuild

review: Needs Fixing (continuous-integration)
lp:~nick-dedekind/ubuntu-settings-components/fix-maximumWaitBufferInterval updated
89. By Nick Dedekind

better test for max_interval

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:89
http://jenkins.qa.ubuntu.com/job/ubuntu-settings-components-ci/110/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-settings-components-qmltests-vivid/29
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-settings-components-vivid-amd64-ci/17
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-settings-components-vivid-armhf-ci/17
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-settings-components-vivid-armhf-ci/17/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-settings-components-ci/110/rebuild

review: Approve (continuous-integration)
Revision history for this message
Albert Astals Cid (aacid) wrote :

running autopilot3 run unity8.indicators.tests.test_action_latency.TestBuffering.test_slider_buffers_activations
gives me

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/unity8/indicators/tests/test_action_latency.py", line 163, in test_slider_buffers_activations
    NotEquals(final_value)
  File "/usr/lib/python3/dist-packages/testtools/testcase.py", line 423, in assertThat
    raise mismatch_error
testtools.matchers._impl.MismatchError: 1.0 == 1.0

Ran 1 test in 25.957s
FAILED (failures=1)

review: Needs Fixing
lp:~nick-dedekind/ubuntu-settings-components/fix-maximumWaitBufferInterval updated
90. By Nick Dedekind

Fixed some more problems with activation damping

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:90
http://jenkins.qa.ubuntu.com/job/ubuntu-settings-components-ci/112/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-settings-components-qmltests-vivid/31
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-settings-components-vivid-amd64-ci/19
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-settings-components-vivid-armhf-ci/19
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-settings-components-vivid-armhf-ci/19/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-settings-components-ci/112/rebuild

review: Approve (continuous-integration)
Revision history for this message
Albert Astals Cid (aacid) wrote :

In the krillin phone
  autopilot3 run unity8.indicators.tests.test_action_latency.TestBuffering.test_slider_buffers_activations

Still gives me

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/unity8/indicators/tests/test_action_latency.py", line 163, in test_slider_buffers_activations
    NotEquals(final_value)
  File "/usr/lib/python3/dist-packages/testtools/testcase.py", line 423, in assertThat
    raise mismatch_error
testtools.matchers._impl.MismatchError: 1.0 == 1.0

Am i actually right in thinking this branch has to fix that autopilot test or am i testing the wrong thing?

review: Needs Fixing
Revision history for this message
Albert Astals Cid (aacid) wrote :

Ok, together with the required MP i can confirm this fixes unity8.indicators.tests.test_action_latency.TestClientRevertsToServerValue.test_slider_reverts_on_late_response and unity8.indicators.tests.test_action_latency.TestBuffering.test_slider_buffers_activations

Now to review the code

Revision history for this message
Albert Astals Cid (aacid) wrote :

 * Did you perform an exploratory manual test run of the code change and any related functionality?
Yes

 * Did CI run pass? If not, please explain why.
Yes

 * Did you make sure that the branch does not contain spurious tags?
Yes

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'plugins/Ubuntu/Settings/Components/serverpropertysynchroniser.cpp'
--- plugins/Ubuntu/Settings/Components/serverpropertysynchroniser.cpp 2015-03-25 09:16:06 +0000
+++ plugins/Ubuntu/Settings/Components/serverpropertysynchroniser.cpp 2015-05-06 16:32:05 +0000
@@ -29,10 +29,12 @@
29 , m_connectedServerTarget(nullptr)29 , m_connectedServerTarget(nullptr)
30 , m_connectedUserTarget(nullptr)30 , m_connectedUserTarget(nullptr)
31 , m_serverSyncTimer(new QTimer(this))31 , m_serverSyncTimer(new QTimer(this))
32 , m_bufferTimeout(nullptr)32 , m_bufferDamper(nullptr)
33 , m_useWaitBuffer(true)33 , m_useWaitBuffer(true)
34 , m_buffering(false)34 , m_haveNextActivate(false)
35 , m_bufferedSyncTimeout(false)35 , m_bufferedSyncTimeout(false)
36 , m_serverUpdatedDuringBufferDamping(false)
37 , m_activateCount(0)
36{38{
37 m_serverSyncTimer->setSingleShot(true);39 m_serverSyncTimer->setSingleShot(true);
38 m_serverSyncTimer->setInterval(30000);40 m_serverSyncTimer->setInterval(30000);
@@ -57,8 +59,10 @@
57 m_serverSyncTimer->stop();59 m_serverSyncTimer->stop();
58 Q_EMIT syncWaitingChanged(false);60 Q_EMIT syncWaitingChanged(false);
59 }61 }
60 if (m_bufferTimeout) m_bufferTimeout->stop();62 if (m_bufferDamper) m_bufferDamper->stop();
61 m_buffering = false;63 m_haveNextActivate = false;
64 m_activateCount = 0;
65 m_serverUpdatedDuringBufferDamping = false;
62}66}
6367
64QObject *ServerPropertySynchroniser::serverTarget() const68QObject *ServerPropertySynchroniser::serverTarget() const
@@ -164,31 +168,31 @@
164168
165int ServerPropertySynchroniser::maximumWaitBufferInterval() const169int ServerPropertySynchroniser::maximumWaitBufferInterval() const
166{170{
167 return m_bufferTimeout ? m_bufferTimeout->interval() : -1;171 return m_bufferDamper ? m_bufferDamper->interval() : -1;
168}172}
169173
170void ServerPropertySynchroniser::setMaximumWaitBufferInterval(int timeout)174void ServerPropertySynchroniser::setMaximumWaitBufferInterval(int timeout)
171{175{
172 if (timeout >= 0) {176 if (timeout >= 0) {
173 if (!m_bufferTimeout) {177 if (!m_bufferDamper) {
174 m_bufferTimeout = new QTimer(this);178 m_bufferDamper = new QTimer(this);
175 m_bufferTimeout->setInterval(timeout);179 m_bufferDamper->setInterval(timeout);
176 m_bufferTimeout->setSingleShot(true);180 m_bufferDamper->setSingleShot(true);
177 connect(m_bufferTimeout, &QTimer::timeout, this, &ServerPropertySynchroniser::bufferTimedOut);181 connect(m_bufferDamper, &QTimer::timeout, this, &ServerPropertySynchroniser::bufferTimedOut);
178182
179 Q_EMIT maximumWaitBufferIntervalChanged(timeout);183 Q_EMIT maximumWaitBufferIntervalChanged(timeout);
180 }184 }
181 else if (timeout != m_bufferTimeout->interval()) {185 else if (timeout != m_bufferDamper->interval()) {
182 m_bufferTimeout->setInterval(timeout);186 m_bufferDamper->setInterval(timeout);
183 Q_EMIT maximumWaitBufferIntervalChanged(timeout);187 Q_EMIT maximumWaitBufferIntervalChanged(timeout);
184 }188 }
185189
186 } else if (m_bufferTimeout) {190 } else if (m_bufferDamper) {
187 if (m_bufferTimeout->isActive()) {191 if (m_bufferDamper->isActive()) {
188 m_buffering = false;192 m_haveNextActivate = false;
189 }193 }
190 delete m_bufferTimeout;194 delete m_bufferDamper;
191 m_bufferTimeout = nullptr;195 m_bufferDamper = nullptr;
192 Q_EMIT maximumWaitBufferIntervalChanged(timeout);196 Q_EMIT maximumWaitBufferIntervalChanged(timeout);
193 }197 }
194}198}
@@ -219,16 +223,16 @@
219223
220 if (m_useWaitBuffer) {224 if (m_useWaitBuffer) {
221 // Dampen the activations? Buffer the change.225 // Dampen the activations? Buffer the change.
222 if (m_bufferTimeout) {226 if (m_bufferDamper) {
223 if (m_bufferTimeout->isActive()) {227 if (m_bufferDamper->isActive()) {
224 m_buffering = true;228 m_haveNextActivate = true;
225 m_busy = false;229 m_busy = false;
226 return;230 return;
227 }231 }
228 m_bufferTimeout->start();232 m_bufferDamper->start();
229 // Not using a buffer timer? Buffer the change till server timeout233 // Not using a damp interval? Buffer the change till we get a server response, or timeout
230 } else if (m_serverSyncTimer->isActive()) {234 } else if (m_serverSyncTimer->isActive()) {
231 m_buffering = true;235 m_haveNextActivate = true;
232 m_busy = false;236 m_busy = false;
233 return;237 return;
234 }238 }
@@ -236,6 +240,7 @@
236240
237 m_serverSyncTimer->start();241 m_serverSyncTimer->start();
238 Q_EMIT syncWaitingChanged(true);242 Q_EMIT syncWaitingChanged(true);
243 m_activateCount++;
239244
240 // Fire off a change to the server user property value245 // Fire off a change to the server user property value
241 QQmlProperty userProp(m_userTarget, m_userProperty);246 QQmlProperty userProp(m_userTarget, m_userProperty);
@@ -251,6 +256,7 @@
251{256{
252 // if we havent finished constructing the class, then wait257 // if we havent finished constructing the class, then wait
253 if (!m_classComplete) return;258 if (!m_classComplete) return;
259 reset();
254260
255 if (m_connectedServerTarget) QObject::disconnect(m_connectedServerTarget, 0, this, 0);261 if (m_connectedServerTarget) QObject::disconnect(m_connectedServerTarget, 0, this, 0);
256 if (!m_serverTarget || m_serverProperty.isEmpty()) {262 if (!m_serverTarget || m_serverProperty.isEmpty()) {
@@ -273,6 +279,7 @@
273{279{
274 // if we havent finished constructing the class, then wait280 // if we havent finished constructing the class, then wait
275 if (!m_classComplete) return;281 if (!m_classComplete) return;
282 reset();
276283
277 if (m_connectedUserTarget) QObject::disconnect(m_connectedUserTarget, 0, this, 0);284 if (m_connectedUserTarget) QObject::disconnect(m_connectedUserTarget, 0, this, 0);
278 if (!m_userTarget) {285 if (!m_userTarget) {
@@ -313,13 +320,21 @@
313 if (m_busy) return;320 if (m_busy) return;
314 m_busy = true;321 m_busy = true;
315322
316 bool waitingBufferedServerChange = m_bufferTimeout && m_bufferTimeout->isActive();323 // Are we waiting for a server sync.
317
318 // If we've been waiting for a sync, stop the wait.
319 if (m_serverSyncTimer->isActive()) {324 if (m_serverSyncTimer->isActive()) {
325 // are we waiting for more from the server? (number of activates sent > number of receives)
326 if (--m_activateCount > 0) {
327 // ignore the change and update on server sync timeout.
328 m_busy = false;
329 return;
330 }
331
332 // stop the wait
320 m_serverSyncTimer->stop();333 m_serverSyncTimer->stop();
321 Q_EMIT syncWaitingChanged(false);334 Q_EMIT syncWaitingChanged(false);
322 }335 }
336 m_activateCount = 0;
337 m_serverUpdatedDuringBufferDamping = m_bufferDamper && m_bufferDamper->isActive();
323338
324 QQmlProperty userProp(m_userTarget, m_userProperty);339 QQmlProperty userProp(m_userTarget, m_userProperty);
325 QQmlProperty serverProp(m_serverTarget, m_serverProperty);340 QQmlProperty serverProp(m_serverTarget, m_serverProperty);
@@ -331,8 +346,8 @@
331 // If we've been buffering changes since last change was send,346 // If we've been buffering changes since last change was send,
332 // we verify that what the server gave us is what we want, and send another347 // we verify that what the server gave us is what we want, and send another
333 // activation if not.348 // activation if not.
334 if (m_buffering) {349 if (m_haveNextActivate) {
335 m_buffering = false;350 m_haveNextActivate = false;
336 m_busy = false;351 m_busy = false;
337 if (serverProp.read() != userProp.read()) {352 if (serverProp.read() != userProp.read()) {
338 activate();353 activate();
@@ -341,7 +356,7 @@
341 }356 }
342357
343 // Don't update until we hit the buffer timeout.358 // Don't update until we hit the buffer timeout.
344 if (waitingBufferedServerChange) {359 if (m_serverUpdatedDuringBufferDamping) {
345 m_busy = false;360 m_busy = false;
346 return;361 return;
347 }362 }
@@ -353,8 +368,8 @@
353368
354void ServerPropertySynchroniser::serverSyncTimedOut()369void ServerPropertySynchroniser::serverSyncTimedOut()
355{370{
356 if (m_buffering && !m_bufferedSyncTimeout) {371 if (m_haveNextActivate && !m_bufferedSyncTimeout) {
357 m_buffering = false;372 m_haveNextActivate = false;
358 }373 }
359 Q_EMIT syncWaitingChanged(false);374 Q_EMIT syncWaitingChanged(false);
360 updateUserValue();375 updateUserValue();
@@ -362,10 +377,13 @@
362377
363void ServerPropertySynchroniser::bufferTimedOut()378void ServerPropertySynchroniser::bufferTimedOut()
364{379{
365 if (m_buffering) {380 if (m_haveNextActivate) {
366 m_buffering = false;381 m_haveNextActivate = false;
367 activate();382 activate();
368 } else {383 }
384 // if we received a server change while we were in change buffer but don't need to send another activate,
385 // update to the value we received.
386 else if (m_serverUpdatedDuringBufferDamping) {
369 // Update the user value.387 // Update the user value.
370 if (m_busy) return;388 if (m_busy) return;
371 m_busy = true;389 m_busy = true;
@@ -379,4 +397,5 @@
379 userProp.write(serverProp.read());397 userProp.write(serverProp.read());
380 m_busy = false;398 m_busy = false;
381 }399 }
400 m_serverUpdatedDuringBufferDamping = false;
382}401}
383402
=== modified file 'plugins/Ubuntu/Settings/Components/serverpropertysynchroniser.h'
--- plugins/Ubuntu/Settings/Components/serverpropertysynchroniser.h 2015-03-25 09:16:06 +0000
+++ plugins/Ubuntu/Settings/Components/serverpropertysynchroniser.h 2015-05-06 16:32:05 +0000
@@ -148,10 +148,13 @@
148 QObject* m_connectedUserTarget;148 QObject* m_connectedUserTarget;
149149
150 QTimer* m_serverSyncTimer;150 QTimer* m_serverSyncTimer;
151 QTimer* m_bufferTimeout;151 QTimer* m_bufferDamper;
152 bool m_useWaitBuffer;152 bool m_useWaitBuffer;
153 bool m_buffering;153 bool m_haveNextActivate;
154 bool m_bufferedSyncTimeout;154 bool m_bufferedSyncTimeout;
155 bool m_serverUpdatedDuringBufferDamping;
156
157 int m_activateCount;
155};158};
156159
157#endif // SERVERPROPERTYSYNCHRONISER_H160#endif // SERVERPROPERTYSYNCHRONISER_H
158161
=== modified file 'tests/qmltests/Components/tst_ServerPropertySynchroniser.qml'
--- tests/qmltests/Components/tst_ServerPropertySynchroniser.qml 2015-03-25 09:16:06 +0000
+++ tests/qmltests/Components/tst_ServerPropertySynchroniser.qml 2015-05-06 16:32:05 +0000
@@ -58,7 +58,7 @@
58 property var changeToValue: undefined58 property var changeToValue: undefined
5959
60 property Timer timer: Timer {60 property Timer timer: Timer {
61 interval: 20061 interval: 2000
6262
63 onTriggered: {63 onTriggered: {
64 sliderBackend.value = sliderBackend.changeToValue;64 sliderBackend.value = sliderBackend.changeToValue;
@@ -165,6 +165,7 @@
165 objectName: "sliderSync"165 objectName: "sliderSync"
166166
167 syncTimeout: 3000167 syncTimeout: 3000
168 maximumWaitBufferInterval: 50
168169
169 userTarget: slider170 userTarget: slider
170 userProperty: "value"171 userProperty: "value"
@@ -276,14 +277,20 @@
276277
277 function init() {278 function init() {
278 waitForRendering(root);279 waitForRendering(root);
280
281 switchSync.reset();
282 checkSync.reset();
283 sliderSync.reset();
284 apMenuSync.reset();
285
279 switchBackend.timer.interval = 100;286 switchBackend.timer.interval = 100;
280 checkBackend.timer.interval = 100;287 checkBackend.timer.interval = 100;
281 sliderBackend.timer.interval = 100;288 sliderBackend.timer.interval = 200;
282 apBackend.timer.interval = 100;289 apBackend.timer.interval = 100;
283290
284 switchSync.syncTimeout = 200;291 switchSync.syncTimeout = 200;
285 checkSync.syncTimeout = 200;292 checkSync.syncTimeout = 200;
286 sliderSync.syncTimeout = 200;293 sliderSync.syncTimeout = 400;
287 apMenuSync.syncTimeout = 200;294 apMenuSync.syncTimeout = 200;
288295
289 sliderSyncActivatedSpy.clear();296 sliderSyncActivatedSpy.clear();
@@ -300,10 +307,6 @@
300 sliderBackend.value = 50;307 sliderBackend.value = 50;
301 apBackend.active = false;308 apBackend.active = false;
302309
303 switchSync.reset();
304 checkSync.reset();
305 sliderSync.reset();
306 apMenuSync.reset();
307 sliderSync.maximumWaitBufferInterval = -1310 sliderSync.maximumWaitBufferInterval = -1
308311
309 tryCompare(switchBackend, "inSync", true);312 tryCompare(switchBackend, "inSync", true);
@@ -350,19 +353,21 @@
350 }353 }
351354
352 function test_buffered_change_with_maximum_interval() {355 function test_buffered_change_with_maximum_interval() {
353 sliderSync.maximumWaitBufferInterval = 50;356 sliderSync.maximumWaitBufferInterval = 25;
354 sliderSync.syncTimeout = 5000;357 sliderSync.syncTimeout = 5000;
355358
356 slider.value = 60;359 slider.value = 60;
357 compare(sliderSyncActivatedSpy.count, 1, "activated signal should have been sent")360 compare(sliderSyncActivatedSpy.count, 1, "activated signal should have been sent");
358 slider.value = 70;361 slider.value = 70;
359 slider.value = 80;362 slider.value = 80;
360 tryCompare(sliderSyncActivatedSpy, "count", 2, 500, "activated signal should have been sent after max interval")363 wait(100); // wait for buffer timeout
364 tryCompare(sliderSyncActivatedSpy, "count", 2, 1000, "aditional activate signal should have been sent");
365 compare(slider.value, 80, "value should be set to last activate");
366
361 slider.value = 90;367 slider.value = 90;
362 tryCompare(sliderSyncActivatedSpy, "count", 3, 500, "activated signal should have been sent after max interval")368 wait(100); // wait for buffer timeout
363 slider.value = 100;369 tryCompare(sliderSyncActivatedSpy, "count", 3, 1000, "aditional activate signal should have been sent");
364 tryCompare(sliderSyncActivatedSpy, "count", 4, 500);370 compare(slider.value, 90, "value should be set to last activate");
365 tryCompare(sliderBackend, "value", 100);
366 }371 }
367372
368 function test_connect_to_another_object() {373 function test_connect_to_another_object() {

Subscribers

People subscribed via source and target branches

to all changes: