Déjà Dup

Merge lp:~andrewfister/deja-dup/network-manager into lp:~deja-dup-team/deja-dup/trunk

network-manager
Merge into trunk

Proposed by Michael Terry on 2009-09-03

Status:

Merged

Merged at revision:

452

Proposed branch:

lp:~andrewfister/deja-dup/network-manager

Merge into:

lp:~deja-dup-team/deja-dup/trunk

Diff against target:

None lines

To merge this branch:

bzr merge lp:~andrewfister/deja-dup/network-manager

Related bugs:

Bug #306978: Don't network-backup if no network	Medium	Fix Released
Bug #397720: Deja Dup needs to wait for any network-up on login	Medium	Triaged
Bug #438389: Pause network backup when network-manager is down	Undecided	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Michael Terry			Needs Fixing on 2009-09-05
Review via email: mp+11123@code.launchpad.net

Revision history for this message

Michael Terry (mterry) wrote on 2009-09-04:

Download full text (3.7 KiB)

So first off, thank you so much for the patch. :) I really
appreciate the work. It's very good! I haven't gotten to test it yet
(very busy week), but I've read through it. Here are some comments
(mostly me nitpicking :)).

* credit

Eh, in AUTHORS just put your name in the "Files: *" section near the
top. The other sections are largely special-cases for imported code.
And for the about page, I'd put your name first, since it sorts first.

* translations

You seem to have changed a lot of files in help/translations/ and po/.
Probably by accident.

* whitespace

There are several lines where you just changed whitespace. Not the
worst thing ever, but makes the diff harder to read and is probably
unintentional (admittedly the code is not 100% consistent on spacing).

* Actual changes :)

+ //Detect a remote backend such as Amazon S3, and delay the start of backup.
+ try {
+ var path = DejaDup.BackendFile.get_location_from_gconf();
+ var file = File.parse_name(path);
+ native_path = file.is_native();
+ }

BackendFile is a specific backend. It may not be the backend the user
is using. You should use Backend.get_default() and
Backend.is_native(). The return value for that can change over the
lifetime of the monitor, so it shouldn't be checked once at the start
and never changed. Rather, when the monitor wants to kickoff, it
should check the current backend/connection state.

- string? get_location_from_gconf() throws Error
+ public static string? get_location_from_gconf() throws Error

Hopefully not needed if the above is fixed.

+ catch (GLib.Error e) {
+ printerr("%s\n\n%s", e.message, "GLib Error!\n");
+ return 1;
+ }

Use warning(), like elsewhere in the code: warning("%s\n", e.message);

Don't early exit (return 1) if possible. The monitor should always
stay running.

+ try {
+ init_dbus_to_network_manager();
+ }
+ catch (Error e) {
+ printerr("%s\n\n%s", e.message, "Failed to initialize DBus\n");
+ return 1;
+ }

NetworkManager not being available shouldn't be a fatal error. If we
can't ask NM what the connection status is, just continue as if we
were connected (i.e. fallback to the naive, non-NM-aware case).

+ if (!native_path && !connected) {
+ debug("No connection found. Postponing the backup.");
+ connection_postponed = true;
+ return false;
+ }

I don't think you need connection_postponed. I'd say just call
prepare_tomorrow() (as insurance that the dbus check didn't
malfunction), and wait for network_manager_state_changed() to kick in
in the mean time.

+static void network_manager_state_changed(DBus.Object obj, uint32 new_state)
+{
+ connected = new_state == NM_STATE_CONNECTED;
+
+ if (connected) {
+ long wait_time;
+ if (!seconds_until_next_run(out wait_time))
+ return;
+
+ if (connection_postponed) {
+ prepare_run(0);
+ connection_postponed = false;
+ }
+ }
+
+}

Could probably just be "if (connected) prepare_next_run();"

* Follow-up work
With the above comments addressed, I would commit. However, there are
some related things that should be done before the next release if you
additionally feel up to them.

1) I don't like...

So first off, thank you so much for the patch.  :)  I really
appreciate the work.  It's very good!  I haven't gotten to test it yet
(very busy week), but I've read through it.  Here are some comments
(mostly me nitpicking :)).

* credit

Eh, in AUTHORS just put your name in the "Files: *" section near the
top.  The other sections are largely special-cases for imported code.
And for the about page, I'd put your name first, since it sorts first.

* translations

You seem to have changed a lot of files in help/translations/ and po/.
 Probably by accident.

* whitespace

There are several lines where you just changed whitespace.  Not the
worst thing ever, but makes the diff harder to read and is probably
unintentional (admittedly the code is not 100% consistent on spacing).

* Actual changes  :)

+  //Detect a remote backend such as Amazon S3, and delay the start of backup.
+  try {
+    var path = DejaDup.BackendFile.get_location_from_gconf();
+    var file = File.parse_name(path);
+    native_path = file.is_native();
+  }

BackendFile is a specific backend.  It may not be the backend the user
is using.  You should use Backend.get_default() and
Backend.is_native().  The return value for that can change over the
lifetime of the monitor, so it shouldn't be checked once at the start
and never changed.  Rather, when the monitor wants to kickoff, it
should check the current backend/connection state.

-  string? get_location_from_gconf() throws Error
+  public static string? get_location_from_gconf() throws Error

Hopefully not needed if the above is fixed.

+  catch (GLib.Error e) {
+    printerr("%s\n\n%s", e.message, "GLib Error!\n");
+    return 1;
+  }

Use warning(), like elsewhere in the code: warning("%s\n", e.message);

Don't early exit (return 1) if possible.  The monitor should always
stay running.

+    try {
+      init_dbus_to_network_manager();
+    }
+    catch (Error e) {
+      printerr("%s\n\n%s", e.message, "Failed to initialize DBus\n");
+      return 1;
+    }

NetworkManager not being available shouldn't be a fatal error.  If we
can't ask NM what the connection status is, just continue as if we
were connected (i.e. fallback to the naive, non-NM-aware case).

+  if (!native_path && !connected) {
+      debug("No connection found. Postponing the backup.");
+      connection_postponed = true;
+      return false;
+  }

I don't think you need connection_postponed.  I'd say just call
prepare_tomorrow() (as insurance that the dbus check didn't
malfunction), and wait for network_manager_state_changed() to kick in
in the mean time.

+static void network_manager_state_changed(DBus.Object obj, uint32 new_state)
+{
+  connected = new_state == NM_STATE_CONNECTED;
+
+  if (connected) {
+    long wait_time;
+    if (!seconds_until_next_run(out wait_time))
+      return;
+
+    if (connection_postponed) {
+      prepare_run(0);
+      connection_postponed = false;
+    }
+  }
+
+}

Could probably just be "if (connected) prepare_next_run();"

* Follow-up work
With the above comments addressed, I would commit.  However, there are
some related things that should be done before the next release if you
additionally feel up to them.

1) I don't like adding the gigantic dependency of libdeja-dup to the
monitor.  The monitor is supposed to be a very light executable
because it always runs in the user's session -- I don't want to take
up much memory.  libdeja-dup will add too many library dependencies.
But, some functions from libdeja-dup are legitimately needed here
(namely, getting the default backend).  So it should be split into
something more modular.

2) Not backing up if it is a 3G network (expensive).  Possibly with a
dialog asking user if it's OK...  Not sure how the UI interaction
should go.

Revision history for this message

Michael Terry (mterry) wrote on 2009-09-05:

Download full text (3.7 KiB)

* credit

* translations

You seem to have changed a lot of files in help/translations/ and po/.
Probably by accident.

* whitespace

* Actual changes :)

- string? get_location_from_gconf() throws Error
+ public static string? get_location_from_gconf() throws Error

Hopefully not needed if the above is fixed.

+ catch (GLib.Error e) {
+ printerr("%s\n\n%s", e.message, "GLib Error!\n");
+ return 1;
+ }

Use warning(), like elsewhere in the code: warning("%s\n", e.message);

Don't early exit (return 1) if possible. The monitor should always
stay running.

+ try {
+ init_dbus_to_network_manager();
+ }
+ catch (Error e) {
+ printerr("%s\n\n%s", e.message, "Failed to initialize DBus\n");
+ return 1;
+ }

NetworkManager not being available shouldn't be a fatal error. If we
can't ask NM what the connection status is, just continue as if we
were connected (i.e. fallback to the naive, non-NM-aware case).

+ if (!native_path && !connected) {
+ debug("No connection found. Postponing the backup.");
+ connection_postponed = true;
+ return false;
+ }

Could probably just be "if (connected) prepare_next_run();"

* Follow-up work
With the above comments addressed, I would commit. However, there are
some related things that should be done before the next release if you
additionally feel up to them.

1) I don't like...

hi Mike,
   Thanks for the feedback. I think I've fixed everything except the
whitespace and the translations parts which are the least consequential, I
think.

var native_path = true;
  try {
    var client = GConf.Client.get_default();
    var backend_name = client.get_string(DejaDup.BACKEND_KEY);

What do you think of this?

-Andrew

On Fri, Sep 4, 2009 at 7:00 PM, Michael Terry
<michael.terry@canonical.com>wrote:

> Review: Needs Fixing
> So first off, thank you so much for the patch.  :)  I really
> appreciate the work.  It's very good!  I haven't gotten to test it yet
> (very busy week), but I've read through it.  Here are some comments
> (mostly me nitpicking :)).
>
> * credit
>
> Eh, in AUTHORS just put your name in the "Files: *" section near the
> top.  The other sections are largely special-cases for imported code.
> And for the about page, I'd put your name first, since it sorts first.
>
> * translations
>
> You seem to have changed a lot of files in help/translations/ and po/.
>  Probably by accident.
>
> * whitespace
>
> There are several lines where you just changed whitespace.  Not the
> worst thing ever, but makes the diff harder to read and is probably
> unintentional (admittedly the code is not 100% consistent on spacing).
>
> * Actual changes  :)
>
> +  //Detect a remote backend such as Amazon S3, and delay the start of
> backup.
> +  try {
> +    var path = DejaDup.BackendFile.get_location_from_gconf();
> +    var file = File.parse_name(path);
> +    native_path = file.is_native();
> +  }
>
> BackendFile is a specific backend.  It may not be the backend the user
> is using.  You should use Backend.get_default() and
> Backend.is_native().  The return value for that can change over the
> lifetime of the monitor, so it shouldn't be checked once at the start
> and never changed.  Rather, when the monitor wants to kickoff, it
> should check the current backend/connection state.
>
> -  string? get_location_from_gconf() throws Error
> +  public static string? get_location_from_gconf() throws Error
>
> Hopefully not needed if the above is fixed.
>
> +  catch (GLib.Error e) {
> +    printerr("%s\n\n%s", e.message, "GLib Error!\n");
> +    return 1;
> +  }
>
> Use warning(), like elsewhere in the code: warning("%s\n", e.message);
>
> Don't early exit (return 1) if possible.  The monitor should always
> stay running.
>
> +    try {
> +      init_dbus_to_network_manager();
> +    }
> +    catch (Error e) {
> +      printerr("%s\n\n%s", e.message, "Failed to initialize DBus\n");
> +      return 1;
> +    }
>
> NetworkManager not being available shouldn't be a fatal error.  If we
> can't ask NM what the connection status is, just continue as if we
> were connected (i.e. fallback to the naive, non-NM-aware case).
>
> +  if (!native_path && !connected) {
> +      debug("No connection found. Postponing the backup.");
> +      connection_postponed = true;
> +      return false;
> +  }
>
> I don't think you need connection_postponed.  I'd say just call
> prepare_tomorrow() (as insurance that the dbus check didn't
> malfunction), and wait for network_manager_state_changed() to kick in
> in the mean time.
>
> +static void network_manager_state_changed(DBus.Object obj, uint32
> new_state)
> +{
> +  connected = new_state == NM_STATE_CONNECTED;
> +
> +  if (connected) {
> +    long wait_time;
> +    if (!seconds_until_next_run(out wait_time))
> +      return;
> +
> +    if (connection_postponed) {
> +      prepare_run(0);
> +      connection_postponed = false;
> +    }
> +  }
> +
> +}
>
> Could probably just be "if (connected) prepare_next_run();"
>
> * Follow-up work
> With the above comments addressed, I would commit.  However, there are
> some related things that should be done before the next release if you
> additionally feel up to them.
>
> 1) I don't like adding the gigantic dependency of libdeja-dup to the
> monitor.  The monitor is supposed to be a very light executable
> because it always runs in the user's session -- I don't want to take
> up much memory.  libdeja-dup will add too many library dependencies.
> But, some functions from libdeja-dup are legitimately needed here
> (namely, getting the default backend).  So it should be split into
> something more modular.
>
> 2) Not backing up if it is a 3G network (expensive).  Possibly with a
> dialog asking user if it's OK...  Not sure how the UI interaction
> should go.
> --
>
> https://code.edge.launchpad.net/~temposs/deja-dup/network-manager/+merge/11123<https://code.edge.launchpad.net/%7Etemposs/deja-dup/network-manager/+merge/11123>
> You are the owner of lp:~temposs/deja-dup/network-manager.
>

Revision history for this message

Michael Terry (mterry) wrote on 2009-09-07:

> var native_path = true;
> try {
> var client = GConf.Client.get_default();
> var backend_name = client.get_string(DejaDup.BACKEND_KEY);
>
> if (backend_name == "s3")
> native_path = false;
> else if (backend_name == "file")
> {
> var path = DejaDup.BackendFile.get_location_from_gconf();
> var backend_file = File.parse_name(path);
> native_path = backend_file.is_native();
> }
> }
>
> What do you think of this?

Note that while get_default() does take a Window argument, it is "Gtk.Window? win" -- the question mark means it's optional. You can just pass "null".

But truthfully, though I don't like the duplication of code, it's probably worth it here if it means dropping the libdeja-dup dependency. Backends don't get added everyday. We can either separate out parts of libdeja-dup later or just deal with the duplication. This would let us merely add dbus as a dependency to the monitor.

You can leave the translation changes in, just as long as I can drop them when I merge (i.e. I don't have to pick out meaningful changes from non-meaningful). I don't think you made any changes to user strings. And the whitespace stuff is fine too.

Revision history for this message

Andrew Fister (andrewfister) wrote on 2009-09-07:

Prompted by this, I've modified Monitor so that libdeja-dup is not required
anymore, and I've removed it from the Makefile.am :-)

-Andrew

On Sun, Sep 6, 2009 at 10:26 PM, Michael Terry
<email address hidden>wrote:

> > var native_path = true;
> > try {
> > var client = GConf.Client.get_default();
> > var backend_name = client.get_string(DejaDup.BACKEND_KEY);
> >
> > if (backend_name == "s3")
> > native_path = false;
> > else if (backend_name == "file")
> > {
> > var path = DejaDup.BackendFile.get_location_from_gconf();
> > var backend_file = File.parse_name(path);
> > native_path = backend_file.is_native();
> > }
> > }
> >
> > What do you think of this?
>
> Note that while get_default() does take a Window argument, it is
> "Gtk.Window? win" -- the question mark means it's optional. You can just
> pass "null".
>
> But truthfully, though I don't like the duplication of code, it's probably
> worth it here if it means dropping the libdeja-dup dependency. Backends
> don't get added everyday. We can either separate out parts of libdeja-dup
> later or just deal with the duplication. This would let us merely add dbus
> as a dependency to the monitor.
>
> You can leave the translation changes in, just as long as I can drop them
> when I merge (i.e. I don't have to pick out meaningful changes from
> non-meaningful). I don't think you made any changes to user strings. And
> the whitespace stuff is fine too.
> --
>
> https://code.edge.launchpad.net/~temposs/deja-dup/network-manager/+merge/11123<https://code.edge.launchpad.net/%7Etemposs/deja-dup/network-manager/+merge/11123>
> You are the owner of lp:~temposs/deja-dup/network-manager.
>

Revision history for this message

Andrew Fister (andrewfister) wrote on 2009-09-07:

The code is ready for another looking over :-)

-Andrew

On Sun, Sep 6, 2009 at 10:26 PM, Michael Terry
<email address hidden>wrote:

lp:~andrewfister/deja-dup/network-manager updated on 2009-09-07

452. By Michael Terry on 2009-09-07: merge Andrew's network-manager branch: automatic backups won't start if no network is connected

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

The diff has been truncated for viewing.

Subscribers

People subscribed via source and target branches

to all changes:

Andrew Fister

Déjà Dup Maintainers

Yoan Blanc

Déjà Dup

Merge lp:~andrewfister/deja-dup/network-manager into lp:~deja-dup-team/deja-dup/trunk

Commit message

Description of the change

Preview Diff

Subscribers

1	=== modified file 'AUTHORS'
2	--- AUTHORS 2009-06-16 17:30:58 +0000
3	+++ AUTHORS 2009-09-03 07:06:47 +0000
4	@@ -7,6 +7,14 @@
5	Copyright: 2008-2009, Michael Terry <mike@mterry.name>
6	License: GPL-3+
7
8	+Files: monitor/*
9	+Copyright: 2009, Andrew Fister <temposs@gmail.com>
10	+License: GPL-3+
11	+
12	+Files: configure.ac
13	+Copyright: 2009, Andrew Fister <temposs@gmail.com>
14	+License: GPL-3+
15	+
16	Files: debian/*
17	Copyright: 2008-2009, Michael Terry <mike@mterry.name>
18	2009, Jose Carlos Garcia Sogo <jsogo@debian.org>
19
20	=== modified file 'configure.ac'
21	--- configure.ac 2009-08-29 02:55:22 +0000
22	+++ configure.ac 2009-09-03 07:04:52 +0000
23	@@ -1,7 +1,8 @@
24	# -- Mode: m4; indent-tabs-mode: nil; tab-width: 2 --
25	#
26	# This file is part of Déjà Dup.
27	-# © 2008,2009 Michael Terry <mike@mterry.name>
28	+# © 2008,2009 Michael Terry <mike@mterry.name>,
29	+# © 2009 Andrew Fister <temposs@gmail.com>
30	#
31	# Déjà Dup is free software; you can redistribute it and/or modify
32	# it under the terms of the GNU General Public License as published by
33	@@ -103,7 +104,9 @@
34	AC_SUBST(LIBRARY_LIBS)
35
36	PKG_CHECK_MODULES(MONITOR,
37	- gconf-2.0)
38	+ gtk+-2.0 >= $GTK_REQ_VER
39	+ gconf-2.0
40	+ dbus-glib-1)