GNOME Bugzilla – Bug 504492
Using auto-sync on a device can delete lots of data without asking/warning the user
Last modified: 2010-03-04 23:05:52 UTC
When pressing "Synchronize iPod" on the upper right when iPod is connected and the media library has only one item, expected result is NOT removing all files in the iPod but ASK whether they should be removed or not. In my case all my songs synchronized from another machine were lost thanks to Banshee. Other information:
I'd like to work on this bug. Let's discuss what would be the best expected result. What I was thinking is something like: The synchronization operation to this device will cause the removal of X items and the addition of Y items. Do you want to continue? Opinions?
If a warning box pops up every time, the user will just ignore it and click "OK". Better to have the box pop up only if there's a good chance the user's about to lose something they care about. For example, when more than 10 songs will be deleted. I assume removing an album's worth of tracks will be an unusual enough action that the window popping up will be noticed. It could be something like this: WARNING / \ This will remove {number} of tracks from your iPod. Are you sure / ! \ you want to continue? ----- [Cancel] [Remove Tracks]
*** Bug 598953 has been marked as a duplicate of this bug. ***
I confirmed this because of the duplicate, and I updated the version to git, as this can still happen with the latest Banshee. I also changed the title to reflect that this is a potential issue for all audio players, not just iPods. It would also seem appropriate to change the "Component," but I can't find any that seem appropriate. Is there a generic DAP component that I'm missing? Also, because this has the potential to destroy a lot of data, I think the severity should be "Normal" at the least. Also I agree with John's idea about only showing the warning when some "significant" number of songs will be deleted. And it's definitely a good idea to have the confirmation button read "Remove Tracks" so that users don't just click through without reading it.
(In reply to comment #4) > I also changed the title to > reflect that this is a potential issue for all audio players, not just iPods. I'm changing it to replace Synchronization by "auto-sync", as this cannot happen with manual sync. > Also, because this has the potential to destroy a lot of data, I think the > severity should be "Normal" at the least. Agreed. > Also I agree with John's idea about only showing the warning when some > "significant" number of songs will be deleted. And it's definitely a good idea > to have the confirmation button read "Remove Tracks" so that users don't just > click through without reading it. Good idea. Apologies because I said I would work on this but for now I didn't have time. Let's see in the upcoming weekends...
This issue resulted in an even worse scenario: I auto-synced my android phone with a playlist - ejected the phone and mounted an external hard drive. Both devices were attached as /dev/sdd. Banshee started to auto-sync with the external hard disc removing all files in /Music from that disk. I'm using Ubuntu with the ppa from launchpad. Version: 1.5.4~really1.5.3-0ubuntu1~hyper1~karmic
Timo: I'm trying to understand how that could happen. Are you mounting/unmounting/ejecting things by hand on the command line? Can you attach the output of hal-device with your DAP plugged in/mounted and with your external drive mounted?
I've umounted the android phone via nautilus. Both devices were mounted via auto mount. I didn't have a look at what banshee was doing, cause if copied some other files on the phone.
Created attachment 153232 [details] hal-device output when android phone is connected
Created attachment 153233 [details] hal-device output when usb disc is connected
Created attachment 153820 [details] [review] Proposed patch, v1 This is the first proposal of a patch. I have tested it and it seems to work. Hopefully we can get less annoyed users by this.
Review of attachment 153820 [details] [review]: A few minor nitpicks, otherwise looks good! ::: src/Dap/Banshee.Dap/Banshee.Dap/DapLibrarySync.cs @@ +200,3 @@ + + } + Sync (false); May be rename `allowed` to `force`, it will make its purpose a bit more clear. @@ +256,3 @@ Catalog.GetString ("{0} to add, {1} to remove, {2} to update"); + internal class PossibleUserErrorException : OperationCanceledException { + AFAIK we are supposed to derive all custom exceptions from ApplicationException. ::: src/Dap/Banshee.Dap/Banshee.Dap/DapSync.cs @@ +362,3 @@ + } catch (DapLibrarySync.PossibleUserErrorException e) { + library_sync.Sync (); + try { You still need to use GetPluralString(), some languages have multiple plural forms, e.g. Russian and Polish have different forms for 21, 22-24 and 25-30 items: http://www.gnu.org/software/gettext/manual/gettext.html#Plural-forms
Created attachment 155221 [details] [review] Patch v2 (In reply to comment #12) > Review of attachment 153820 [details] [review]: > > A few minor nitpicks, otherwise looks good! Thanks for the review! > ::: src/Dap/Banshee.Dap/Banshee.Dap/DapLibrarySync.cs > @@ +200,3 @@ > + > + } > + Sync (false); > > May be rename `allowed` to `force`, it will make its purpose a bit more clear. Ok, done. > @@ +256,3 @@ > Catalog.GetString ("{0} to add, {1} to remove, {2} to update"); > + internal class PossibleUserErrorException : OperationCanceledException > { > + > > AFAIK we are supposed to derive all custom exceptions from > ApplicationException. Mmmm, seems right because MSDN says[1]: "User applications, not the common language runtime, throw custom exceptions derived from the ApplicationException class. The ApplicationException class differentiates between exceptions defined by applications versus exceptions defined by the system." However I liked the fact that it inherited from the meaningful "OperationCanceledException" rather than the plain "ApplicationException" and MSDN also says[1]: "If you are designing an application that needs to create its own exceptions, you are advised to derive custom exceptions from the Exception class. It was originally thought that custom exceptions should derive from the ApplicationException class; however in practice this has not been found to add significant value." BUT, I found that it seems all banshee's exceptions follow this rule (even for PlaylistImportCanceledException), so I followed the current convention. [1] http://msdn.microsoft.com/en-us/library/system.applicationexception.aspx > ::: src/Dap/Banshee.Dap/Banshee.Dap/DapSync.cs > @@ +362,3 @@ > + } catch (DapLibrarySync.PossibleUserErrorException e) { > + library_sync.Sync (); > + try { > > You still need to use GetPluralString(), some languages have multiple plural > forms, e.g. Russian and Polish have different forms for 21, 22-24 and 25-30 > items: > > http://www.gnu.org/software/gettext/manual/gettext.html#Plural-forms Wow, didn't know about this. I bet then that there are a lot of places where we should use it and we're not using it. The downside of this is that the N=1 form will be unused though.
Committed, thanks Andrés!