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.
_______________________________________________
Mailing list: https://launchpad.net/~deja-dup-team
Post to : [email protected]
Unsubscribe : https://launchpad.net/~deja-dup-team
More help : https://help.launchpad.net/ListHelp
