zeitgeist team mailing list archive

[Siegfried Gevatter <rainct@xxxxxxxxxx>]

Review: Needs Fixing

Hey,

Good job, thanks for working on this.

Some comments:

> (this.network != null) { ... }
What's this supposed to do? You've already unwatched everything in the "else { ... }", so you can just return.

> this.network.setup ();
I'm not too happy about this. Hm. Let's leave it like this, but at least add a comment to the abstract setup() document that it will call on_network_up/on_network_up with the initial state of the network.

> public interface NetworkMonitor: Object
Shouldn't this be private? It's not supposed to be used anywhere outside the extension.

> Write connectivity to the DB.
Not sure what this comment is supposed to mean.

> private void name_appeared_handler(DBusConnection connection, string name, string name_owner)
You're missing a space between "handler (".

> Checks whether there is a funtioning
Typo (funtioning -> functioning). I'd reword it to something like "Monitor the availability of working network connections using Network Manager (0.8 or later)".

> // ^^ There is a bug in some Connman versions causing it to not emit the
This comment is about the proxy.state_changed.connect, but the position where it's places make it look like it's proxy.get_state.
-- 
https://code.launchpad.net/~cando/zeitgeist/storage-monitor/+merge/84299
Your team Zeitgeist Framework Team is subscribed to branch lp:zeitgeist.