Merge lp:~alecu/ubuntuone-client/cleanup-emit-signals into lp:ubuntuone-client

[Alejandro J. Cura]

Connect each IPC client to the signals it should listen to. (LP: #839060)

Connect each IPC client to the signals it should listen to. (LP: #839060)

[Manuel de la Peña (mandel)]

A small comment, sending the entire collection of remote_calls to be signals seems wrong to me. Because remove_calls can be all the calls that can be remotly called from the client. I know that at this point we just have signals.. but who knows, what about using the @signal decorator to create a signals collection? Or at least a subset of all the remote_calls.

review: Needs Fixing

[Natalia Bidart (nataliabidart)]

Looks good to me!

review: Approve

[lp:~alecu/ubuntuone-client/cleanup-emit-signals]

1124. By Alejandro J. Cura on 2011-09-02

separate the list of signal handlers from the list of remote calls.

1125. By Alejandro J. Cura on 2011-09-02

fix some tests

[Manuel de la Peña (mandel)]

Nice!

review: Approve

=== modified file 'tests/platform/windows/test_ipc.py'
--- tests/platform/windows/test_ipc.py	2011-08-26 00:03:30 +0000
+++ tests/platform/windows/test_ipc.py	2011-09-02 15:03:18 +0000
@@ -43,6 +43,7 @@
     IPCInterface,
     IPCRoot,
     PublicFiles,
+    RemoteMeta,
     Shares,
     SignalBroadcaster,
     Status,
@@ -58,9 +59,10 @@
     FoldersClient,
     FileSystemClient,
     PublicFilesClient,
+    RemoteClient,
     StatusClient,
     SyncDaemonClient,
-    SharesClient
+    SharesClient,
 )
 
 TEST_PORT = 40404
@@ -272,6 +274,7 @@
         """Return the public files remote object."""
         return self._public_files
 
+
 class TestSignalBroadcaster(MockerTestCase):
     """Test the signal broadcaster code."""
 
@@ -279,13 +282,15 @@
         super(TestSignalBroadcaster, self).setUp()
         self.client = self.mocker.mock()
         self.broad_caster = SignalBroadcaster()
-        self.broad_caster.clients.append(self.client)
 
     def test_remote_register_to_signals(self):
         """Assert that the client was added."""
         self.mocker.replay()
-        self.broad_caster.remote_register_to_signals(self.client)
-        self.assertTrue(self.client in self.broad_caster.clients)
+        signals = ["demo_signal1", "demo_signal2"]
+        self.broad_caster.remote_register_to_signals(self.client, signals)
+        for signal in signals:
+            clients = self.broad_caster.clients_per_signal[signal]
+            self.assertTrue(self.client in clients)
 
     def test_emit_signal(self):
         """Assert that the client method was called."""
@@ -299,6 +304,8 @@
         deferred.addErrback(ANY, ANY, ANY)
         deferred.addErrback(ANY, ANY, ANY)
         self.mocker.replay()
+        signals = [signal_name]
+        self.broad_caster.remote_register_to_signals(self.client, signals)
         self.broad_caster.emit_signal(signal_name, first, second, word=word)
 
     def test_emit_signal_dead_reference(self):
@@ -310,10 +317,11 @@
         self.mocker.replay()
 
         sb = SignalBroadcaster()
-        sb.remote_register_to_signals(fake_remote_client)
-        self.assertIn(fake_remote_client, sb.clients)
+        sb.remote_register_to_signals(fake_remote_client, [sample_signal])
+        self.assertIn(fake_remote_client, sb.clients_per_signal[sample_signal])
         sb.emit_signal(sample_signal)
-        self.assertNotIn(fake_remote_client, sb.clients)
+        self.assertNotIn(fake_remote_client,
+                         sb.clients_per_signal[sample_signal])
 
     def test_emit_signal_some_dead_some_not(self):
         """Test a clean reference after a dead one."""
@@ -328,8 +336,8 @@
         self.mocker.replay()
 
         sb = SignalBroadcaster()
-        sb.remote_register_to_signals(fake_dead_remote)
-        sb.remote_register_to_signals(fake_alive_remote)
+        sb.remote_register_to_signals(fake_dead_remote, [sample_signal])
+        sb.remote_register_to_signals(fake_alive_remote, [sample_signal])
         sb.emit_signal(sample_signal)
 
 
@@ -360,8 +368,10 @@
         fake_remote_client = FakeRemoteClient()
 
         sb = SignalBroadcaster()
-        sb.remote_register_to_signals(fake_remote_client)
-        self.assertIn(fake_remote_client, sb.clients)
+        signals = [fake_remote_client.missing_signal]
+        sb.remote_register_to_signals(fake_remote_client, signals)
+        sb_clients = sb.clients_per_signal[fake_remote_client.missing_signal]
+        self.assertIn(fake_remote_client, sb_clients)
         sb.emit_signal(fake_remote_client.missing_signal)
 
         expected = (
@@ -379,8 +389,10 @@
         fake_remote_client = FakeRemoteClient()
 
         sb = SignalBroadcaster()
-        sb.remote_register_to_signals(fake_remote_client)
-        self.assertIn(fake_remote_client, sb.clients)
+        signals = [fake_remote_client.failing_signal]
+        sb.remote_register_to_signals(fake_remote_client, signals)
+        sb_clients = sb.clients_per_signal[fake_remote_client.failing_signal]
+        self.assertIn(fake_remote_client, sb_clients)
         sb.emit_signal(fake_remote_client.failing_signal)
 
         expected = (
@@ -392,11 +404,38 @@
         self.assertIn(expected, warnings)
 
 
+class FakeRemoteObject(object):
+    """A test helper."""
+
+    def __init__(self):
+        """Initialize this test helper."""
+        self.called = []
+
+    def callRemote(self, *args):
+        """A remote call to this object."""
+        self.called.append(args)
+
+
+class RemoteClientTestCase(TestCase):
+    """Tests for the RemoteClient class."""
+
+    def test_register_to_signals(self):
+        """Test the register_to_signals method."""
+        fake_remote_object = FakeRemoteObject()
+        client = RemoteClient(fake_remote_object)
+        client.signal_handlers = ["on_abc"]
+        client.register_to_signals()
+        expected = [
+            ("register_to_signals", client, client.signal_handlers)
+        ]
+        self.assertEqual(fake_remote_object.called, expected)
+
+
 class TestStatusEmitSignals(PerspectiveBrokerTestCase, MockerTestCase):
     """Tests that the emit methods are correct."""
 
     def setUp(self):
-        """Setup tests."""   
+        """Setup tests."""
         super(TestStatusEmitSignals, self).setUp()
         self.signal_method = self.mocker.mock()
         self.status.syncdaemon_status = self.mocker.mock()
@@ -885,21 +924,11 @@
         root = yield self.client_factory.getRootObject()
         remote = yield root.callRemote('get_status')
         client = StatusClient(remote)
+        # set the cb
+        callback_names = [signal + "_cb" for signal in client.signal_handlers]
         yield client.register_to_signals()
-        # set the cb
-        for signal in ['on_content_queue_changed_cb',
-                       'on_invalid_name_cb',
-                       'on_broken_node_cb',
-                       'on_status_changed_cb',
-                       'on_download_started_cb',
-                       'on_download_file_progress_cb',
-                       'on_download_finished_cb',
-                       'on_upload_started_cb',
-                       'on_upload_file_progress_cb',
-                       'on_upload_finished_cb',
-                       'on_account_changed_cb',
-                       'on_metaqueue_changed_cb']:
-            setattr(client, signal, self.mocker.mock())
+        for callback_name in callback_names:
+            setattr(client, callback_name, self.mocker.mock())
         defer.returnValue(client)
 
     def test_current_status(self):
@@ -3119,6 +3148,48 @@
         self.sender.handle_default(event_name, **kwargs)
 
 
+class RemoteMetaTestCase(TestCase):
+    """Tests for the RemoteMeta metaclass."""
+
+    def test_remote_calls_renamed(self):
+        """The remote_calls are renamed."""
+        test_token = object()
+
+        class TestClass(object):
+            """A class for testing."""
+
+            __metaclass__ = RemoteMeta
+
+            remote_calls = ['test_method']
+
+            def test_method(self):
+                """Fake call."""
+                return test_token
+
+        tc = TestClass()
+        self.assertEquals(tc.test_method(), test_token)
+        self.assertEquals(tc.remote_test_method(), test_token)
+
+    def test_signal_handlers_renamed(self):
+        """The signal_handlers are renamed."""
+        test_token = object()
+
+        class TestClass(object):
+            """A class for testing."""
+
+            __metaclass__ = RemoteMeta
+
+            signal_handlers = ['test_signal_handler']
+
+            def test_signal_handler(self):
+                """Fake call."""
+                return test_token
+
+        tc = TestClass()
+        self.assertEquals(tc.test_signal_handler(), test_token)
+        self.assertEquals(tc.remote_test_signal_handler(), test_token)
+
+
 class TestIPCRoot(BaseIPCTestCase):
     """Ensure that the IPCRoot works as expected."""
 

=== modified file 'ubuntuone/platform/windows/ipc.py'
--- ubuntuone/platform/windows/ipc.py	2011-08-26 00:03:30 +0000
+++ ubuntuone/platform/windows/ipc.py	2011-09-02 15:03:18 +0000
@@ -20,6 +20,7 @@
 
 import logging
 
+from collections import defaultdict
 from threading import Thread
 
 from twisted.internet import defer, reactor
@@ -139,10 +140,10 @@
     """
 
     def __new__(cls, name, bases, attrs):
-        remote_calls = attrs.get('remote_calls', None)
-        if remote_calls:
-            for current in remote_calls:
-                attrs['remote_' + current] = attrs[current]
+        remote_calls = attrs.get('remote_calls', [])
+        signal_handlers = attrs.get('signal_handlers', [])
+        for current in remote_calls + signal_handlers:
+            attrs['remote_' + current] = attrs[current]
         return super(RemoteMeta, cls).__new__(cls, name, bases, attrs)
 
 
@@ -154,7 +155,7 @@
 
     def __init__(self):
         """Create a new instance."""
-        self.clients = []
+        self.clients_per_signal = defaultdict(set)
 
     def _ignore_no_such_method(self, failure, signal_name, current_client):
         """NoSuchMethod is not an error, ignore it."""
@@ -167,33 +168,29 @@
                     current_client, failure.value)
         logger.warning('Traceback is:\n%s', failure.printDetailedTraceback())
 
-    def remote_register_to_signals(self, client):
-        """Allow a client to register to a signal."""
-        if client not in self.clients:
-            self.clients.append(client)
-        else:
-            logger.warning('Client %s tried to register twice.', client)
+    def remote_register_to_signals(self, client, signals):
+        """Allow a client to register to some signals."""
+        for signal in signals:
+            self.clients_per_signal[signal].add(client)
 
     def remote_unregister_to_signals(self, client):
-        """Allow a client to register to a signal."""
-        if client in self.clients:
-            self.clients.remove(client)
-        else:
-            logger.warning('Tried to remove %s when was not registered.',
-                           client)
+        """Allow a client to unregister from the signal."""
+        for connected_clients in self.clients_per_signal.values():
+            if client in connected_clients:
+                connected_clients.remove(client)
 
     def emit_signal(self, signal_name, *args, **kwargs):
         """Emit the given signal to the clients."""
         logger.debug("emitting %r to all connected clients.", signal_name)
-        dead_clients = []
-        for current_client in self.clients:
+        dead_clients = set()
+        for current_client in self.clients_per_signal[signal_name]:
             try:
                 d = current_client.callRemote(signal_name, *args, **kwargs)
                 d.addErrback(self._ignore_no_such_method, signal_name,
                                                           current_client)
                 d.addErrback(self._other_failure, signal_name, current_client)
             except DeadReferenceError:
-                dead_clients.append(current_client)
+                dead_clients.add(current_client)
         for client in dead_clients:
             self.remote_unregister_to_signals(client)
 
@@ -379,6 +376,7 @@
 
     def __init__(self, root, main, volume_manager, action_queue):
         """ Creates the instance."""
+        super(SyncDaemon, self).__init__()
         self.service = SyncdaemonService(root, main, volume_manager,
             action_queue)
         self.clients = []

=== modified file 'ubuntuone/platform/windows/ipc_client.py'
--- ubuntuone/platform/windows/ipc_client.py	2011-07-15 17:03:13 +0000
+++ ubuntuone/platform/windows/ipc_client.py	2011-09-02 15:03:18 +0000
@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 #
-# Author: Manuel de la Pena<[e-mail]>
+# Authors: Manuel de la Pena <[e-mail]>
+#          Alejandro J. Cura <[e-mail]>
 #
 # Copyright 2011 Canonical Ltd.
 #
@@ -65,13 +66,16 @@
 class RemoteClient(object):
     """Represent a client for remote calls."""
 
+    signal_handlers = []
+
     def __init__(self, remote_object):
         """Create instance."""
         self.remote = remote_object
 
     def register_to_signals(self):
         """Register to the signals."""
-        return self.remote.callRemote('register_to_signals', self)
+        return self.remote.callRemote('register_to_signals', self,
+                                      self.signal_handlers)
 
     def unregister_to_signals(self):
         """Register to the signals."""
@@ -124,18 +128,20 @@
     __metaclass__ = RemoteMeta
 
     # calls that will be accessible remotely
-    remote_calls = ['on_content_queue_changed',
-                    'on_invalid_name',
-                    'on_broken_node',
-                    'on_status_changed',
-                    'on_download_started',
-                    'on_download_file_progress',
-                    'on_download_finished',
-                    'on_upload_started',
-                    'on_upload_file_progress',
-                    'on_upload_finished',
-                    'on_account_changed',
-                    'on_metaqueue_changed']
+    signal_handlers = [
+        'on_content_queue_changed',
+        'on_invalid_name',
+        'on_broken_node',
+        'on_status_changed',
+        'on_download_started',
+        'on_download_file_progress',
+        'on_download_finished',
+        'on_upload_started',
+        'on_upload_file_progress',
+        'on_upload_finished',
+        'on_account_changed',
+        'on_metaqueue_changed',
+    ]
 
     def __init__(self, remote_status):
         """Creates the instance."""
@@ -228,7 +234,7 @@
     __metaclass__ = RemoteMeta
 
     # calls that will be accessible remotely
-    remote_calls = ['on_event',]
+    signal_handlers = ['on_event',]
 
     def __init__(self, remote_events):
         """Creates the instance."""
@@ -249,8 +255,7 @@
     __metaclass__ = RemoteMeta
 
     # calls that will be accessible remotely
-    remote_calls = ['on_root_mismatch',
-                    'on_quota_exceeded']
+    signal_handlers = ['on_root_mismatch', 'on_quota_exceeded']
 
     def __init__(self, remote_daemon):
         """Creates the instance."""
@@ -332,17 +337,19 @@
     __metaclass__ = RemoteMeta
 
     # calls that will be accessible remotely
-    remote_calls = ['on_share_deleted',
-                    'on_share_changed',
-                    'on_share_delete_error',
-                    'on_share_created',
-                    'on_share_create_error',
-                    'on_share_answer_response',
-                    'on_new_share',
-                    'on_share_subscribed',
-                    'on_share_subscribe_error',
-                    'on_share_unsubscribed',
-                    'on_share_unsubscribe_error']
+    signal_handlers = [
+        'on_share_deleted',
+        'on_share_changed',
+        'on_share_delete_error',
+        'on_share_created',
+        'on_share_create_error',
+        'on_share_answer_response',
+        'on_new_share',
+        'on_share_subscribed',
+        'on_share_subscribe_error',
+        'on_share_unsubscribed',
+        'on_share_unsubscribe_error',
+    ]
 
     def __init__(self, remote_shares):
         """Create the instance."""
@@ -556,14 +563,16 @@
     __metaclass__ = RemoteMeta
 
     # calls that will be accessible remotely
-    remote_calls = ['on_folder_created',
-                    'on_folder_create_error',
-                    'on_folder_deleted',
-                    'on_folder_delete_error',
-                    'on_folder_subscribed',
-                    'on_folder_subscribe_error',
-                    'on_folder_unsubscribed',
-                    'on_folder_unsubscribe_error']
+    signal_handlers = [
+        'on_folder_created',
+        'on_folder_create_error',
+        'on_folder_deleted',
+        'on_folder_delete_error',
+        'on_folder_subscribed',
+        'on_folder_subscribe_error',
+        'on_folder_unsubscribed',
+        'on_folder_unsubscribe_error',
+    ]
 
     def __init__(self, remote_folders):
         """Creates the instance."""
@@ -636,10 +645,12 @@
     __metaclass__ = RemoteMeta
 
     # calls that will be accessible remotely
-    remote_calls = ['on_public_access_changed',
-                    'on_public_access_change_error',
-                    'on_public_files_list',
-                    'on_public_files_list_error']
+    signal_handlers = [
+        'on_public_access_changed',
+        'on_public_access_change_error',
+        'on_public_files_list',
+        'on_public_files_list_error',
+    ]
 
     def __init__(self, remote_public_files):
         super(PublicFilesClient, self).__init__(remote_public_files)