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

[Nick Dedekind]

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

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

[PS Jenkins bot (ps-jenkins)]

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]

89. By Nick Dedekind on 2015-04-24

better test for max_interval

[PS Jenkins bot (ps-jenkins)]

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)

[Albert Astals Cid (aacid)]

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]

90. By Nick Dedekind on 2015-05-06

Fixed some more problems with activation damping

[PS Jenkins bot (ps-jenkins)]

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)

[Albert Astals Cid (aacid)]

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

[Albert Astals Cid (aacid)]

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

[Albert Astals Cid (aacid)]

 * 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

=== 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 @@
     , m_connectedServerTarget(nullptr)
     , m_connectedUserTarget(nullptr)
     , m_serverSyncTimer(new QTimer(this))
-    , m_bufferTimeout(nullptr)
+    , m_bufferDamper(nullptr)
     , m_useWaitBuffer(true)
-    , m_buffering(false)
+    , m_haveNextActivate(false)
     , m_bufferedSyncTimeout(false)
+    , m_serverUpdatedDuringBufferDamping(false)
+    , m_activateCount(0)
 {
     m_serverSyncTimer->setSingleShot(true);
     m_serverSyncTimer->setInterval(30000);
@@ -57,8 +59,10 @@
         m_serverSyncTimer->stop();
         Q_EMIT syncWaitingChanged(false);
     }
-    if (m_bufferTimeout) m_bufferTimeout->stop();
-    m_buffering = false;
+    if (m_bufferDamper) m_bufferDamper->stop();
+    m_haveNextActivate = false;
+    m_activateCount = 0;
+    m_serverUpdatedDuringBufferDamping = false;
 }
 
 QObject *ServerPropertySynchroniser::serverTarget() const
@@ -164,31 +168,31 @@
 
 int ServerPropertySynchroniser::maximumWaitBufferInterval() const
 {
-    return m_bufferTimeout ? m_bufferTimeout->interval() : -1;
+    return m_bufferDamper ? m_bufferDamper->interval() : -1;
 }
 
 void ServerPropertySynchroniser::setMaximumWaitBufferInterval(int timeout)
 {
     if (timeout >= 0) {
-        if (!m_bufferTimeout) {
-            m_bufferTimeout = new QTimer(this);
-            m_bufferTimeout->setInterval(timeout);
-            m_bufferTimeout->setSingleShot(true);
-            connect(m_bufferTimeout, &QTimer::timeout, this, &ServerPropertySynchroniser::bufferTimedOut);
-
-            Q_EMIT maximumWaitBufferIntervalChanged(timeout);
-        }
-        else if (timeout != m_bufferTimeout->interval()) {
-            m_bufferTimeout->setInterval(timeout);
-            Q_EMIT maximumWaitBufferIntervalChanged(timeout);
-        }
-
-    } else if (m_bufferTimeout) {
-        if (m_bufferTimeout->isActive()) {
-            m_buffering = false;
-        }
-        delete m_bufferTimeout;
-        m_bufferTimeout = nullptr;
+        if (!m_bufferDamper) {
+            m_bufferDamper = new QTimer(this);
+            m_bufferDamper->setInterval(timeout);
+            m_bufferDamper->setSingleShot(true);
+            connect(m_bufferDamper, &QTimer::timeout, this, &ServerPropertySynchroniser::bufferTimedOut);
+
+            Q_EMIT maximumWaitBufferIntervalChanged(timeout);
+        }
+        else if (timeout != m_bufferDamper->interval()) {
+            m_bufferDamper->setInterval(timeout);
+            Q_EMIT maximumWaitBufferIntervalChanged(timeout);
+        }
+
+    } else if (m_bufferDamper) {
+        if (m_bufferDamper->isActive()) {
+            m_haveNextActivate = false;
+        }
+        delete m_bufferDamper;
+        m_bufferDamper = nullptr;
         Q_EMIT maximumWaitBufferIntervalChanged(timeout);
     }
 }
@@ -219,16 +223,16 @@
 
     if (m_useWaitBuffer) {
         // Dampen the activations? Buffer the change.
-        if (m_bufferTimeout) {
-            if (m_bufferTimeout->isActive()) {
-                m_buffering = true;
+        if (m_bufferDamper) {
+            if (m_bufferDamper->isActive()) {
+                m_haveNextActivate = true;
                 m_busy = false;
                 return;
             }
-            m_bufferTimeout->start();
-        // Not using a buffer timer? Buffer the change till server timeout
+            m_bufferDamper->start();
+        // Not using a damp interval? Buffer the change till we get a server response, or timeout
         } else if (m_serverSyncTimer->isActive()) {
-            m_buffering = true;
+            m_haveNextActivate = true;
             m_busy = false;
             return;
         }
@@ -236,6 +240,7 @@
 
     m_serverSyncTimer->start();
     Q_EMIT syncWaitingChanged(true);
+    m_activateCount++;
 
     // Fire off a change to the server user property value
     QQmlProperty userProp(m_userTarget, m_userProperty);
@@ -251,6 +256,7 @@
 {
     // if we havent finished constructing the class, then wait
     if (!m_classComplete) return;
+    reset();
 
     if (m_connectedServerTarget) QObject::disconnect(m_connectedServerTarget, 0, this, 0);
     if (!m_serverTarget || m_serverProperty.isEmpty()) {
@@ -273,6 +279,7 @@
 {
     // if we havent finished constructing the class, then wait
     if (!m_classComplete) return;
+    reset();
 
     if (m_connectedUserTarget) QObject::disconnect(m_connectedUserTarget, 0, this, 0);
     if (!m_userTarget) {
@@ -313,13 +320,21 @@
     if (m_busy) return;
     m_busy = true;
 
-    bool waitingBufferedServerChange = m_bufferTimeout && m_bufferTimeout->isActive();
-
-    // If we've been waiting for a sync, stop the wait.
+    // Are we waiting for a server sync.
     if (m_serverSyncTimer->isActive()) {
+        // are we waiting for more from the server? (number of activates sent > number of receives)
+        if (--m_activateCount > 0) {
+            // ignore the change and update on server sync timeout.
+            m_busy = false;
+            return;
+        }
+
+        // stop the wait
         m_serverSyncTimer->stop();
         Q_EMIT syncWaitingChanged(false);
     }
+    m_activateCount = 0;
+    m_serverUpdatedDuringBufferDamping = m_bufferDamper && m_bufferDamper->isActive();
 
     QQmlProperty userProp(m_userTarget, m_userProperty);
     QQmlProperty serverProp(m_serverTarget, m_serverProperty);
@@ -331,8 +346,8 @@
     // If we've been buffering changes since last change was send,
     // we verify that what the server gave us is what we want, and send another
     // activation if not.
-    if (m_buffering) {
-        m_buffering = false;
+    if (m_haveNextActivate) {
+        m_haveNextActivate = false;
         m_busy = false;
         if (serverProp.read() != userProp.read()) {
             activate();
@@ -341,7 +356,7 @@
     }
 
     // Don't update until we hit the buffer timeout.
-    if (waitingBufferedServerChange) {
+    if (m_serverUpdatedDuringBufferDamping) {
         m_busy = false;
         return;
     }
@@ -353,8 +368,8 @@
 
 void ServerPropertySynchroniser::serverSyncTimedOut()
 {
-    if (m_buffering && !m_bufferedSyncTimeout) {
-        m_buffering = false;
+    if (m_haveNextActivate && !m_bufferedSyncTimeout) {
+        m_haveNextActivate = false;
     }
     Q_EMIT syncWaitingChanged(false);
     updateUserValue();
@@ -362,10 +377,13 @@
 
 void ServerPropertySynchroniser::bufferTimedOut()
 {
-    if (m_buffering) {
-        m_buffering = false;
+    if (m_haveNextActivate) {
+        m_haveNextActivate = false;
         activate();
-    } else {
+    }
+    // if we received a server change while we were in change buffer but don't need to send another activate,
+    // update to the value we received.
+    else if (m_serverUpdatedDuringBufferDamping) {
         // Update the user value.
         if (m_busy) return;
         m_busy = true;
@@ -379,4 +397,5 @@
         userProp.write(serverProp.read());
         m_busy = false;
     }
+    m_serverUpdatedDuringBufferDamping = false;
 }

=== 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 @@
     QObject* m_connectedUserTarget;
 
     QTimer* m_serverSyncTimer;
-    QTimer* m_bufferTimeout;
+    QTimer* m_bufferDamper;
     bool m_useWaitBuffer;
-    bool m_buffering;
+    bool m_haveNextActivate;
     bool m_bufferedSyncTimeout;
+    bool m_serverUpdatedDuringBufferDamping;
+
+    int m_activateCount;
 };
 
 #endif // SERVERPROPERTYSYNCHRONISER_H

=== 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 @@
         property var changeToValue: undefined
 
         property Timer timer: Timer {
-            interval: 200
+            interval: 2000
 
             onTriggered: {
                 sliderBackend.value = sliderBackend.changeToValue;
@@ -165,6 +165,7 @@
                     objectName: "sliderSync"
 
                     syncTimeout: 3000
+                    maximumWaitBufferInterval: 50
 
                     userTarget: slider
                     userProperty: "value"
@@ -276,14 +277,20 @@
 
         function init() {
             waitForRendering(root);
+
+            switchSync.reset();
+            checkSync.reset();
+            sliderSync.reset();
+            apMenuSync.reset();
+
             switchBackend.timer.interval = 100;
             checkBackend.timer.interval = 100;
-            sliderBackend.timer.interval = 100;
+            sliderBackend.timer.interval = 200;
             apBackend.timer.interval = 100;
 
             switchSync.syncTimeout = 200;
             checkSync.syncTimeout = 200;
-            sliderSync.syncTimeout = 200;
+            sliderSync.syncTimeout = 400;
             apMenuSync.syncTimeout = 200;
 
             sliderSyncActivatedSpy.clear();
@@ -300,10 +307,6 @@
             sliderBackend.value = 50;
             apBackend.active = false;
 
-            switchSync.reset();
-            checkSync.reset();
-            sliderSync.reset();
-            apMenuSync.reset();
             sliderSync.maximumWaitBufferInterval = -1
 
             tryCompare(switchBackend, "inSync", true);
@@ -350,19 +353,21 @@
         }
 
         function test_buffered_change_with_maximum_interval() {
-            sliderSync.maximumWaitBufferInterval = 50;
+            sliderSync.maximumWaitBufferInterval = 25;
             sliderSync.syncTimeout = 5000;
 
             slider.value = 60;
-            compare(sliderSyncActivatedSpy.count, 1, "activated signal should have been sent")
+            compare(sliderSyncActivatedSpy.count, 1, "activated signal should have been sent");
             slider.value = 70;
             slider.value = 80;
-            tryCompare(sliderSyncActivatedSpy, "count", 2, 500, "activated signal should have been sent after max interval")
+            wait(100); // wait for buffer timeout
+            tryCompare(sliderSyncActivatedSpy, "count", 2, 1000, "aditional activate signal should have been sent");
+            compare(slider.value, 80, "value should be set to last activate");
+
             slider.value = 90;
-            tryCompare(sliderSyncActivatedSpy, "count", 3, 500, "activated signal should have been sent after max interval")
-            slider.value = 100;
-            tryCompare(sliderSyncActivatedSpy, "count", 4, 500);
-            tryCompare(sliderBackend, "value", 100);
+            wait(100); // wait for buffer timeout
+            tryCompare(sliderSyncActivatedSpy, "count", 3, 1000, "aditional activate signal should have been sent");
+            compare(slider.value, 90, "value should be set to last activate");
         }
 
         function test_connect_to_another_object() {