GNOME Bugzilla – Bug 619014
CopyRecursive needs cleaning up
Last modified: 2018-07-01 08:55:48 UTC
CopyRecursive needs improving: * Exceptions are only meant as an exceptional failure notifier (they have horrible performance). So the answer to the TODO is: check for directory. * I get critical GIO warnings (just enabled stack dumping of them) * Progress reporting is probably wrong, but I'm not sure if you can easily improve on it, except for walking the whole tree first (that would be the right solution, walk the tree and count the number of files, then walk again and copy them). * Log.Debug (String.Format (Catalog.GetString("Copying {0} -> {1}"), source_file.Path, target_file.Path)); this can be: Log.Debug (Catalog.GetString("Copying {0} -> {1}"), source_file.Path, target_file.Path); Log.Debug supports parameters.
Created attachment 161412 [details] [review] Avoid use of exceptions, fix directory type comparison
Created attachment 161413 [details] [review] Fix critical GLib warning regarding canncellable
Progress reporting (and the correct use of cancellable) should be fixed in FolderExport.
The following fixes have been pushed: 53bf478 Fix critical GLib warning regarding canncellable 57f4a55 Avoid use of exceptions, fix directory type comparison Thanks for cleaning this up! One last notice: you can use extension methods as member methods instead of static methods. So instead of: CopyRecursivei (source_file, target_file ...) You can write: source_file.CopyRecursive (target_file ...) Which is a bit nicer on the eye :-).
Created attachment 161415 [details] [review] Fix critical GLib warning regarding canncellable
Created attachment 161416 [details] [review] Avoid use of exceptions, fix directory type comparison
Okay that didn't work out as planned, for some reason git-bz added new patches instead of marking the old ones as committed. Sorry for the noise!
Reopening, progress reporting isn't right yet, we need something smarter. It currently reports progress for files, but that means we get non-monotonic reporting: Suppose we have two files A and B. It would report something like: (A) 33% (A) 66% (A) 100% (B) 33% (B) 66% (B) 100% What we really want is: (Starting) 0% (A done) 50% (B done) 100% That would require walking the directory first :-) ======================================================== Also, cancellable doesn't really work correctly: if I use the supplied cancellable to abort the copy, it won't really stop copying, it'll just abort some of the operations but still walk through the entire tree. It's probably better if you pass each of the suboperations a null cancellable and check the status of the cancellable inside CopyRecursive, breaking out of the loop / methods when cancelled. This is what I think causes the GIO errors, passing null should be allowed.
Created attachment 161419 [details] [review] Use CopyRecursive as extension method
Comment on attachment 161419 [details] [review] Use CopyRecursive as extension method Attachment 161419 [details] pushed as a46f010 - Use CopyRecursive as extension method
I passed null as cancellable before, in the first call of CopyRecursive in FolderExport.cs. So null seems to cause the GLib warnings. But you're right, I should check if cancellable was cancelled and about the walk through the tree. (But then we should add a Cancel button to the ThreadProgressDialog.) To cancel it, may be we should use CopyAsync?
Created attachment 161425 [details] [review] More cleanups and clearer dialog messages
Created attachment 161426 [details] [review] No need to check result as if result is false, an exception will be thrown before.
Created attachment 161427 [details] [review] More cleanup, use Log.Debug and consistent dialog messages
Review of attachment 161425 [details] [review]: Looks good, but please don't mark debug messages as translatable. They're not user visible anyway (and I don't want to be debugging chinese messages ;-)). We don't want to give our translators more work than needed.
Review of attachment 161426 [details] [review]: Looks good.
Review of attachment 161427 [details] [review]: Looks good
Created attachment 161428 [details] [review] Don't mark debug output as translatable, logging errors to Log.Error
Created attachment 161436 [details] [review] Cleanup, consistent debug and dialog messages, debug/error output
Created attachment 161445 [details] [review] dialog messages and clean up
Created attachment 161446 [details] [review] No need to check result as if result is false, an exception will be thrown before.
Created attachment 161447 [details] [review] Cleanup, consistent debug and dialog messages, debug/error output
Attachment 161445 [details] pushed as 9e9edec - dialog messages and clean up Attachment 161446 [details] pushed as 7fb4980 - No need to check result as if result is false, an exception will be thrown before. Attachment 161447 [details] pushed as 7e2d3c5 - Cleanup, consistent debug and dialog messages, debug/error output Thanks!
f-spot is not under active development anymore, has not seen code changes for five years, and saw its last tarball release in the year 2010. Its codebase has been archived: https://gitlab.gnome.org/Archive/f-spot/commits/master Closing this report as WONTFIX as part of Bugzilla Housekeeping to reflect reality. Please feel free to reopen this ticket (or rather transfer the project to GNOME Gitlab, as GNOME Bugzilla is deprecated) if anyone takes the responsibility for active development again.