Ubuntu One Control Panel

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

socket-communication
Merge into trunk

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://qastaging/~diegosarmentero/ubuntuone-control-panel/socket-communication

Merge into:

lp://qastaging/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://qastaging/~diegosarmentero/ubuntuone-control-panel/socket-communication

Undecided

Fix Released

Link a bug report

Reviewer	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.qastaging.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.

lp://qastaging/~diegosarmentero/ubuntuone-control-panel/socket-communication updated on 2012-10-31

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

Revision history for this message

dobey (dobey) wrote on 2012-10-31:

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://qastaging/~diegosarmentero/ubuntuone-control-panel/socket-communication updated on 2012-11-01

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

Revision history for this message

dobey (dobey) wrote on 2012-11-01:

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

lp://qastaging/~diegosarmentero/ubuntuone-control-panel/socket-communication updated on 2012-11-01

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

Revision history for this message

dobey (dobey) wrote on 2012-11-01:

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://qastaging/~diegosarmentero/ubuntuone-control-panel/socket-communication updated on 2012-11-02

390. By Diego Sarmentero on 2012-11-02: adding comment to explain protocol

Revision history for this message

Mike McCracken (mikemc) wrote on 2012-11-02:

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://qastaging/~diegosarmentero/ubuntuone-control-panel/socket-communication updated on 2012-11-05

391. By Diego Sarmentero on 2012-11-05: Making sure that the server is cleaned in case of fail

Revision history for this message

Diego Sarmentero (diegosarmentero) wrote on 2012-11-05:

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