deja-dup-team team mailing list archive

[Michael Terry <michael.terry@xxxxxxxxxxxxx>]

Review: Needs Fixing
Deja Dup Review

Thanks for this branch!  I ran it and tested it and it was very awesome to see it actually work.  :)

Nice code comments you added around the place. :)

In files you made, please add a file header (copyright, modeline).

Use spaces not tabs (would be fixed by using modeline)

No libgee if possible.  Is PriorityQueue doing a lot of heavy lifting for us?  Is it much different than, say, Glib.Queue.  If so, we can keep it, but in general I prefer to remove dependencies.  I'm assuming Gee.ArrayList is just a nicer version of GLib.List?  That would be an easy replacement.

There are merge conflicts in configure.ac, deja-dup/Makefile, and po/deja-dup.pot

+ public Time time {public get; public set;}
doesn't need public on get; set;

+ Object(xid: xid, mode: Mode.LIST, source: source_in);
+ this.time = time_in;

Neat trick is that you don't need to name incoming source as 'source_in'.  You can do Object(source: source).  Vala knows what's what.  And you should be able to do time: time too, without that extra line.

396	+ public override void start() throws Error
397	+ {
398	+ base.start();
399	+ }
400	+
401	+ protected override void operation_finished(Duplicity dup, bool success, bool cancelled)
402	+ {
403	+ base.operation_finished(dup, success, cancelled);
404	+ }

Do we need these functions?

Changes in OperationRestore seem deletable.  Same for OperationStatus.

I'd like to see at least one test in the test suite added.

AssistantDirectoryHistory:
Lots of commented-out code
Mark table headers for translation ("File name")
"Last day" -> "Yesterday"

AssistantOperation:
1453	- this.op = null;
1454	+ op = null;

This seems like a bug.  We should null the class's pointer.

1527	- op.continue_with_passphrase(passphrase);
1528	+ //op.continue_with_passphrase(passphrase);

Why this change?  Seems like it would break other code.  Does this branch pass a "make test" in the tests directory?


UI Review:
"Deleted Files" -> "Restore Missing Files"
"Backup source" -> "Backup location:"
"Status" -> "Status:"
"File name" -> "File"
"Deleted" -> "Last seen"

I don't think we need to show the backup location in this dialog.  It's the sort of thing a user sets once and then doesn't need to be reminded about.  What would be useful is showing the current directory we are scanning in. Like "Directory: Documents/Images" or something (there's a CommonUtils function for getting a nice short name like that I believe).

Put a little bit of space between spinner and date

Make first column of table take the right amount of space from the start, not just when it adds its first file.

Can we have a line of text in the table when there's no content like "no missing files found"?

Put the status label/widget below the table, drop the leading label "Status" and just have the spinner with text like "Scanning for files from last month..."

Instead of "Done!", hide the Status label/widget.

On Summary page, again, use "location" not "source" and use colons.  Make the "Restoring files:" widget look like the label from the restore-single-file widget from AssistantRestore (that is, a list of files with commas that is entirely on the right side of the dialog).  Make the label use ngettext like AssistantRestore does too.

The progress page doesn't have a header title

Didn't seem like it handled missing folders?

Bugs:
Pressed back after letting dialog do its thing to Done! state.  Selected the one file there, pressed Forward.  Then pressed Back.  Segfaulted.
-- 
https://code.launchpad.net/~urbans/deja-dup/deja-dup.nautilus/+merge/29723
Your team Déjà Dup Maintainers is subscribed to branch lp:deja-dup.