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

Proposed by Diego Sarmentero on 2012-10-31
Status: Merged
Approved by: dobey on 2012-11-05
Approved revision: 393
Merged at revision: 378
Proposed branch: lp:~diegosarmentero/ubuntuone-control-panel/socket-communication
Merge into: lp:ubuntuone-control-panel
Diff against target: 351 lines (+188/-10)
6 files modified
ubuntuone/controlpanel/gui/qt/gui.py (+16/-0)
ubuntuone/controlpanel/gui/qt/main/tests/test_main.py (+1/-1)
ubuntuone/controlpanel/gui/qt/tests/test_start.py (+11/-0)
ubuntuone/controlpanel/gui/qt/uniqueapp/__init__.py (+56/-0)
ubuntuone/controlpanel/gui/qt/uniqueapp/tests/test_unique_app.py (+99/-4)
ubuntuone/controlpanel/gui/tests/__init__.py (+5/-5)
To merge this branch: bzr merge lp:~diegosarmentero/ubuntuone-control-panel/socket-communication
Reviewer Review Type Date Requested Status
Mike McCracken (community) Approve on 2012-11-05
dobey (community) 2012-10-31 Approve on 2012-11-01
Review via email: mp+132409@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
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) wrote :

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
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) wrote :

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

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) wrote :

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
390. By Diego Sarmentero on 2012-11-02

adding comment to explain protocol

Mike McCracken (mikemc) wrote :

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
391. By Diego Sarmentero on 2012-11-05

Making sure that the server is cleaned in case of fail

Diego Sarmentero (diegosarmentero) wrote :

> 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!

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) wrote :

looks good, tests pass on darwin. thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ubuntuone/controlpanel/gui/qt/gui.py'
2--- ubuntuone/controlpanel/gui/qt/gui.py 2012-08-20 17:51:54 +0000
3+++ ubuntuone/controlpanel/gui/qt/gui.py 2012-11-05 17:30:44 +0000
4@@ -61,6 +61,21 @@
5 else:
6 self.entry = None
7
8+ def connect_app_signal(self, app):
9+ """Connect App signals."""""
10+ app.switch_to.connect(self.switch_to)
11+ app.activate_window.connect(self.raise_to_top)
12+
13+ def raise_to_top(self):
14+ """Force the application to raise in top of any other app."""
15+ self.activateWindow()
16+ self.raise_()
17+ if self.isMinimized():
18+ self.setWindowFlags(QtCore.Qt.WindowStaysOnTopHint)
19+ self.showNormal()
20+ self.setWindowFlags(QtCore.Qt.Window)
21+ self.showNormal()
22+
23 def _setup(self):
24 """Do some extra setupping for the UI."""
25 self.ui.control_panel.finished.connect(self.close)
26@@ -128,6 +143,7 @@
27 window.check_updates()
28 window.show()
29 window.raise_()
30+ window.connect_app_signal(app)
31 else:
32 window = None
33 if with_icon or minimized:
34
35=== modified file 'ubuntuone/controlpanel/gui/qt/main/tests/test_main.py'
36--- ubuntuone/controlpanel/gui/qt/main/tests/test_main.py 2012-10-04 20:47:39 +0000
37+++ ubuntuone/controlpanel/gui/qt/main/tests/test_main.py 2012-11-05 17:30:44 +0000
38@@ -274,7 +274,7 @@
39 """Ensure the new_instance signal is connected."""
40 main.main([sys.argv[0]])
41 self.assertEqual(self.app.new_instance.target,
42- self.start.window.raise_)
43+ [self.start.window.raise_])
44
45 def test_darwin_installs_qt4reactor(self):
46 """Ensure the qt4 reactor is installed when requested."""
47
48=== modified file 'ubuntuone/controlpanel/gui/qt/tests/test_start.py'
49--- ubuntuone/controlpanel/gui/qt/tests/test_start.py 2012-06-28 05:32:24 +0000
50+++ ubuntuone/controlpanel/gui/qt/tests/test_start.py 2012-11-05 17:30:44 +0000
51@@ -36,6 +36,7 @@
52 def __init__(self):
53 self.args = []
54 self.updates_checked = defer.Deferred()
55+ self.app = None
56
57 def __call__(self, *args, **kwargs):
58 self.args.append((args, kwargs))
59@@ -57,6 +58,10 @@
60 self.updates_checked.callback(None)
61 return self.updates_checked
62
63+ def connect_app_signal(self, app):
64+ """Fake connect_app_signal."""
65+ self.app = app
66+
67
68 class StartTestCase(TestCase):
69 """Test the qt control panel."""
70@@ -124,3 +129,9 @@
71 # a timeout in this test means that the check_updates method
72 # was not called
73 yield self.main_window.updates_checked
74+
75+ def test_connect_app_signals_is_called(self):
76+ """Check that connect_app_signal is properly called on start."""
77+ gui.start(close_callback=self.close_cb)
78+ app = gui.QtGui.QApplication.instance()
79+ self.assertEqual(self.main_window.app, app)
80
81=== modified file 'ubuntuone/controlpanel/gui/qt/uniqueapp/__init__.py'
82--- ubuntuone/controlpanel/gui/qt/uniqueapp/__init__.py 2012-04-24 17:59:49 +0000
83+++ ubuntuone/controlpanel/gui/qt/uniqueapp/__init__.py 2012-11-05 17:30:44 +0000
84@@ -20,15 +20,31 @@
85
86 from PyQt4 import QtNetwork, QtGui, QtCore
87
88+from ubuntuone.controlpanel.logger import setup_logging
89+
90+
91+logger = setup_logging('uniqueapp')
92+
93+# Arg with valid value
94+SOCKET_MESSAGES = {
95+ '--switch-to': ["folders", "share_links", "devices",
96+ "settings", "account"],
97+}
98+
99
100 class UniqueApplication(QtGui.QApplication):
101
102 """A dummy UniqueApplication class."""
103
104 new_instance = QtCore.pyqtSignal()
105+ switch_to = QtCore.pyqtSignal(unicode)
106+ activate_window = QtCore.pyqtSignal()
107
108 def __init__(self, argv, key):
109 super(UniqueApplication, self).__init__(argv)
110+ self.mapping_signals = {
111+ '--switch-to': self.switch_to,
112+ }
113 self.key = key
114 self.server = QtNetwork.QLocalServer(self)
115 self.server.newConnection.connect(self.new_instance.emit)
116@@ -38,11 +54,51 @@
117 socket.connectToServer(key, QtCore.QIODevice.WriteOnly)
118 if socket.waitForConnected(500):
119 # Connected, exit
120+ self._send_messages(socket, argv)
121 sys.exit()
122
123 # Not connected, start server
124+ self.cleanup()
125 self.ready = self.server.listen(key)
126+ if not self.ready:
127+ logger.debug(self.server.errorString())
128+ self.server.newConnection.connect(self._process_messages)
129
130 def cleanup(self):
131 """Remove the socket when we die."""
132 self.server.removeServer(self.key)
133+
134+ def _send_messages(self, socket, argv):
135+ """Send messages to the running application."""
136+ # This only take care of those args that are defined in SOCKET_MESSAGES
137+ # at this moment just "--switch-to", sending a message to the already
138+ # running client with: "arg=arg_value". If we have more than a single
139+ # arg, each arg is concatenated with "|".
140+ try:
141+ data = []
142+ size_argv = len(argv)
143+ for index in range(2, size_argv):
144+ if (argv[index] in SOCKET_MESSAGES and index < size_argv and
145+ argv[index + 1] in SOCKET_MESSAGES[argv[index]]):
146+ data.append('%s=%s' % (argv[index], argv[index + 1]))
147+ message = '|'.join(data)
148+ socket.write(message)
149+ socket.flush()
150+ socket.close()
151+ except:
152+ # The message couldn't be send through the socket,
153+ # but we avoided to open multiple instances of control panel.
154+ pass
155+
156+ def _process_messages(self):
157+ """Get the messages from the other instances."""
158+ connection = self.server.nextPendingConnection()
159+ connection.waitForReadyRead()
160+ data = unicode(connection.readAll()).split('|')
161+ connection.close()
162+ for item in data:
163+ item_value = item.split('=')
164+ signal = self.mapping_signals.get(item_value[0])
165+ if signal:
166+ signal.emit(item_value[1])
167+ self.activate_window.emit()
168
169=== modified file 'ubuntuone/controlpanel/gui/qt/uniqueapp/tests/test_unique_app.py'
170--- ubuntuone/controlpanel/gui/qt/uniqueapp/tests/test_unique_app.py 2012-04-24 17:59:49 +0000
171+++ ubuntuone/controlpanel/gui/qt/uniqueapp/tests/test_unique_app.py 2012-11-05 17:30:44 +0000
172@@ -19,9 +19,9 @@
173 from PyQt4 import QtCore
174 from twisted.internet.defer import inlineCallbacks
175
176+from ubuntuone.controlpanel.gui.tests import FakeSignal
177 from ubuntuone.controlpanel.gui.qt import uniqueapp
178 from ubuntuone.controlpanel.tests import TestCase
179-from ubuntuone.controlpanel.gui.tests import FakeSignal
180
181
182 #pylint: disable=C0103
183@@ -32,6 +32,7 @@
184 self.connect_calls = []
185 self.connect_timeouts = []
186 self.connect_succeeds = True
187+ self.message = None
188
189 def connectToServer(self, *args, **kwargs):
190 """Fake connectToServer."""
191@@ -42,18 +43,51 @@
192 self.connect_timeouts.append(timeout)
193 return self.connect_succeeds
194
195+ def write(self, message):
196+ """Fake write."""
197+ self.message = message
198+
199+ def flush(self):
200+ """Fake flush."""
201+
202+ def close(self):
203+ """Fake close."""
204+
205+ def waitForReadyRead(self):
206+ """Fake waitForReadyRead."""
207+
208+ def readAll(self):
209+ """Fake readAll: return the message."""
210+ return self.message
211+
212
213 class FakeLocalServer(object):
214
215 """A fake QLocalServer."""
216
217- def __init__(self):
218+ def __init__(self, connected=True):
219 self.newConnection = FakeSignal()
220 self.listen_args = []
221+ self.socket = None
222+ self._removed_key = None
223+ self._is_connected = connected
224
225 def listen(self, *args, **kwargs):
226 """Fake listen."""
227 self.listen_args.append((args, kwargs))
228+ return self._is_connected
229+
230+ def nextPendingConnection(self):
231+ """Fake nextPendingConnection."""
232+ return self.socket
233+
234+ def removeServer(self, key):
235+ """Fake removeServer."""
236+ self._removed_key = key
237+
238+ def errorString(self):
239+ """Fake errorString."""
240+ return 'error'
241
242
243 class FakeApplication(object):
244@@ -77,6 +111,21 @@
245 self.patch(uniqueapp.UniqueApplication, "aboutToQuit", self.fake_quit)
246 self.patch(uniqueapp.QtGui, "QApplication", FakeApplication)
247
248+ def test_cleanup_called_on_init(self):
249+ """Check that cleanup is called on initialization."""
250+ uniapp = uniqueapp.UniqueApplication([], "key")
251+ self.assertEqual("key", uniapp.server._removed_key)
252+
253+ def test_on_failed_connection(self):
254+ """Check the flow of the program on connection fail."""
255+ data = []
256+ local_server = FakeLocalServer(False)
257+ self.patch(uniqueapp.QtNetwork, "QLocalServer",
258+ lambda parent: local_server)
259+ self.patch(uniqueapp.logger, "debug", data.append)
260+ uniqueapp.UniqueApplication([], "key")
261+ self.assertEqual(data, ['error'])
262+
263 def test_client_socket(self):
264 """Check that the client socket is used correctly."""
265 self.local_socket.connect_succeeds = True
266@@ -105,10 +154,56 @@
267 app = uniqueapp.UniqueApplication([], "key")
268 # Yes, this is ugly. I can't find any other meaningful
269 # way to compare them though.
270- self.assertEqual(str(app.server.newConnection.target.__self__),
271+ self.assertEqual(str(app.server.newConnection.target[0].__self__),
272 str(app.new_instance))
273+ self.assertEqual(app.server.newConnection.target[1],
274+ app._process_messages)
275
276 def test_cleanup(self):
277 """Check that cleanup is called with the right key."""
278 app = uniqueapp.UniqueApplication([], "key")
279- self.assertEqual(self.fake_quit.target, app.cleanup)
280+ self.assertEqual(self.fake_quit.target, [app.cleanup])
281+
282+ def test_send_messages_valid(self):
283+ """Check the message is created correctly."""
284+ self.local_socket.connect_succeeds = True
285+ argv = ['python', 'ubuntuone-control-panel-qt',
286+ '--switch-to', 'share_links']
287+ uniqueapp.UniqueApplication(argv, "key")
288+ expected = "--switch-to=share_links"
289+ self.assertEqual(self.local_socket.message, expected)
290+
291+ def test_send_messages_invalid(self):
292+ """Check the message is created correctly."""
293+ self.local_socket.connect_succeeds = True
294+ argv = ['python', 'ubuntuone-control-panel-qt']
295+ uniqueapp.UniqueApplication(argv, "key")
296+ expected = ""
297+ self.assertEqual(self.local_socket.message, expected)
298+
299+ def test_process_message_with_message(self):
300+ """Check that we are able to parse the message received."""
301+ data = []
302+ self.local_socket.connect_succeeds = True
303+ argv = ['python', 'ubuntuone-control-panel-qt',
304+ '--switch-to', 'share_links']
305+ app = uniqueapp.UniqueApplication(argv, "key")
306+ self.local_server.socket = self.local_socket
307+ app.switch_to.connect(data.append)
308+ app.activate_window.connect(lambda: data.append(True))
309+
310+ app.server.newConnection.emit()
311+ self.assertEqual(data, ["share_links", True])
312+
313+ def test_process_message_no_message(self):
314+ """Check that we are able to parse the message received."""
315+ data = []
316+ self.local_socket.connect_succeeds = True
317+ argv = ['python', 'ubuntuone-control-panel-qt']
318+ app = uniqueapp.UniqueApplication(argv, "key")
319+ self.local_server.socket = self.local_socket
320+ app.switch_to.connect(data.append)
321+ app.activate_window.connect(lambda: data.append(True))
322+
323+ app.server.newConnection.emit()
324+ self.assertEqual(data, [True])
325
326=== modified file 'ubuntuone/controlpanel/gui/tests/__init__.py'
327--- ubuntuone/controlpanel/gui/tests/__init__.py 2012-04-24 17:59:49 +0000
328+++ ubuntuone/controlpanel/gui/tests/__init__.py 2012-11-05 17:30:44 +0000
329@@ -185,17 +185,17 @@
330
331 def __init__(self, *args, **kwargs):
332 """Initialize."""
333- self.target = None
334+ self.target = []
335
336 def connect(self, target):
337 """Fake connect."""
338- self.target = target
339+ self.target.append(target)
340
341 def disconnect(self, *args):
342 """Fake disconnect."""
343- self.target = None
344+ self.target = []
345
346 def emit(self, *args):
347 """Fake emit."""
348- if self.target:
349- self.target(*args)
350+ for target in self.target:
351+ target(*args)

Subscribers

People subscribed via source and target branches