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

[Diego Sarmentero]

- Making the handler connection to be a list of handlers (LP: #1061880).

[Mike McCracken (mikemc)]

This is related to this control-panel branch:
https://code.launchpad.net/~diegosarmentero/ubuntuone-control-panel/u1-cp-publishapi/+merge/128316

but in light of that branch, it's not totally clear why we need to change this here.

share_links is the only part of control panel that ever sets signal handlers for the public_files handler and the public_access_changed handler. Is it possible to have multiple invocations of this remote call from control panel? ie, does this happen if we click fast to share two files in the table or something? or is that impossible?

Also, just a note that on IRC I requested a test that shows the problem and breaks with the old code… Diego was working on that at EOD friday…

review: Needs Information

[Diego Sarmentero (diegosarmentero)]

We needed to store a list of handlers to be called, because the setattr is overriding the behaviour of that part of the object every time a connect_signal is requested, and it was happening that several functions were being connected to the same signals, the ones with the handler that we actually want, and some other for clean up at syncdaemon core, and only the last handler connected was the one that was being called.

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

1335. By Diego Sarmentero on 2012-10-08

merge

1336. By Diego Sarmentero on 2012-10-09

improving handler calls

[Manuel de la Peña (mandel)]

73 + self.addCleanup(self.sdtool.shutdown)

Why not adding it after the creation of the sdtool?

review: Needs Fixing

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

1337. By Diego Sarmentero on 2012-10-09

improve test

1338. By Diego Sarmentero on 2012-10-09

fix test

1339. By Diego Sarmentero on 2012-10-09

adding new test

[Mike McCracken (mikemc)]

minor - test_connect_avoid_repeated_connection has the same docstring as test_connect_several_handlers_to_same_signal but they test different things

review: Needs Fixing

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

1340. By Diego Sarmentero on 2012-10-09

fixed docstring

[Mike McCracken (mikemc)]

Approve. These new tests pass on OSX*, and testing the share links panel shows no further hangs.

* the whole suite still has a ton of dirty reactor errors though

review: Approve

[Manuel de la Peña (mandel)]

review: Approve

=== modified file 'tests/platform/tools/test_tools.py'
--- tests/platform/tools/test_tools.py	2012-08-27 15:22:13 +0000
+++ tests/platform/tools/test_tools.py	2012-10-09 15:37:25 +0000
@@ -139,11 +139,11 @@
     def method_call(self, *args, **kwargs):
         """Fake a remote method call."""
         if self.number_calls == 0:
-           self.number_calls += 1
-           raise pb.DeadReferenceError()
+            self.number_calls += 1
+            raise pb.DeadReferenceError()
         else:
-           self.called.append((args, kwargs))
-           return defer.succeed(self.number_calls)
+            self.called.append((args, kwargs))
+            return defer.succeed(self.number_calls)
 
 
 class PerspectiveBrokerReconnect(TestCase):
@@ -184,8 +184,100 @@
     @defer.inlineCallbacks
     def test_reconnect_signals(self):
         """Test reconnection with signals."""
-        self.sdtool.connected_signals = dict(create_signal=lambda:None,
-                                             delete_signal=lambda:None)
+        self.sdtool.connected_signals = dict(create_signal=[lambda: None],
+                                             delete_signal=[lambda: None])
         yield self.sdtool.call_method('fake_remote', 'method_call')
         self.assertTrue(self.reconnected)
         self.assertEqual(2, len(self.connected_signals))
+
+
+class FakeU1Objects(object):
+    """Fake PublicFiles for UbuntuOneClient."""
+
+    def on_public_files_list_cb(self):
+        """Do nothing."""
+
+    def on_share_changed_cb(self):
+        """Do nothing."""
+
+
+class FakeUbuntuOneClient(object):
+    """Fake UbuntuOneClient."""
+
+    def __init__(self):
+        self.public_files = FakeU1Objects()
+        self.shares = FakeU1Objects()
+        self.connected = True
+
+    def is_connected(self):
+        """Fake is_connected."""
+        return self.connected
+
+    def connect(self):
+        """Fake connect."""
+        self.connected = True
+        yield self.connected
+
+    def disconnect(self):
+        """Fake disconnect."""
+        self.connected = False
+
+
+class PerspectiveBrokerConnectSignal(TestCase):
+    """Test when the ipc is reconnected."""
+
+    @defer.inlineCallbacks
+    def setUp(self):
+        """Set the tests."""
+        yield super(PerspectiveBrokerConnectSignal, self).setUp()
+        self.patch(perspective_broker, 'UbuntuOneClient', FakeUbuntuOneClient)
+        self.sdtool = perspective_broker.SyncDaemonToolProxy()
+        self.addCleanup(self.sdtool.shutdown)
+
+    def test_connect_several_handlers_to_same_signal(self):
+        """Connect more than one handler to the same signal name."""
+        data = []
+
+        signal_name = "PublicFilesList"
+        self.sdtool.connect_signal(signal_name, lambda *a: data.append(a))
+        self.sdtool.connect_signal(signal_name, lambda *a: data.append(a))
+
+        self.assertEqual(len(self.sdtool.connected_signals[signal_name]), 2)
+        self.sdtool.client.public_files.on_public_files_list_cb()
+        expected = [(), ()]
+        self.assertEqual(data, expected)
+
+    def test_connect_avoid_repeated_connection(self):
+        """Ensure that we don't have the same handler called twice."""
+        data = []
+
+        signal_name = "PublicFilesList"
+        func = lambda *a: data.append(a)
+        self.sdtool.connect_signal(signal_name, func)
+        self.sdtool.connect_signal(signal_name, func)
+
+        self.assertEqual(len(self.sdtool.connected_signals[signal_name]), 1)
+        self.sdtool.client.public_files.on_public_files_list_cb()
+        expected = [()]
+        self.assertEqual(data, expected)
+
+    def test_proper_connections(self):
+        """Check that the proper handlers are called."""
+        data = []
+        data2 = []
+
+        signal_name = "PublicFilesList"
+        signal_name2 = "ShareChanges"
+        self.sdtool.connect_signal(signal_name, lambda *a: data.append(a))
+        self.sdtool.connect_signal(signal_name2, lambda *a: data2.append(a))
+
+        self.assertEqual(len(self.sdtool.connected_signals[signal_name]), 1)
+        self.assertEqual(len(self.sdtool.connected_signals[signal_name2]), 1)
+        self.sdtool.client.public_files.on_public_files_list_cb()
+        expected = [()]
+        self.assertEqual(data, expected)
+        self.assertEqual(data2, [])
+        data = []
+        self.sdtool.client.shares.on_share_changed_cb()
+        self.assertEqual(data, [])
+        self.assertEqual(data2, expected)
\ No newline at end of file

=== modified file 'ubuntuone/platform/tools/perspective_broker.py'
--- ubuntuone/platform/tools/perspective_broker.py	2012-08-27 15:49:13 +0000
+++ ubuntuone/platform/tools/perspective_broker.py	2012-10-09 15:37:25 +0000
@@ -30,6 +30,7 @@
 
 import logging
 import subprocess
+from collections import defaultdict
 
 from twisted.internet import defer
 from twisted.spread.pb import DeadReferenceError
@@ -122,7 +123,7 @@
             'perspective_broker')
         self.client = UbuntuOneClient()
         self.connected = None
-        self.connected_signals = {}
+        self.connected_signals = defaultdict(set)
 
     def _call_after_connection(self, method):
         """Make sure Perspective Broker is connected before calling."""
@@ -150,8 +151,9 @@
         self.connected = False
         yield self.client.reconnect()
         # do connect all the signals again
-        for signal_name, handler in self.connected_signals.items():
-            self.connect_signal(signal_name, handler)
+        for signal_name, handlers in self.connected_signals.items():
+            for handler in handlers:
+                self.connect_signal(signal_name, handler)
 
     @defer.inlineCallbacks
     def call_method(self, client_kind, method_name, *args, **kwargs):
@@ -173,13 +175,21 @@
         """Close connections."""
         return self.client.disconnect()
 
+    def _handler(self, signal_name, *args, **kwargs):
+        """Call all the handlers connected to signal_name."""
+        for cb_handler in self.connected_signals[signal_name]:
+            cb_handler(*args, **kwargs)
+
     def connect_signal(self, signal_name, handler):
         """Connect 'handler' with 'signal_name'."""
         client_kind, callback = self._SIGNAL_MAPPING[signal_name]
         client = getattr(self.client, client_kind)
-        setattr(client, callback, handler)
+        if len(self.connected_signals[signal_name]) == 0:
+            setattr(client, callback,
+                lambda *args, **kwargs:
+                    self._handler(signal_name, *args, **kwargs))
         # do remember the connected signal in case we need to reconnect
-        self.connected_signals[signal_name] = handler
+        self.connected_signals[signal_name].add(handler)
         return handler
 
     def disconnect_signal(self, signal_name, handler_or_match):