Merge lp:~diegosarmentero/ubuntuone-control-panel/socket-communication into lp:ubuntuone-control-panel

[Diego Sarmentero]

- Adding socket communication, so the new instances can send messages to the already running instance (LP: #1063927).

Currently this only supports the "switch to tab" option, because it's the only one that is being used, and it didn't make any sense to me to send the other args to the already running application, although it can be easily extended.

You can test this IRL, running an instance of UbuntuOne Client (the one which adds the "Share a File" option will be the best option to test it) adding this branch to the PYTHONPATH of that instance, and choose the "Share a file" option from the menu and see how control panel behaves.

[lp:~diegosarmentero/ubuntuone-control-panel/socket-communication]

378. By Diego Sarmentero on 2012-10-31

fixing docstring

379. By Diego Sarmentero on 2012-10-31

executes show normal for not arg cases

[dobey (dobey)]

This is still not working for me (the tab does switch, but the window doesn't get focus, or raised from minimized state). Also, the latest change seems to result in the unique app support being broken, and I get multiple control panel instances with it.

review: Needs Fixing

[lp:~diegosarmentero/ubuntuone-control-panel/socket-communication]

380. By Diego Sarmentero on 2012-11-01

always raise

381. By Diego Sarmentero on 2012-11-01

improve tests

382. By Diego Sarmentero on 2012-11-01

fixing activatewindow

383. By Diego Sarmentero on 2012-11-01

adding showNormal

384. By Diego Sarmentero on 2012-11-01

removing commented line

385. By Diego Sarmentero on 2012-11-01

removed unused code

[dobey (dobey)]

Just updated with the latest revno of this, and it's still opening 2 instances of the control panel for me.

[lp:~diegosarmentero/ubuntuone-control-panel/socket-communication]

386. By Diego Sarmentero on 2012-11-01

adding try-except

387. By Diego Sarmentero on 2012-11-01

force raise

388. By Diego Sarmentero on 2012-11-01

show on minimized

389. By Diego Sarmentero on 2012-11-01

improve tests redeability

[dobey (dobey)]

I still don't like that it doesn't guarantee that focus will be grabbed, or that the window will be raised, but this does seem to be a problem with Qt itself.

review: Approve

[lp:~diegosarmentero/ubuntuone-control-panel/socket-communication]

390. By Diego Sarmentero on 2012-11-02

adding comment to explain protocol

[Mike McCracken (mikemc)]

The code is a little fragile wrt crashes. (This branch hasn't made it any worse, but now's a good time to fix this, since it's short and related.)

If CP crashes and doesn't get to call removeServer, then the socket file is left around and future servers can't be started with the same name, so from then on, every invocation of CP will fail to connect to a server, think it's the unique instance, and start up happily.

I'd like to suggest a couple of improvements:
1. check self.ready, the response from server.listen. If this is false, log server.errorString(). In the current code, we assume server.listen always works and having this log would've made it a lot easier to find the problem I was seeing.

2. call self.cleanup() (or just server.removeServer) before server.listen. In the event that another CP has crashed, leaving around a socket file, this will clean it up so server.listen should always succeed like we want.

This should be ok to call unconditionally, because if we get to this line, we've failed to connect to any server on that socket, and we really should be the unique instance.

NOTE: windows doesn't use socket files, but Qt says that removeServer does nothing on windows, so at least this shouldn't break anything.

With these changes, I am seeing no problems on osx even when I cause a CP crash and verify that there's a zombie socket file before I try re-starting CP.

review: Needs Fixing

[lp:~diegosarmentero/ubuntuone-control-panel/socket-communication]

391. By Diego Sarmentero on 2012-11-05

Making sure that the server is cleaned in case of fail

[Diego Sarmentero (diegosarmentero)]

> The code is a little fragile wrt crashes. (This branch hasn't made it any
> worse, but now's a good time to fix this, since it's short and related.)
>
> If CP crashes and doesn't get to call removeServer, then the socket file is
> left around and future servers can't be started with the same name, so from
> then on, every invocation of CP will fail to connect to a server, think it's
> the unique instance, and start up happily.
>
> I'd like to suggest a couple of improvements:
> 1. check self.ready, the response from server.listen. If this is false, log
> server.errorString(). In the current code, we assume server.listen always
> works and having this log would've made it a lot easier to find the problem I
> was seeing.
>
> 2. call self.cleanup() (or just server.removeServer) before server.listen. In
> the event that another CP has crashed, leaving around a socket file, this will
> clean it up so server.listen should always succeed like we want.
>
> This should be ok to call unconditionally, because if we get to this line,
> we've failed to connect to any server on that socket, and we really should be
> the unique instance.
>
> NOTE: windows doesn't use socket files, but Qt says that removeServer does
> nothing on windows, so at least this shouldn't break anything.
>
> With these changes, I am seeing no problems on osx even when I cause a CP
> crash and verify that there's a zombie socket file before I try re-starting
> CP.

Done!

[lp:~diegosarmentero/ubuntuone-control-panel/socket-communication]

392. By Diego Sarmentero on 2012-11-05

adding more tests

393. By Diego Sarmentero on 2012-11-05

removing unused import

[Mike McCracken (mikemc)]

looks good, tests pass on darwin. thanks!

review: Approve

=== modified file 'ubuntuone/controlpanel/gui/qt/gui.py'
--- ubuntuone/controlpanel/gui/qt/gui.py	2012-08-20 17:51:54 +0000
+++ ubuntuone/controlpanel/gui/qt/gui.py	2012-11-05 17:30:44 +0000
@@ -61,6 +61,21 @@
         else:
             self.entry = None
 
+    def connect_app_signal(self, app):
+        """Connect App signals."""""
+        app.switch_to.connect(self.switch_to)
+        app.activate_window.connect(self.raise_to_top)
+
+    def raise_to_top(self):
+        """Force the application to raise in top of any other app."""
+        self.activateWindow()
+        self.raise_()
+        if self.isMinimized():
+            self.setWindowFlags(QtCore.Qt.WindowStaysOnTopHint)
+            self.showNormal()
+            self.setWindowFlags(QtCore.Qt.Window)
+            self.showNormal()
+
     def _setup(self):
         """Do some extra setupping for the UI."""
         self.ui.control_panel.finished.connect(self.close)
@@ -128,6 +143,7 @@
         window.check_updates()
         window.show()
         window.raise_()
+        window.connect_app_signal(app)
     else:
         window = None
     if with_icon or minimized:
 
=== modified file 'ubuntuone/controlpanel/gui/qt/main/tests/test_main.py'
--- ubuntuone/controlpanel/gui/qt/main/tests/test_main.py	2012-10-04 20:47:39 +0000
+++ ubuntuone/controlpanel/gui/qt/main/tests/test_main.py	2012-11-05 17:30:44 +0000
@@ -274,7 +274,7 @@
         """Ensure the new_instance signal is connected."""
         main.main([sys.argv[0]])
         self.assertEqual(self.app.new_instance.target,
-            self.start.window.raise_)
+            [self.start.window.raise_])
 
     def test_darwin_installs_qt4reactor(self):
         """Ensure the qt4 reactor is installed when requested."""
 
=== modified file 'ubuntuone/controlpanel/gui/qt/tests/test_start.py'
--- ubuntuone/controlpanel/gui/qt/tests/test_start.py	2012-06-28 05:32:24 +0000
+++ ubuntuone/controlpanel/gui/qt/tests/test_start.py	2012-11-05 17:30:44 +0000
@@ -36,6 +36,7 @@
     def __init__(self):
         self.args = []
         self.updates_checked = defer.Deferred()
+        self.app = None
 
     def __call__(self, *args, **kwargs):
         self.args.append((args, kwargs))
@@ -57,6 +58,10 @@
         self.updates_checked.callback(None)
         return self.updates_checked
 
+    def connect_app_signal(self, app):
+        """Fake connect_app_signal."""
+        self.app = app
+
 
 class StartTestCase(TestCase):
     """Test the qt control panel."""
@@ -124,3 +129,9 @@
         # a timeout in this test means that the check_updates method
         # was not called
         yield self.main_window.updates_checked
+
+    def test_connect_app_signals_is_called(self):
+        """Check that connect_app_signal is properly called on start."""
+        gui.start(close_callback=self.close_cb)
+        app = gui.QtGui.QApplication.instance()
+        self.assertEqual(self.main_window.app, app)
 
=== modified file 'ubuntuone/controlpanel/gui/qt/uniqueapp/__init__.py'
--- ubuntuone/controlpanel/gui/qt/uniqueapp/__init__.py	2012-04-24 17:59:49 +0000
+++ ubuntuone/controlpanel/gui/qt/uniqueapp/__init__.py	2012-11-05 17:30:44 +0000
@@ -20,15 +20,31 @@
 
 from PyQt4 import QtNetwork, QtGui, QtCore
 
+from ubuntuone.controlpanel.logger import setup_logging
+
+
+logger = setup_logging('uniqueapp')
+
+# Arg with valid value
+SOCKET_MESSAGES = {
+    '--switch-to': ["folders", "share_links", "devices",
+                    "settings", "account"],
+}
+
 
 class UniqueApplication(QtGui.QApplication):
 
     """A dummy UniqueApplication class."""
 
     new_instance = QtCore.pyqtSignal()
+    switch_to = QtCore.pyqtSignal(unicode)
+    activate_window = QtCore.pyqtSignal()
 
     def __init__(self, argv, key):
         super(UniqueApplication, self).__init__(argv)
+        self.mapping_signals = {
+            '--switch-to': self.switch_to,
+        }
         self.key = key
         self.server = QtNetwork.QLocalServer(self)
         self.server.newConnection.connect(self.new_instance.emit)
@@ -38,11 +54,51 @@
         socket.connectToServer(key, QtCore.QIODevice.WriteOnly)
         if socket.waitForConnected(500):
             # Connected, exit
+            self._send_messages(socket, argv)
             sys.exit()
 
         # Not connected, start server
+        self.cleanup()
         self.ready = self.server.listen(key)
+        if not self.ready:
+            logger.debug(self.server.errorString())
+        self.server.newConnection.connect(self._process_messages)
 
     def cleanup(self):
         """Remove the socket when we die."""
         self.server.removeServer(self.key)
+
+    def _send_messages(self, socket, argv):
+        """Send messages to the running application."""
+        # This only take care of those args that are defined in SOCKET_MESSAGES
+        # at this moment just "--switch-to", sending a message to the already
+        # running client with: "arg=arg_value". If we have more than a single
+        # arg, each arg is concatenated with "|".
+        try:
+            data = []
+            size_argv = len(argv)
+            for index in range(2, size_argv):
+                if (argv[index] in SOCKET_MESSAGES and index < size_argv and
+                   argv[index + 1] in SOCKET_MESSAGES[argv[index]]):
+                    data.append('%s=%s' % (argv[index], argv[index + 1]))
+            message = '|'.join(data)
+            socket.write(message)
+            socket.flush()
+            socket.close()
+        except:
+            # The message couldn't be send through the socket,
+            # but we avoided to open multiple instances of control panel.
+            pass
+
+    def _process_messages(self):
+        """Get the messages from the other instances."""
+        connection = self.server.nextPendingConnection()
+        connection.waitForReadyRead()
+        data = unicode(connection.readAll()).split('|')
+        connection.close()
+        for item in data:
+            item_value = item.split('=')
+            signal = self.mapping_signals.get(item_value[0])
+            if signal:
+                signal.emit(item_value[1])
+        self.activate_window.emit()
 
=== modified file 'ubuntuone/controlpanel/gui/qt/uniqueapp/tests/test_unique_app.py'
--- ubuntuone/controlpanel/gui/qt/uniqueapp/tests/test_unique_app.py	2012-04-24 17:59:49 +0000
+++ ubuntuone/controlpanel/gui/qt/uniqueapp/tests/test_unique_app.py	2012-11-05 17:30:44 +0000
@@ -19,9 +19,9 @@
 from PyQt4 import QtCore
 from twisted.internet.defer import inlineCallbacks
 
+from ubuntuone.controlpanel.gui.tests import FakeSignal
 from ubuntuone.controlpanel.gui.qt import uniqueapp
 from ubuntuone.controlpanel.tests import TestCase
-from ubuntuone.controlpanel.gui.tests import FakeSignal
 
 
 #pylint: disable=C0103
@@ -32,6 +32,7 @@
         self.connect_calls = []
         self.connect_timeouts = []
         self.connect_succeeds = True
+        self.message = None
 
     def connectToServer(self, *args, **kwargs):
         """Fake connectToServer."""
@@ -42,18 +43,51 @@
         self.connect_timeouts.append(timeout)
         return self.connect_succeeds
 
+    def write(self, message):
+        """Fake write."""
+        self.message = message
+
+    def flush(self):
+        """Fake flush."""
+
+    def close(self):
+        """Fake close."""
+
+    def waitForReadyRead(self):
+        """Fake waitForReadyRead."""
+
+    def readAll(self):
+        """Fake readAll: return the message."""
+        return self.message
+
 
 class FakeLocalServer(object):
 
     """A fake QLocalServer."""
 
-    def __init__(self):
+    def __init__(self, connected=True):
         self.newConnection = FakeSignal()
         self.listen_args = []
+        self.socket = None
+        self._removed_key = None
+        self._is_connected = connected
 
     def listen(self, *args, **kwargs):
         """Fake listen."""
         self.listen_args.append((args, kwargs))
+        return self._is_connected
+
+    def nextPendingConnection(self):
+        """Fake nextPendingConnection."""
+        return self.socket
+
+    def removeServer(self, key):
+        """Fake removeServer."""
+        self._removed_key = key
+
+    def errorString(self):
+        """Fake errorString."""
+        return 'error'
 
 
 class FakeApplication(object):
@@ -77,6 +111,21 @@
         self.patch(uniqueapp.UniqueApplication, "aboutToQuit", self.fake_quit)
         self.patch(uniqueapp.QtGui, "QApplication", FakeApplication)
 
+    def test_cleanup_called_on_init(self):
+        """Check that cleanup is called on initialization."""
+        uniapp = uniqueapp.UniqueApplication([], "key")
+        self.assertEqual("key", uniapp.server._removed_key)
+
+    def test_on_failed_connection(self):
+        """Check the flow of the program on connection fail."""
+        data = []
+        local_server = FakeLocalServer(False)
+        self.patch(uniqueapp.QtNetwork, "QLocalServer",
+            lambda parent: local_server)
+        self.patch(uniqueapp.logger, "debug", data.append)
+        uniqueapp.UniqueApplication([], "key")
+        self.assertEqual(data, ['error'])
+
     def test_client_socket(self):
         """Check that the client socket is used correctly."""
         self.local_socket.connect_succeeds = True
@@ -105,10 +154,56 @@
         app = uniqueapp.UniqueApplication([], "key")
         # Yes, this is ugly. I can't find any other meaningful
         # way to compare them though.
-        self.assertEqual(str(app.server.newConnection.target.__self__),
+        self.assertEqual(str(app.server.newConnection.target[0].__self__),
             str(app.new_instance))
+        self.assertEqual(app.server.newConnection.target[1],
+            app._process_messages)
 
     def test_cleanup(self):
         """Check that cleanup is called with the right key."""
         app = uniqueapp.UniqueApplication([], "key")
-        self.assertEqual(self.fake_quit.target, app.cleanup)
+        self.assertEqual(self.fake_quit.target, [app.cleanup])
+
+    def test_send_messages_valid(self):
+        """Check the message is created correctly."""
+        self.local_socket.connect_succeeds = True
+        argv = ['python', 'ubuntuone-control-panel-qt',
+            '--switch-to', 'share_links']
+        uniqueapp.UniqueApplication(argv, "key")
+        expected = "--switch-to=share_links"
+        self.assertEqual(self.local_socket.message, expected)
+
+    def test_send_messages_invalid(self):
+        """Check the message is created correctly."""
+        self.local_socket.connect_succeeds = True
+        argv = ['python', 'ubuntuone-control-panel-qt']
+        uniqueapp.UniqueApplication(argv, "key")
+        expected = ""
+        self.assertEqual(self.local_socket.message, expected)
+
+    def test_process_message_with_message(self):
+        """Check that we are able to parse the message received."""
+        data = []
+        self.local_socket.connect_succeeds = True
+        argv = ['python', 'ubuntuone-control-panel-qt',
+            '--switch-to', 'share_links']
+        app = uniqueapp.UniqueApplication(argv, "key")
+        self.local_server.socket = self.local_socket
+        app.switch_to.connect(data.append)
+        app.activate_window.connect(lambda: data.append(True))
+
+        app.server.newConnection.emit()
+        self.assertEqual(data, ["share_links", True])
+
+    def test_process_message_no_message(self):
+        """Check that we are able to parse the message received."""
+        data = []
+        self.local_socket.connect_succeeds = True
+        argv = ['python', 'ubuntuone-control-panel-qt']
+        app = uniqueapp.UniqueApplication(argv, "key")
+        self.local_server.socket = self.local_socket
+        app.switch_to.connect(data.append)
+        app.activate_window.connect(lambda: data.append(True))
+
+        app.server.newConnection.emit()
+        self.assertEqual(data, [True])
 
=== modified file 'ubuntuone/controlpanel/gui/tests/__init__.py'
--- ubuntuone/controlpanel/gui/tests/__init__.py	2012-04-24 17:59:49 +0000
+++ ubuntuone/controlpanel/gui/tests/__init__.py	2012-11-05 17:30:44 +0000
@@ -185,17 +185,17 @@
 
     def __init__(self, *args, **kwargs):
         """Initialize."""
-        self.target = None
+        self.target = []
 
     def connect(self, target):
         """Fake connect."""
-        self.target = target
+        self.target.append(target)
 
     def disconnect(self, *args):
         """Fake disconnect."""
-        self.target = None
+        self.target = []
 
     def emit(self, *args):
         """Fake emit."""
-        if self.target:
-            self.target(*args)
+        for target in self.target:
+            target(*args)