Merge lp:~diegosarmentero/ubuntuone-client/ubuntuone-client-syncstate into lp:ubuntuone-client

[Diego Sarmentero]

- Recceive notifications from aggregator when the status of syncdaemon change (LP: #1053631).

[lp:~diegosarmentero/ubuntuone-client/ubuntuone-client-syncstate]

1332. By Diego Sarmentero on 2012-10-10

fixing link

[Roberto Alsina (ralsina)]

Code looks good, running tests takes way too long here :-(

review: Approve

[Manuel de la Peña (mandel)]

Code looks good and tests pass. I have a small idea to make things a little nicer, what about doing the foSyncllowing:

SyncMenuStatus = namedtuple('SyncMenuStatus',
    ['_connected', 'ignore_status_event', 'connect_called', 'disconnect_called'], verbose=False)

def _assert_sync_status(self, expected):
    self.assertEqual(expected.connected, self.sync_menu._connected)
    self.assertEqual(expected.ignore_status_event, self.sync_menu._ignore_status_event)
    self.assertEqual(expected.connected_called, self.sync_menu._syncdaemon_service.connect_called)
    self.assertEqual(expected.disconnected_called, self.sync_menu._syncdaemon_service.disconnect_called)

and the we can have tests like the following (based on a current test):

def test_status_change_from_menu(self):
    """Check the behavior when the status is changed from the menu."""
    self.sync_menu.change_sync_status()
    self._assert_sync_status(SyncMenuStatus(connected=False, ignore_status_event=False,
                                            connect_called=False, disconnect_called=True))

    self.sync_menu._syncdaemon_service.disconnect_called = False
    self.sync_menu.change_sync_status()

    self._assert_sync_status(SyncMenuStatus(connected=True, ignore_status_event=False,
                                            connect_called=True, disconnect_called=True))
    self.assertTrue(self.sync_menu._connected)

instead of:

71 + def test_status_change_from_menu(self):
72 + """Check the behavior when the status is changed from the menu."""
73 + self.sync_menu.change_sync_status()
74 + self.assertFalse(self.sync_menu._connected)
75 + self.assertFalse(self.sync_menu._ignore_status_event)
76 + self.assertFalse(self.sync_menu._syncdaemon_service.connect_called)
77 + self.assertTrue(self.sync_menu._syncdaemon_service.disconnect_called)
78 +
79 + self.sync_menu._syncdaemon_service.disconnect_called = False
80 + self.sync_menu.change_sync_status()
81 + self.assertTrue(self.sync_menu._connected)
82 + self.assertFalse(self.sync_menu._ignore_status_event)
83 + self.assertTrue(self.sync_menu._syncdaemon_service.connect_called)
84 + self.assertFalse(self.sync_menu._syncdaemon_service.disconnect_called)

Of course you can remove the use of the named tuple and just define the assert_method as:

def _assert_sync_status(self, connected=False, ignore_status_event=False,
                              connect_called=False, disconnect_called=False):

Which ever you prefer :) (I felt funny using the namedtuple :P )

review: Approve

=== modified file 'tests/platform/sync_menu/test_linux.py'
--- tests/platform/sync_menu/test_linux.py	2012-10-05 15:14:55 +0000
+++ tests/platform/sync_menu/test_linux.py	2012-10-10 15:00:35 +0000
@@ -73,6 +73,18 @@
 class FakeSyncdaemonService(object):
     """Fake SyncdaemonService."""
 
+    def __init__(self):
+        self.connect_called = False
+        self.disconnect_called = False
+
+    def connect(self):
+        """Set connect to True."""
+        self.connect_called = True
+
+    def disconnect(self):
+        """Set connect to True."""
+        self.disconnect_called = True
+
 
 class FakeSyncMenuApp(object):
     """Fake SyncMenu."""
@@ -95,6 +107,10 @@
         """Fake connect."""
         self.data['connect'] = (signal, callback)
 
+    def set_paused(self, status):
+        """Set the pause state."""
+        self.data['paused'] = status
+
 
 class SyncMenuDummyTestCase(TestCase):
     """Test the SyncMenu."""
@@ -123,18 +139,9 @@
         self.syncdaemon_service = FakeSyncdaemonService()
         self.status_frontend = FakeStatusFrontend()
         self._paused = False
-        self.patch(sync_menu.UbuntuOneSyncMenu, "change_sync_status",
-            self._change_sync_status)
         self.sync_menu = sync_menu.UbuntuOneSyncMenu(self.status_frontend,
             self.syncdaemon_service)
 
-    def _change_sync_status(self, *args):
-        """Fake change_sync_status."""
-        if self._paused:
-            self._paused = False
-        else:
-            self._paused = True
-
     def test_init(self):
         """Check that the menu is properly initialized."""
         self.assertIsInstance(FakeSyncMenuApp.data['server'],
@@ -161,12 +168,6 @@
         self.assertEqual(self.sync_menu.get_help.property_get(
             linux.Dbusmenu.MENUITEM_PROP_LABEL), linux.GET_HELP)
 
-        self.assertEqual(self.sync_menu.app.data['connect'],
-            ("notify::paused", self.sync_menu.change_sync_status))
-        self.sync_menu.app.data['connect'][1]()
-        self.assertTrue(self._paused)
-        self.sync_menu.app.data['connect'][1]()
-        self.assertFalse(self._paused)
         self.sync_menu.transfers.update_progress()
         self.assertIsInstance(self.sync_menu.transfers.separator,
             linux.Dbusmenu.Menuitem)
@@ -386,3 +387,37 @@
         self.sync_menu.next_update = time.time() * 2
         self.sync_menu.update_transfers()
         self.assertEqual(self.sync_menu.timer.delay, 3)
+
+    def test_status_change_from_menu(self):
+        """Check the behavior when the status is changed from the menu."""
+        self.sync_menu.change_sync_status()
+        self.assertFalse(self.sync_menu._connected)
+        self.assertFalse(self.sync_menu._ignore_status_event)
+        self.assertFalse(self.sync_menu._syncdaemon_service.connect_called)
+        self.assertTrue(self.sync_menu._syncdaemon_service.disconnect_called)
+
+        self.sync_menu._syncdaemon_service.disconnect_called = False
+        self.sync_menu.change_sync_status()
+        self.assertTrue(self.sync_menu._connected)
+        self.assertFalse(self.sync_menu._ignore_status_event)
+        self.assertTrue(self.sync_menu._syncdaemon_service.connect_called)
+        self.assertFalse(self.sync_menu._syncdaemon_service.disconnect_called)
+
+    def test_ignore_status(self):
+        """Check that neither connect or disconnect are called."""
+        self.sync_menu._ignore_status_event = True
+        self.assertTrue(self.sync_menu._ignore_status_event)
+        self.sync_menu.change_sync_status()
+        self.assertTrue(self.sync_menu._connected)
+        self.assertFalse(self.sync_menu._ignore_status_event)
+        self.assertFalse(self.sync_menu._syncdaemon_service.connect_called)
+        self.assertFalse(self.sync_menu._syncdaemon_service.disconnect_called)
+
+    def test_sync_status_changed(self):
+        """Check sync_status_changed behavior."""
+        self.sync_menu.sync_status_changed(True)
+        self.assertNotIn('paused', self.sync_menu.app.data)
+        self.sync_menu.sync_status_changed(False)
+        self.assertFalse(self.sync_menu._connected)
+        self.assertTrue(self.sync_menu._ignore_status_event)
+        self.assertTrue(self.sync_menu.app.data['paused'])
 \ No newline at end of file
 
=== modified file 'tests/status/test_aggregator.py'
--- tests/status/test_aggregator.py	2012-10-01 15:37:01 +0000
+++ tests/status/test_aggregator.py	2012-10-10 15:00:35 +0000
@@ -1330,6 +1330,36 @@
             self.aggregator.register_progress_listener, [])
         self.assertEqual(len(self.aggregator.progress_listeners), 0)
 
+    def test_register_connection_listener(self):
+        """Check that register listener handles properly additions."""
+
+        def fake_callback():
+            """Do nothing."""
+
+        self.aggregator.register_connection_listener(fake_callback)
+        self.assertEqual(len(self.aggregator.connection_listeners), 1)
+
+    def test_register_connection_listener_fail(self):
+        """Check that register listener fails with not Callable objects."""
+        self.assertRaises(TypeError,
+            self.aggregator.register_connection_listener, [])
+        self.assertEqual(len(self.aggregator.connection_listeners), 0)
+
+    def test_connection_notifications(self):
+        """Check that the connection lister is notified."""
+        data = {}
+
+        def fake_callback(status):
+            """Register status."""
+            data['status'] = status
+
+        self.aggregator.register_connection_listener(fake_callback)
+        self.assertEqual(data, {})
+        self.aggregator.connection_lost()
+        self.assertFalse(data['status'])
+        self.aggregator.connection_made()
+        self.assertTrue(data['status'])
+
     def assertMiscCommandQueued(self, fc):
         """Assert that some command was queued."""
         self.assertEqual(len(self.aggregator.to_do), 1)
 
=== modified file 'ubuntuone/platform/sync_menu/linux.py'
--- ubuntuone/platform/sync_menu/linux.py	2012-10-09 14:31:11 +0000
+++ ubuntuone/platform/sync_menu/linux.py	2012-10-10 15:00:35 +0000
@@ -82,8 +82,9 @@
     def __init__(self, status, syncdaemon_service):
         """Initialize menu."""
         self._syncdaemon_service = syncdaemon_service
-        self._paused = False
+        self._connected = True
         self.timer = None
+        self._ignore_status_event = False
         self.next_update = time.time()
         self.root_menu = Dbusmenu.Menuitem()
 
@@ -129,14 +130,23 @@
         self.app.set_menu(self.server)
         self.app.connect("notify::paused", self.change_sync_status)
 
+    def sync_status_changed(self, status):
+        """Listen to the changes for the sync status."""
+        if status != self._connected:
+            self._connected = status
+            self._ignore_status_event = True
+            self.app.set_paused(not self._connected)
+
     def change_sync_status(self, *args):
         """Triggered when the sync status is changed fromm the menu."""
-        if self._paused:
+        if self._ignore_status_event:
+            self._ignore_status_event = False
+        elif self._connected:
+            self._syncdaemon_service.disconnect()
+            self._connected = False
+        else:
             self._syncdaemon_service.connect()
-            self._paused = False
-        else:
-            self._syncdaemon_service.disconnect()
-            self._paused = True
+            self._connected = True
 
     def open_control_panel(self, *args):
         """Open the Ubuntu One Control Panel."""
 
=== modified file 'ubuntuone/status/aggregator.py'
--- ubuntuone/status/aggregator.py	2012-10-01 15:37:01 +0000
+++ ubuntuone/status/aggregator.py	2012-10-10 15:00:35 +0000
@@ -616,6 +616,7 @@
         self.progress = {}
         self.to_do = {}
         self.recent_transfers = deque(maxlen=5)
+        self.connection_listeners = []
         self.progress_listeners = []
 
     def get_notification(self):
@@ -654,6 +655,13 @@
         else:
             raise TypeError("Callable object expected.")
 
+    def register_connection_listener(self, listener):
+        """Register a callable object to be notified."""
+        if isinstance(listener, Callable):
+            self.connection_listeners.append(listener)
+        else:
+            raise TypeError("Callable object expected.")
+
     def get_discovery_message(self):
         """Get the text for the discovery bubble."""
         lines = []
@@ -793,10 +801,14 @@
     def connection_lost(self):
         """The connection to the server was lost."""
         self.file_discovery_bubble.connection_lost()
+        for callback in self.connection_listeners:
+            callback(False)
 
     def connection_made(self):
         """The connection to the server was made."""
         self.file_discovery_bubble.connection_made()
+        for callback in self.connection_listeners:
+            callback(True)
 
 
 class StatusFrontend(object):
@@ -818,6 +830,8 @@
         if self.syncdaemon_service is not None:
             self.sync_menu = sync_menu.UbuntuOneSyncMenu(self,
                 self.syncdaemon_service)
+            self.aggregator.register_connection_listener(
+                self.sync_menu.sync_status_changed)
             self.aggregator.register_progress_listener(
                 self.sync_menu.update_transfers)