deja-dup-team team mailing list archive

[Michael Terry <michael.terry@xxxxxxxxxxxxx>]

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
Your team Déjà Dup Maintainers is subscribed to branch lp:deja-dup.