deja-dup-team team mailing list archive

[Urban Skudnik <urban.skudnik@xxxxxxxxx>]

Hey,

I made a couple of adjustments and need a bit of feedback on some  
others.

NOTE: All work was done on a branch that I have a backup of because my  
branch stopped working for me after merge with trunk (DD crashed if I  
try to configure it with GSettings). Will resolve the issue in  
conjunction with mterry.

On Jul 18, 2010, at 1:53 PM, Michael Terry wrote:

> 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).

Done.

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

Fixed this in AssistantDirectoryHistory and OperationFiles; will fix  
in other files if I discover I mixed tabs and spaces anywhere.

>
> 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.

Will check it out, but I think Gee.PriorityQueue has a nice feature of  
accepting compare function and can automatically sort itself out.

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

Fixed.

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

Indeed, thanks.

>
> + 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.

Thanks for source, forgot about that. time: time doesn't work because  
construct does not support Time type. Error:

OperationFiles.vala:25.26-25.35: error: construct properties not  
supported for specified
property type
	public Time time {get; construct;}

>
> 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?

Nope. They were there because I they helped me a bit in the past with  
debugging and I forgot to remove them. Thanks.

>
> Changes in OperationRestore seem deletable.  Same for OperationStatus.

Yup, seems so. I think I reverted all of my changes.

>
> I'd like to see at least one test in the test suite added.
>
> AssistantDirectoryHistory:
> Lots of commented-out code

Removed.

> Mark table headers for translation ("File name")

Done.

> "Last day" -> "Yesterday"

Woops, silly me. :> Fixed

>
> AssistantOperation:
> 1453	- this.op = null;
> 1454	+ op = null;
>
> This seems like a bug.  We should null the class's pointer.

I wasn't sure about this and whether or not I added it or not. I fixed  
to this.op both in AssistantOperation and in AssistantDirectoryHistory.

But shouldn't this.op and op be the same? As far as I understand Vala,  
this can be avoided if it is clear that its instance based (and on  
ambiguity compilation should fail)?

>
> 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?

Due to a race condition - if this is included, code (sometimes) breaks  
with the following error:

** (deja-dup:2897): CRITICAL **:  
deja_dup_operation_continue_with_passphrase: assertion `self != NULL'
failed

which I could remove if I commented-out this. I just tried something  
out but can not guarantee at this
moment that this has been resolved.

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

Done.

>
> 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).

Maybe, but since we need to scan an entire archive I have my doubts  
about the usefulness of this? Haven't implemented yet.

>
> Put a little bit of space between spinner and date

Added 5. Tell if we need more/less.

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

Done.

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

Not in the table itself. I can destroy the table and display another  
widget (label) maybe - or just write "no missing files found" under  
the table where I write "Done!"?

>
> 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..."

Done. Looks better indeed.

>
> Instead of "Done!", hide the Status label/widget.
	
I think this is a lot more informative to user than just stopping the  
scanning process?

>
> On Summary page, again, use "location" not "source" and use colons.

Done.

> 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).

Are you sure about this? If user restores multiple files, tables seems  
to be a lot more appropriate widget? In a table we can also add time  
info of when was file last seen (should I add this?)?

> Make the label use ngettext like AssistantRestore does too.

Good idea, fixed! Btw, should we also add strings for dual and four  
forms?

>
> The progress page doesn't have a header title
>
> Didn't seem like it handled missing folders?

Just tested, and it restored a folder and all of its files?

>
> Bugs:
> Pressed back after letting dialog do its thing to Done! state.   
> Selected the one file there, pressed Forward.  Then pressed Back.   
> Segfaulted.

Resolved.

> -- 
> https://code.launchpad.net/~urbans/deja-dup/deja-dup.nautilus/+merge/ 
> 29723
> You are the owner of lp:~urbans/deja-dup/deja-dup.nautilus.

-- 
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.