deja-dup-team team mailing list archive

[Andrew Fister <temposs@xxxxxxxxx>]

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.

There's one part in your comments where I agree that what I did should be
changed but I don't agree with the solution. In the part regarding Backend,
the get_default() method takes a Gtk.Window object, and there is no such
object in the current context. I created an alternative method to get the
same effect, I think:

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?

-Andrew

On Fri, Sep 4, 2009 at 7:00 PM, Michael Terry
<michael.terry@xxxxxxxxxxxxx>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.
>

-- 
https://code.launchpad.net/~temposs/deja-dup/network-manager/+merge/11123
Your team Déjà Dup Maintainers is subscribed to branch lp:deja-dup.