Merge lp:~mikemc/ubuntuone-control-panel/fix-sync-status into lp:ubuntuone-control-panel

[Mike McCracken]

- Fix sync indicator in control panel main window. (LP: #1044197)

- Fix sync indicator in control panel main window. (LP: #1044197)

Added support in controlbackend for multiple status handlers.

Both the filsyncstatus.py widget in the main window and the menu item
in systray.py try to set the handler. This results in the
filesyncstatus widget no longer being updated, as the single variable
holding the callback in controlbackend is overwritten.

The issue does not appear on linux, because although the
controlbackend only has one variable, when it is changed, it wraps the
new callback and calls sd_client.set_status_changed_handler() with
that wrapper. On linux, the dbus implementation of the proxy that
sd_client uses *ADDS* the wrapper, while on the other platforms the
twisted implementation just overwrites the old handler.

This branch changes it so that we only ever set the sd_client callback
once, to a function in controlbackend that calls each of a list of
callbacks.

The filesyncstatus and systray widgets now call an
add_status_changed_handler function that adds to that list instead of
setting the single callback via a property.

Tests are included that use the new API, but would break if the new
API behaved the same as the old one.

IRL test shows this working on darwin. I had great difficulty getting
windows running from source, so I didn't do an IRL test on windows.

[Roberto Alsina (ralsina)]

Looks good to me!

review: Approve

[Alejandro J. Cura (alecu)]

Looks great. I requested on IRC a small removal, will approve when that gets pushed.

review: Needs Fixing

[lp:~mikemc/ubuntuone-control-panel/fix-sync-status]

359. By Mike McCracken on 2012-09-06

remove unnecessary property for accessing status_changed_handlers

[Alejandro J. Cura (alecu)]

+1

review: Approve

=== modified file 'ubuntuone/controlpanel/backend.py'
--- ubuntuone/controlpanel/backend.py	2012-08-30 00:18:06 +0000
+++ ubuntuone/controlpanel/backend.py	2012-09-06 16:37:20 +0000
@@ -130,7 +130,7 @@
 
     def __init__(self, shutdown_func=None):
         """Initialize the web_client."""
-        self._status_changed_handler = None
+        self._status_changed_handlers = []
         self._credentials = None
         self._volumes = {}  # cache last known volume info
 
@@ -139,6 +139,7 @@
 
         self.login_client = CredentialsManagementTool()
         self.sd_client = sd_client.SyncDaemonClient()
+        self.is_sd_client_handler_set = False
         self.wc = WebClient(self.get_credentials)
 
         logger.info('ControlBackend: instance started.')
@@ -202,24 +203,27 @@
         else:
             return result
 
-    def _set_status_changed_handler(self, handler):
-        """Set 'handler' to be called when file sync status changes."""
-        logger.debug('setting status_changed_handler to %r', handler)
-
-        def process_and_callback(status):
-            """Process syncdaemon's status and callback 'handler'."""
-            result = self._process_file_sync_status(status)
+    def process_status_and_call_handlers(self, status):
+        """Process syncdaemon's status and call each of the handlers."""
+        result = self._process_file_sync_status(status)
+        for handler in self._status_changed_handlers:
             handler(result)
 
-        self._status_changed_handler = handler
-        self.sd_client.set_status_changed_handler(process_and_callback)
-
-    def _get_status_changed_handler(self):
-        """Return the handler to be called when file sync status changes."""
-        return self._status_changed_handler
-
-    status_changed_handler = property(_get_status_changed_handler,
-                                      _set_status_changed_handler)
+    def add_status_changed_handler(self, handler):
+        """Add 'handler' to list of callbacks for file sync status changes."""
+
+        if handler in self._status_changed_handlers:
+            logger.debug('Handler %r already in list of handlers, ignoring.' %
+                         handler)
+            return
+
+        logger.debug('adding %r to list of status_changed_handlers', handler)
+
+        self._status_changed_handlers.append(handler)
+        if not self.is_sd_client_handler_set:
+            cb = self.process_status_and_call_handlers
+            self.sd_client.set_status_changed_handler(cb)
+            self.is_sd_client_handler_set = True
 
     @inlineCallbacks
     def _process_device_web_info(self, devices,

=== modified file 'ubuntuone/controlpanel/dbus_service.py'
--- ubuntuone/controlpanel/dbus_service.py	2012-01-23 20:15:09 +0000
+++ ubuntuone/controlpanel/dbus_service.py	2012-09-06 16:37:20 +0000
@@ -278,8 +278,7 @@
     @method(dbus_interface=DBUS_PREFERENCES_IFACE, in_signature="")
     def file_sync_status(self):
         """Get the status of the file sync service."""
-        if self.backend.status_changed_handler is None:
-            self.backend.status_changed_handler = self.process_status
+        self.backend.add_status_changed_handler(self.process_status)
 
         d = self.backend.file_sync_status()
         d.addCallback(self.process_status)

=== modified file 'ubuntuone/controlpanel/dbustests/test_dbus_service.py'
--- ubuntuone/controlpanel/dbustests/test_dbus_service.py	2011-10-24 22:14:32 +0000
+++ ubuntuone/controlpanel/dbustests/test_dbus_service.py	2012-09-06 16:37:20 +0000
@@ -148,7 +148,7 @@
 
     def __init__(self, shutdown_func=None):
         MockBackend.shutdown_func = shutdown_func
-        self._status_changed_handler = []
+        self._status_changed_handlers = []
 
     def _process(self, result):
         """Process the request with the given result."""
@@ -157,23 +157,10 @@
             return defer.fail(self.exception(result))
         return defer.succeed(result)
 
-    def _set_status_changed_handler(self, handler):
+    def add_status_changed_handler(self, handler):
         """Set 'handler' to be called when file sync status changes."""
-        if handler is not None:
-            self._status_changed_handler.append(handler)
-        else:
-            self._status_changed_handler = []
-
-    def _get_status_changed_handler(self):
-        """Return the handler to be called when file sync status changes."""
-        if len(self._status_changed_handler) > 0:
-            result = self._status_changed_handler[-1]
-        else:
-            result = None
-        return result
-
-    status_changed_handler = property(_get_status_changed_handler,
-                                      _set_status_changed_handler)
+        if handler not in self._status_changed_handlers:
+            self._status_changed_handlers.append(handler)
 
     def account_info(self):
         """Get the user account info."""
@@ -751,29 +738,29 @@
             return self.assert_correct_status_signal(*args, expected_msg=arg)
 
     def test_status_changed_handler(self):
-        """The status changed handler is properly set."""
+        """There is no default status changed handler."""
         be = MockBackend()
         dbus_service.ControlPanelBackend(backend=be)
-
-        self.assertEqual(be.status_changed_handler, None)
+        # pylint: disable=W0212
+        self.assertEqual(be._status_changed_handlers, [])
 
     def test_status_changed_handler_after_status_requested(self):
-        """The status changed handler is properly set."""
+        """file_sync_status sets the callback in the CP backend."""
         be = MockBackend()
         cpbe = dbus_service.ControlPanelBackend(backend=be)
         cpbe.file_sync_status()
-
-        self.assertEqual(be.status_changed_handler, cpbe.process_status)
+        # pylint: disable=W0212
+        self.assertEqual(be._status_changed_handlers, [cpbe.process_status])
 
     def test_status_changed_handler_after_status_requested_twice(self):
-        """The status changed handler is properly set."""
+        """The Handler is only added once after 2 calls to file_sync_status."""
         be = MockBackend()
         cpbe = dbus_service.ControlPanelBackend(backend=be)
         cpbe.file_sync_status()
         cpbe.file_sync_status()
 
         # pylint: disable=W0212
-        self.assertEqual(be._status_changed_handler, [cpbe.process_status])
+        self.assertEqual(be._status_changed_handlers, [cpbe.process_status])
 
 
 class ShutdownTestCase(BaseTestCase):

=== modified file 'ubuntuone/controlpanel/gui/qt/filesyncstatus.py'
--- ubuntuone/controlpanel/gui/qt/filesyncstatus.py	2012-08-08 18:36:45 +0000
+++ ubuntuone/controlpanel/gui/qt/filesyncstatus.py	2012-09-06 16:37:20 +0000
@@ -61,7 +61,7 @@
         """Load specific tab info."""
         info = yield self.backend.file_sync_status()
         self.process_info(info)
-        self.backend.status_changed_handler = self.process_info
+        self.backend.add_status_changed_handler(self.process_info)
 
     @log_call(logger.debug)
     def process_info(self, status):

=== modified file 'ubuntuone/controlpanel/gui/qt/systray.py'
--- ubuntuone/controlpanel/gui/qt/systray.py	2012-08-20 18:53:30 +0000
+++ ubuntuone/controlpanel/gui/qt/systray.py	2012-09-06 16:37:20 +0000
@@ -127,6 +127,7 @@
         self.get_more_storage = None
         self.transfers = None
         self.open_u1_folder = None
+        self.backend.add_status_changed_handler(self._process_status)
 
         self.load_menu()
         # Refresh the Shares every five minutes if needed.
@@ -204,7 +205,6 @@
         """Update Ubuntu One status."""
         info = yield self.backend.file_sync_status()
         self._process_status(info)
-        self.backend.status_changed_handler = self._process_status
 
     def _process_status(self, status):
         """Match status with signals."""

=== modified file 'ubuntuone/controlpanel/gui/qt/tests/__init__.py'
--- ubuntuone/controlpanel/gui/qt/tests/__init__.py	2012-08-28 00:32:41 +0000
+++ ubuntuone/controlpanel/gui/qt/tests/__init__.py	2012-09-06 16:37:20 +0000
@@ -123,6 +123,7 @@
     next_result = []
     exposed_methods = [
         'account_info',
+        'add_status_changed_handler',
         'build_signed_iri',
         'change_device_settings',
         'change_file_sync_settings',

=== modified file 'ubuntuone/controlpanel/gui/qt/tests/test_filesyncstatus.py'
--- ubuntuone/controlpanel/gui/qt/tests/test_filesyncstatus.py	2012-08-08 18:36:45 +0000
+++ ubuntuone/controlpanel/gui/qt/tests/test_filesyncstatus.py	2012-09-06 16:37:20 +0000
@@ -109,10 +109,11 @@
         actual_text = unicode(self.ui.ui.sync_status_label.text())
         self.assertEqual(qt.FILE_SYNC_STARTING, actual_text)
 
-    def test_process_info_changed(self):
+    def test_status_changed_handler_is_set(self):
         """Backend's file sync status changed callback is connected."""
-        self.assertEqual(self.ui.backend.status_changed_handler,
-                         self.ui.process_info)
+        # pylint: disable=W0212
+        self.assertEqual(self.ui.backend._called['add_status_changed_handler'],
+                         ((self.ui.process_info,), {}))
 
     def test_file_sync_status_is_requested_on_load(self):
         """The file sync status is requested to the backend."""

=== modified file 'ubuntuone/controlpanel/gui/qt/tests/test_systray.py'
--- ubuntuone/controlpanel/gui/qt/tests/test_systray.py	2012-08-28 00:32:41 +0000
+++ ubuntuone/controlpanel/gui/qt/tests/test_systray.py	2012-09-06 16:37:20 +0000
@@ -107,11 +107,11 @@
         actual_text = unicode(self.ui.status.text())
         self.assertEqual(qt.FILE_SYNC_STARTING, actual_text)
 
-    def test_process_info_changed(self):
+    def test_status_change_handler_set(self):
         """Backend's file sync status changed callback is connected."""
-        self.ui.refresh_status()
-        self.assertEqual(self.ui.backend.status_changed_handler,
-                         self.ui._process_status)
+        # pylint: disable=W0212
+        self.assertEqual(self.ui.backend._called['add_status_changed_handler'],
+                         ((self.ui._process_status,), {}))
 
     def test_process_info_disabled(self):
         """File sync status disabled update the label."""

=== modified file 'ubuntuone/controlpanel/tests/test_backend.py'
--- ubuntuone/controlpanel/tests/test_backend.py	2012-08-30 00:13:45 +0000
+++ ubuntuone/controlpanel/tests/test_backend.py	2012-09-06 16:37:20 +0000
@@ -1419,8 +1419,8 @@
         self.assertEqual(self.was_disabled, self.be.file_sync_disabled)
 
     def test_status_changed(self):
-        """The file_sync_status is the status changed handler."""
-        self.be.status_changed_handler = self._set_called
+        """The status changed handler is called with a processed status."""
+        self.be.add_status_changed_handler(self._set_called)
         status = {'name': 'foo', 'description': 'bar', 'is_error': '',
                   'is_connected': '', 'is_online': '', 'queues': ''}
         self.be.sd_client.status_changed_handler(status)
@@ -1430,12 +1430,56 @@
 
     def test_invalid_status_type(self):
         """Check that the method return None with invalid types."""
-        self.be.status_changed_handler = self._set_called
+        self.be.add_status_changed_handler(self._set_called)
         status = 3
         self.be.sd_client.status_changed_handler(status)
 
         self.assertEqual(self._called, ((None,), {}))
 
+    def test_add_same_status_handler_twice(self):
+        """Should ignore adding same handler twice."""
+        self.call_count = 0
+
+        def inc_count(status):
+            """Fake Callback."""
+            self.call_count += 1
+
+        self.addCleanup(delattr, self, "call_count")
+
+        self.be.add_status_changed_handler(inc_count)
+        self.be.add_status_changed_handler(inc_count)
+
+        status = {'name': 'foo', 'description': 'bar', 'is_error': '',
+                  'is_connected': '', 'is_online': '', 'queues': ''}
+        self.be.sd_client.status_changed_handler(status)
+
+        self.assertEqual(self.call_count, 1)
+        self.assertEqual(self.be._status_changed_handlers,
+                         [inc_count])
+
+    def test_add_two_status_handlers(self):
+        """Should accept and call multiple status handlers."""
+        self.call_count_a = 0
+        self.call_count_b = 0
+
+        def inc_a(status):
+            """Fake status handler #1"""
+            self.call_count_a += 1
+
+        def inc_b(status):
+            """Fake status handler #2"""
+            self.call_count_b += 1
+
+        self.addCleanup(delattr, self, "call_count_a")
+        self.addCleanup(delattr, self, "call_count_b")
+
+        self.be.add_status_changed_handler(inc_a)
+        self.be.add_status_changed_handler(inc_b)
+
+        self.be.sd_client.status_changed_handler({})
+        self.assertEqual(self.call_count_b, 1)
+        self.assertEqual(self.call_count_a, 1)
+
 
 class BackendSyncStatusIfDisabledTestCase(BackendSyncStatusTestCase):
     """Syncdaemon state for the backend when file sync is disabled."""