Bug 38978

Summary: NetworkManager / ConnMan support
Product: Telepathy Reporter: Guillaume Desmottes <guillaume.desmottes>
Component: mission-controlAssignee: Will Thompson <will>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: enhancement    
Priority: medium CC: olivier.crete
Version: unspecifiedKeywords: patch
Hardware: Other   
OS: All   
URL: http://cgit.collabora.com/git/user/wjt/telepathy-mission-control-wjt.git/log/?h=fd.o-38978-connectivity
Whiteboard: r+
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 28370, 40130, 40131, 41148    
Attachments: generic example of how an mcd transport plugin is formed

Description Guillaume Desmottes 2011-07-05 06:33:35 UTC
At the moment Empathy is the one doing connectivity checking. This should be done by MC so other clients (KDE, gnome-shell, etc) would work properly as well.

We should re-use EmpathyConnectivity as much as possible
http://git.gnome.org/browse/empathy/tree/libempathy/empathy-connectivity.c

How should we implement this? As a connectivity plugin I guess?

Empathy has a gsetting key to disable connectivity checking, do we want that as well?

Also, should we provide a way for applications to know the connectivity status? Some empathy components are using EmpathyConnectivity:
- EmpathyPresenceChooser: unsensitive if network is offline
- EmpathyPresenceManager: implement the reconnecting policy. Should move to MC.
- EmpathyAccountsDialog: to tell user accounts are offline because network is down. Could be replaced by a TP_CONNECTION_STATUS_REASON_NETWORK_OFFLINE returned by tp_account_get_connection_status()
Comment 1 Will Thompson 2011-07-05 06:50:46 UTC
MC has a GInterface for network states, McdTransport.

On the Nokia N900, there's an implementation of this for Nokia's proprietary ConIc service (which is analogous to NM and ConnMan). Sadly, there are no open source implementations of the interface, but the code to make use of the connectivity information is mainly in MC itself (which of course is Free). So Empathy's code could potentially be transplanted into an McdTransport implementation within the MC tree.
Comment 2 Olivier Crête 2011-07-05 08:01:29 UTC
Looks good, it will use a bit more CPU, but I guess it's not dramatic. And we have to remember to remove it if/when we move the UI to something like clutter (it's cheaper to have the CPU do the flippage)
Comment 3 Olivier Crête 2011-07-05 08:03:35 UTC
arg, wrong bug.. 
What I mean to say here is: don't forget my pet bug: Empathy goes back online even if it was disconnected by connecting somewhere else and then NM goes online->offline->online..
Comment 4 Vivek Dasmohapatra 2011-07-06 11:25:20 UTC
Created attachment 48825 [details]
generic example of how an mcd transport plugin is formed
Comment 5 Guillaume Desmottes 2011-08-15 07:01:55 UTC
Assigning to Will as he volunteer to fix during the BoF so he can't step back now. :p
Comment 6 Will Thompson 2011-09-22 07:19:15 UTC
Here are some preliminary uncontroversial tweaks to MC's tests and build system: http://cgit.collabora.com/git/user/wjt/telepathy-mission-control-wjt.git/log/?h=uncontroversial

I have functioning ConnMan and Network Manager support, and functioning tests for it, except that one of my test changes seems to expose a race in a completely unrelated test.
Comment 7 Simon McVittie 2011-09-22 07:27:21 UTC
uncontroversial branch is uncontroversial. ++
Comment 8 Will Thompson 2011-09-23 03:23:19 UTC
It is done.

There are still verrrrry occasional bugs in the test suite, due to the tests taking a match-all match rule plus the issue I keep hitting where libdbus doesn't check the sender of method replies. The symptom I hit a few times is that one head of the test suite calls RequestName; another head replies to a ConnMan connectivity query with "offline"; and libdbus misconstrues the "offline" reply as a reply from the bus daemon to the call to RequestName! This is unfixable without removing the match-all rule and figuring out how to avoid races and keep things coherent in the tests with >1 bus connection, which I can't bring myself to do right now.
Comment 9 Will Thompson 2011-09-23 03:25:32 UTC
A word on the design of the branch: I chose to keep the NM/ConnMan-tracking code separate from the McdTransportPlugin implementation, with a view to making it easier, in the future, to move to a better internal API for this stuff without having to make invasive changes to the actual tracking code. McdKludgeTransport is basically boilerplate glue between McdConnectivityMonitor and the guts of MC.
Comment 10 Will Thompson 2011-09-23 03:29:10 UTC
(In reply to comment #7)
> uncontroversial branch is uncontroversial. ++

Merged it, ta.
Comment 11 Guillaume Desmottes 2011-09-23 05:01:08 UTC
Looks great; you owned with those tests!


+ * Copyright (C) 2009 Collabora Ltd.
should be updated

Would be good to open a nm-glib bug report as in a perfect world MC shouldn't do blocking calls.

McdKludgeTransport: why "kludge"?


+    GList *one;
I'd add a comment explaining what's this; it's not that clear to me.


  return "i love the internet";
rly? :)
Comment 12 Guillaume Desmottes 2011-09-23 05:13:25 UTC
Did you test it with a NM enabled Empathy? It's a bit late to remove this code for 3.2 (release on Monday...).
Comment 13 Will Thompson 2011-09-23 08:50:58 UTC
(In reply to comment #11)
> Looks great; you owned with those tests!

I aim to please.

> + * Copyright (C) 2009 Collabora Ltd.
> should be updated

Done. I'll squash this back. I updated the .h but forgot to update the .c.

> Would be good to open a nm-glib bug report as in a perfect world MC shouldn't
> do blocking calls.

I guess the counter-argument will be that one blocking call during initialization is not bad enough to warrant async-ifying the entire API. This is an unusual case. I actually tried making the fake NM and ConnMan services run in a separate process from the test case, which would (I think) be better in any case, but that fails due to heinous crimes committed by the MC test suite.

> McdKludgeTransport: why "kludge"?

For want of a better name, really.

> +    GList *one;
> I'd add a comment explaining what's this; it's not that clear to me.

I have done so—I've renamed the variable too.

>   return "i love the internet";
> rly? :)

nyanyanyanyanyanyanyanyanyanyan
Comment 14 Guillaume Desmottes 2011-09-26 00:42:49 UTC
(In reply to comment #13)
> > + * Copyright (C) 2009 Collabora Ltd.
> > should be updated
> 
> Done. I'll squash this back. I updated the .h but forgot to update the .c.

yeah fixup the fixup. :)


Looks good to me but you mentionned some issues when not using
ConnectAutomatically?
Comment 15 Will Thompson 2011-09-27 10:26:45 UTC
(In reply to comment #14)
> (In reply to comment #13)
> > > + * Copyright (C) 2009 Collabora Ltd.
> > > should be updated
> > 
> > Done. I'll squash this back. I updated the .h but forgot to update the .c.
> 
> yeah fixup the fixup. :)

I'll fix it up before merging; I just wanted to make it clear what I'd changed.

> Looks good to me but you mentionned some issues when not using
> ConnectAutomatically?

Yes: accounts didn't actually get reconnected if ¬ConnectAutomatically. So I've pushed a pile more patches that make that part work.

I've tested it with a version of Empathy that listens to Network Manager, and they seem to work fine together. It also works if Empathy is not running.
Comment 16 Guillaume Desmottes 2011-09-28 01:30:23 UTC
Looks pretty good. Thanks for these very nicely separated and clear commits, there were a joy to review. :)
Comment 17 Will Thompson 2011-09-28 03:34:30 UTC
Merged! This will be in 5.9.3. :D

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.