*** Bug 640300 has been marked as a duplicate of this bug. ***

From Bug 640300, it doesn't take a typo to run into this kind of database annihilation; if your music library is stored on an external drive, it's as simple as hitting 'rescan' while the drive is disconnected.  Since this clears out all of the user's data, almost certainly unintentionally, I'd consider this much more significant than most bugs -- even crashers -- so I'm increasing the severity to critical.

It would be awesome if someone would feel inspired to tackle this before 2.0.

Understood. And thank you for the apology Michael. I'm a software engineer undergrad so I know unforeseen events can occur on small projects let alone large projects such as this.

I've been taught C# academically, but program in PHP professionally, so I can't promise much, but if someone can push in me in the right direction, I might be able to help.

If not, I know how you programmers like seeing "+1" in the comments of a bug :p

Created attachment 200771 [details] [review]
Rescan warning when delete a lot of file

This patch is much more complicated than it deserve because I want to keep Rescan process in Banshee.Service but it mean that I have no access to Gtk dialog. So I put a questionPrompt in Client.
I am not really happy with this soluton but that was the cleaner I found.

Else we can move Rescan process (RescanPipeline.cs) to Banshee.ThickClient assembly.

The only other option is have an interface/abstract class on service and implementor in ThickClient. ReUse Client class was the cleaner way.
Bertrand Or Andres, if you find other better option feel free to do so...

Review of attachment 200771 [details] [review]:

Thanks for the patch! Below my comments:

>  
> +        public override bool QuestionPrompt (string title, string message)
> +        {
> +            Log.DebugFormat ("{0} : Auto anwser: yes", message);
> +            return true;

If you're going this route (I mean, using a generic QuestionPrompt that does nothing in the nonGUI case), I would prefer the default to return false.


> diff --git a/src/Core/Banshee.Services/Banshee.Collection/RescanPipeline.cs b/src/Core/Banshee.Services/Banshee.Collection/RescanPipeline.cs
> index 82441e2..83837bc 100644
> --- a/src/Core/Banshee.Services/Banshee.Collection/RescanPipeline.cs
> +++ b/src/Core/Banshee.Services/Banshee.Collection/RescanPipeline.cs
> @@ -104,16 +104,24 @@ namespace Banshee.Collection
>                  @"WHERE PrimarySourceID = ? AND Uri LIKE ? ESCAPE '\' AND LastSyncedStamp IS NOT NULL AND LastSyncedStamp < ?";
>              string uri = Hyena.StringUtil.EscapeLike (new SafeUri (psource.BaseDirectoryWithSeparator).AbsoluteUri) + "%";
>  
> -            ServiceManager.DbConnection.Execute (String.Format (@"BEGIN;
> -                    DELETE FROM CorePlaylistEntries WHERE TrackID IN (SELECT TrackID FROM CoreTracks {0});
> -                    DELETE FROM CoreSmartPlaylistEntries WHERE TrackID IN (SELECT TrackID FROM CoreTracks {0});
> -                    DELETE FROM CoreTracks {0}; COMMIT",
> -                condition),
> -                psource.DbId, uri, scan_started,
> -                psource.DbId, uri, scan_started,
> -                psource.DbId, uri, scan_started
> -            );
> -
> +            int total = ServiceManager.DbConnection.Query<int> (String.Format (@"SELECT count(*) FROM CoreTracks {0};", condition));
> +            bool delete = true;
> +            if (total > 50) {
> +                delete = Application.ActiveClient.QuestionPrompt (String.Format (
> +                        Catalog.GetString ("Banshee detect that {0} files have been erased.{1}Do you want that banshee clean up all metadata?"), total, Environment.NewLine),
> +                    Catalog.GetString ("Rescan delete many files"));
> +            }
> +            if (delete) {
> +                ServiceManager.DbConnection.Execute (String.Format (@"BEGIN;
> +                        DELETE FROM CorePlaylistEntries WHERE TrackID IN (SELECT TrackID FROM CoreTracks {0});
> +                        DELETE FROM CoreSmartPlaylistEntries WHERE TrackID IN (SELECT TrackID FROM CoreTracks {0});
> +                        DELETE FROM CoreTracks {0}; COMMIT",
> +                    condition),
> +                    psource.DbId, uri, scan_started,
> +                    psource.DbId, uri, scan_started,
> +                    psource.DbId, uri, scan_started
> +                );
> +            }
>              // TODO prune artists/albums
>              psource.Reload ();
>              psource.NotifyTracksChanged ();
> diff --git a/src/Core/Banshee.Services/Banshee.ServiceStack/Client.cs b/src/Core/Banshee.Services/Banshee.ServiceStack/Client.cs
> index fd207d3..26d796f 100644
> --- a/src/Core/Banshee.Services/Banshee.ServiceStack/Client.cs
> +++ b/src/Core/Banshee.Services/Banshee.ServiceStack/Client.cs
> @@ -60,5 +60,7 @@ namespace Banshee.ServiceStack
>                  handler (this);
>              }
>          }
> +
> +        public abstract bool QuestionPrompt (string title, string message);
>      }
>  }
> diff --git a/src/Core/Banshee.ThickClient/Banshee.Gui/GtkBaseClient.cs b/src/Core/Banshee.ThickClient/Banshee.Gui/GtkBaseClient.cs
> index c5c5675..5ea66e0 100644
> --- a/src/Core/Banshee.ThickClient/Banshee.Gui/GtkBaseClient.cs
> +++ b/src/Core/Banshee.ThickClient/Banshee.Gui/GtkBaseClient.cs
> @@ -286,6 +286,24 @@ namespace Banshee.Gui
>              }
>          }
>  
> +        public override bool QuestionPrompt (string title, string message)
> +        {

We have already a very similar method to this one in DapActions.cs, cannot you reuse that one? And I don't mean linking ThickClient to Dap extension, but maybe move the method in DapActions to here, and then make DapActions use this as well. If you do this, you would not hardcode "50" either but use the constant that we already have there: MAX_NOWARN_TRACKS_REMOVAL.


(In reply to comment #4)
> This patch is much more complicated than it deserve because I want to keep
> Rescan process in Banshee.Service but it mean that I have no access to Gtk
> dialog. 

Fair enough.


> So I put a questionPrompt in Client.
> I am not really happy with this soluton

I feel the same.

>  but that was the cleaner I found.
> Else we can move Rescan process (RescanPipeline.cs) to Banshee.ThickClient
> assembly.

> The only other option is have an interface/abstract class on service and
> implementor in ThickClient. ReUse Client class was the cleaner way.
> Bertrand Or Andres, if you find other better option feel free to do so...

I think I might have an interest idea.

The case we have here is something like, from ThickClient in pseudcode:

void Rescan(){
  var pieceOfWork = FindWhatToDo ();
  if (IsBig(pieceOfWork.Removal.Size)) {
    if (GetFeedbackFromUserToConfirm()) {
      DoIt(pieceOfWork);
    }
  }
}

The problem here is that GetFeedbackFromUserToConfirm() needs to access the GUI, but Rescan() is in the backend and thus cannot link to Gtk. So how about splitting Rescan in two?

void PreRescan(){
  return FindWhatToDo ();
}
void PerformRescan(PieceOfWork work){
  DoIt(pieceOfWork);
}

This way, ThickClient code can do this:

var work = PreRescan();
bool confirm = false;
if (work.Removal > MAX_NOWARN_TRACKS_REMOVAL) {
  confirm = GetFeedbackFromUserToConfirm();
}
if (confirm) {
  PerformRescan(work);
}

This way we don't need to do callbacks, events or abstract methods :)
What do you think?

What about a simple solution:

Add a new Ctor for RescanPipeline with a new parameter which is a delegate.
The delegate is the function to call if not null to query user ?
With 2 ctor we have the choice to used it or not.

Here is pseudo code :
Validate function to use any time we want a window to validate:

        private bool ValidateDeletion (long deleteCount)
        {
            bool answer;
               
            Gtk.Window window = null;

            if (ServiceManager.Contains<GtkElementsService> ()) {
                window = ServiceManager.Get<GtkElementsService> ().PrimaryWindow;
            }
            using (var message_dialog = new Gtk.MessageDialog (window, 0,
                    Gtk.MessageType.Warning, Gtk.ButtonsType.YesNo, Catalog.GetString ("Rescan delete many files"))) {
                message_dialog.WindowPosition = Gtk.WindowPosition.CenterOnParent;
                message_dialog.Title = String.Format (
                    Catalog.GetString ("Banshee detect that {0} files have been erased.{1}Do you want that banshee clean up all metadata?"), deleteCount, Environment.NewLine);
                answer = (message_dialog.Run () == (int)Gtk.ResponseType.Yes);
                message_dialog.Destroy ();
            }

            return answer;
        }

// here is how to call it
new Banshee.Collection.RescanPipeline (ServiceManager.SourceManager.MusicLibrary, ValidateDeletion););

// here is change in Rescan pipeline:
+         private Func<long, bool> delete_validation;
+         private readonly long MAX_NOWARN_TRACKS_REMOVAL = 50l;
- public RescanPipeline (LibrarySource psource) : base ()
+ public RescanPipeline (LibrarySource psource, Func<long, bool> deleteValidation) : base ()

// and in OnFinished: 

long total = ServiceManager.DbConnection.Query<long> (String.Format (@"SELECT count(*) FROM CoreTracks {0};", condition));

            bool delete = true;
            if (deleteCount > MAX_NOWARN_TRACKS_REMOVAL && delete_validation != null) {
                delete = delete_validation (total);
            }
                
            if (delete) {
                ServiceManager.DbConnection.Execute (String.Format (@"BEGIN;
                        DELETE FROM CorePlaylistEntries WHERE TrackID IN (SELECT TrackID FROM CoreTracks {0});
                        DELETE FROM CoreSmartPlaylistEntries WHERE TrackID IN (SELECT TrackID FROM CoreTracks {0});
                        DELETE FROM CoreTracks {0}; COMMIT",
                    condition),
                    psource.DbId, uri, scan_started,
                    psource.DbId, uri, scan_started,
                    psource.DbId, uri, scan_started
                );

PS: I can not done your solution because We start by doing the rescan and deletion is just delete of all track which has not been updated.
So we can not know the amount of work to do before doing it.

and it is an async job because it can take a lot of time.
So it need to be done with event to avoid waiting the return of the function.

Created attachment 202437 [details] [review]
delete validation patch

here is another patch.
Still not perfect solution but it is the better I found.
The solution of Andres seems not applicable because we know what we have to delete at the end when it is finished.

Hey Olivier, give me some time to figure out if my suggestion in comment#5 really cannot be done :)

And, even if you're going to not wait, at least can you have a try at reusing the logic in the Dap extension instead of duplicating it? (The piece of code that is already using MAX_NOWARN_TRACKS_REMOVAL const) I said that in comment#5 too ;)

Do you talk about the code of DapSync.cs ?
because do not found what you talked about in DapActions.cs...
I am thining to make a class which take the text and action as parameter.
if click yes ==> launch action.
and used it for dap and rescan of course.

(In reply to comment #11)
> Do you talk about the code of DapSync.cs ?

No, I'm talking about at least reusing the method ConfirmUserAction(int tracks_to_remove) in the file src/Dap/Banshee.Dap/Banshee.Dap.Gui/DapActions.cs.

Andrès,

Do you succeed to do the comment 5.
Else Bertrand or you have you an idea about a solution for that?

I didn't have time yet sorry :(

BTW, now that bug 638130 is fixed, bug 610800 can be done and be more useful than before, however I think we should fix this bug before it because otherwise we could make the user lost tracks when he launches Banshee (so I'm setting this one as a dependency).

This makes me think that we should maybe just have a flag in the Rescan() algorithm with something like "allow_removal". This way, we could call Rescan(false) from the code that fixes bug 610800.

Created attachment 231049 [details] [review]
Patch v3

Sorry for the long delay here, Olivier and others. Finally I found time to use for this.

(In reply to comment #9)
> The solution of Andres seems not applicable because we know what we have to
> delete at the end when it is finished.

So Olivier was right in the end. I thought we could imply some trick here, like making RescanPipeline() class not start the pipeline when created, but have an Execute method, so we could mimic the code I did some time ago for confirming the user action when Dap sync was about to delete tracks, this way:

rescan_pipeline = new RescanPipeline (ServiceManager.SourceManager.MusicLibrary);
try {
    rescan_pipeline.Execute ();
} catch (PossibleUserErrorException e) {
    if (Gui.ConfirmUserAction (e.TracksToRemove)) {
        rescan_pipeline.ConfirmRemoval ();
    }
}

This way we could overcome the problem that Olivier said: we don't know how many we are about to delete until we execute it, because rescan_pipeline would still be the same object, and we would reuse its data (i.e. the scan_started DateTime) to be able to issue the same removal operation.

However the problem is that RescanPipeline is an "async" class. After you instantiate it, it starts a BatchUserJob and returns immediately to the caller, so we cannot use a try-catch here. (Unless, of course, we embraced the "async" keyword of C# 4.5... but I guess it's too early to do that.)

So I'll give up and use Olivier's initial Func<long,bool> approach. However I've done some modifications to his patch (and update it to be able to apply to current master branch), that I start to explain here:

> diff --git a/src/Core/Banshee.Services/Banshee.Collection/RescanPipeline.cs b/src/Core/Banshee.Services/Banshee.Collection/RescanPipeline.cs
> index 034e377..5eafb9a 100644
> --- a/src/Core/Banshee.Services/Banshee.Collection/RescanPipeline.cs
> +++ b/src/Core/Banshee.Services/Banshee.Collection/RescanPipeline.cs
> @@ -59,11 +59,14 @@ namespace Banshee.Collection
>          private PrimarySource psource;
>          private BatchUserJob job;
>          private TrackSyncPipelineElement track_sync;
> +        private Func<long, bool> delete_validation;
> +        private readonly long MAX_NOWARN_TRACKS_REMOVAL = 50L;

I've changed this to be an int, there's other places in Banshee's code where it's an int (when querying CoreTracks) and it works.

>  
> -        public RescanPipeline (LibrarySource psource) : base ()
> +        public RescanPipeline (LibrarySource psource, Func<long, bool> deleteValidation) : base ()
>          {
>              this.psource = psource;
>              scan_started = DateTime.Now;
> +            delete_validation = deleteValidation;
>  
>              AddElement (new Banshee.IO.DirectoryScannerPipelineElement ());
>              AddElement (track_sync = new TrackSyncPipelineElement (psource, scan_started));
> @@ -103,16 +106,26 @@ namespace Banshee.Collection
>              string condition =
>                  @"WHERE PrimarySourceID = ? AND Uri LIKE ? ESCAPE '\' AND LastSyncedStamp IS NOT NULL AND LastSyncedStamp < ?";
>              string uri = Hyena.StringUtil.EscapeLike (new SafeUri (psource.BaseDirectoryWithSeparator).AbsoluteUri) + "%";
> +            long deleteCount = ServiceManager.DbConnection.Query<long> (String.Format (@"SELECT count(*) FROM CoreTracks {0};", condition));

This cannot work, did you test your own patch Olivier? :)

The reason is that you're missing some parameters for the DB query, such as the PrimarySourceID. Then what happens is that this exception is raised:

Unhandled Exception: System.ArgumentException: Command SELECT count(*) FROM CoreTracks WHERE PrimarySourceID = ? AND CoreTracks.Uri LIKE ? ESCAPE '\' AND CoreTracks.LastSyncedStamp IS NOT NULL AND CoreTracks.LastSyncedStamp < ?; has 3 parameters, but 0 values given.
  at Hyena.Data.Sqlite.HyenaSqliteCommand.ApplyValues (System.Object[] param_values) [0x000a9] in /home/andres1210/Code/banshee/src/Hyena/Hyena.Data.Sqlite/Hyena.Data.Sqlite/HyenaSqliteCommand.cs:203 

So I fixed it in the next version of the patch which I'm attaching.

Plus, "deleteCount" name of the variable didn't comply with style guidelines (HACKING file), fixed.

Oh, and I changed "COUNT(*)" with "COUNT('x')" because in theory it's more efficient.

>  
> -            ServiceManager.DbConnection.Execute (String.Format (@"BEGIN;
> -                    DELETE FROM CorePlaylistEntries WHERE TrackID IN (SELECT TrackID FROM CoreTracks {0});
> -                    DELETE FROM CoreSmartPlaylistEntries WHERE TrackID IN (SELECT TrackID FROM CoreTracks {0});
> -                    DELETE FROM CoreTracks {0}; COMMIT",
> -                condition),
> -                psource.DbId, uri, scan_started,
> -                psource.DbId, uri, scan_started,
> -                psource.DbId, uri, scan_started
> -            );
> +            bool delete = true;

I've changed all occurrences of "delete" with "remove", because RescanPipeline actually doesn't delete files, just removes the rows from the DB, so it could be confusing.


> +            if (deleteCount > MAX_NOWARN_TRACKS_REMOVAL
> +                && delete_validation != null) {
> +
> +                delete = delete_validation (deleteCount);
> +            }

I've changed this to be safer: if delete_validation is null, it is assumed that RescanPipeline should never delete tracks.


> +
> +            if (delete) {

Instead of doing this, I've returned early in the method if there's no deletion to be done, but I left the "if" clause, to compare with deleteCount > 0 (because in this case there's nothing to be done later really, microoptimization!).


> diff --git a/src/Core/Banshee.ThickClient/Banshee.Gui/GlobalActions.cs b/src/Core/Banshee.ThickClient/Banshee.Gui/GlobalActions.cs
> index 54add5b..8654ebd 100644
> --- a/src/Core/Banshee.ThickClient/Banshee.Gui/GlobalActions.cs
> +++ b/src/Core/Banshee.ThickClient/Banshee.Gui/GlobalActions.cs
> @@ -198,6 +198,27 @@ namespace Banshee.Gui
>              Banshee.ServiceStack.Application.Shutdown ();
>          }
>  
> +        public static bool ValidateDeletion (long deleteCount)
> +        {
> +            bool answer;
> +
> +            Gtk.Window window = null;
> +
> +            if (ServiceManager.Contains<GtkElementsService> ()) {
> +                window = ServiceManager.Get<GtkElementsService> ().PrimaryWindow;
> +            }
> +            using (var message_dialog = new Gtk.MessageDialog (window, 0,
> +                    Gtk.MessageType.Warning, Gtk.ButtonsType.YesNo, Catalog.GetString ("Rescan delete many files"))) {
> +                message_dialog.WindowPosition = Gtk.WindowPosition.CenterOnParent;
> +                message_dialog.Title = String.Format (
> +                    Catalog.GetString ("Banshee detect that {0} files have been erased.{1}Do you want that banshee clean up all metadata?"), deleteCount, Environment.NewLine);

You were using the large text for the title of the window, and the short one for the text inside the window. I've fixed this. Also I've improved the text a bit (I know you're not native English speaker, and neither am I! so any native reviewing this is appreciated :) )


> +                answer = (message_dialog.Run () == (int)Gtk.ResponseType.Yes);

This was crashing for me because you were not sending the GUI call to the main thread.
I've fixed it, but actually now reusing the DapActions.ConfirmUserAction() that I had already coded a while ago for confirming removal of tracks when syncing Dap (obviously I had to move this method outside Dap code, but that's fine).


> +                message_dialog.Destroy ();
> +            }
> +
> +            return answer;
> +        }
> +
>  #endregion
>  
>  #region Edit Menu Actions
> diff --git a/src/Extensions/Banshee.LibraryWatcher/Banshee.LibraryWatcher/LibraryWatcherService.cs b/src/Extensions/Banshee.LibraryWatcher/Banshee.LibraryWatcher/LibraryWatcherService.cs
> index 6e7558e..2479423 100644
> --- a/src/Extensions/Banshee.LibraryWatcher/Banshee.LibraryWatcher/LibraryWatcherService.cs
> +++ b/src/Extensions/Banshee.LibraryWatcher/Banshee.LibraryWatcher/LibraryWatcherService.cs
> @@ -68,14 +72,35 @@ namespace Banshee.LibraryWatcher
>              AddLibrary (ServiceManager.SourceManager.VideoLibrary);
>  
>              if (ServiceManager.SourceManager.MusicLibrary.Count == 0) {
> -                new Banshee.Collection.RescanPipeline (ServiceManager.SourceManager.MusicLibrary);
> +                new Banshee.Collection.RescanPipeline (ServiceManager.SourceManager.MusicLibrary, ValidateDeletion);
>              }
>  
>              if (ServiceManager.SourceManager.VideoLibrary.Count == 0) {
> -                new Banshee.Collection.RescanPipeline (ServiceManager.SourceManager.VideoLibrary);
> +                new Banshee.Collection.RescanPipeline (ServiceManager.SourceManager.VideoLibrary, ValidateDeletion);
>              }
>          }
>  
> +        public bool ValidateDeletion (long deleteCount)

This other ValidateDeletion method had a lot of code in common with the previous ValidateDeletion. In my patch there's no such duplication.

> +        {
> +            bool answer;
> +
> +            Gtk.Window window = null;
> +
> +            if (ServiceManager.Contains<GtkElementsService> ()) {
> +                window = ServiceManager.Get<GtkElementsService> ().PrimaryWindow;
> +            }
> +            using (var message_dialog = new Gtk.MessageDialog (window, 0,
> +                    Gtk.MessageType.Warning, Gtk.ButtonsType.YesNo, Catalog.GetString ("Rescan delete many files"))) {
> +                message_dialog.WindowPosition = Gtk.WindowPosition.CenterOnParent;
> +                message_dialog.Title = String.Format (
> +                    Catalog.GetString ("Banshee detect that {0} files have been erased.{1}Do you want that banshee clean up all metadata?"), deleteCount, Environment.NewLine);
> +                answer = (message_dialog.Run () == (int)Gtk.ResponseType.Yes);
> +                message_dialog.Destroy ();
> +            }
> +
> +            return answer;
> +        }
> +
>          public void Dispose ()
>          {
>              lock (watchers) {
> diff --git a/src/Hyena b/src/Hyena
> --- a/src/Hyena
> +++ b/src/Hyena
> @@ -1 +1 @@
> -Subproject commit 5efb54b661583708ba1a55ce74727141dada7943
> +Subproject commit 5efb54b661583708ba1a55ce74727141dada7943-dirty


Last but not least, I've removed this part of the patch which was unnecessary:

s/unnecessary:/unnecessary/

Created attachment 231055 [details] [review]
Patch v4

Small corrections.

I have not test my patch because I want to know what is the best design. It was more a proof of concept to know the way to do before coding the fix...

Fair enough :)

Comment on attachment 231055 [details] [review]
Patch v4

http://git.gnome.org/browse/banshee/commit/?id=be00cb6234e5d5881c7414ac1a3a505321b9d2b1

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.