Created attachment 271292 [details] [review]
Does not import files with no read permissions.

BTW, I've tested this patch on windows, too.
here the video: http://www.youtube.com/watch?v=cHH98QehWUs

Comment on attachment 271292 [details] [review]
Does not import files with no read permissions.

Nice! Reviewing:

> From 070ea372a0fb188aa82ef42c116cba911f112e71 Mon Sep 17 00:00:00 2001
> From: Dmitry Petukhov <dimart.sp@gmail.com>
> Date: Sat, 8 Mar 2014 08:56:03 +0400
> Subject: [PATCH] Does not import files with no read permissions.

Please read our git guidelines (I think our HACKING file points to a gnome wiki where this is explained), you need to put first the area of the code you're modifying, and then a short summary (and then the bug number). In this case, I would choose "Importing: bail for nonreadable files (bgo#725185)".

> Now each track, with no read permission, is shown
> under an "Error" sources underneath the library.
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=725185

We also don't require putting the URL of the bug in the long version of the commit message, but I guess we're not strict about it.

> ---
>  src/Core/Banshee.Core/Banshee.IO/DemuxVfs.cs                       | 5 +++++
>  .../Banshee.Collection.Database/DatabaseImportManager.cs           | 7 +++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/src/Core/Banshee.Core/Banshee.IO/DemuxVfs.cs b/src/Core/Banshee.Core/Banshee.IO/DemuxVfs.cs
> index a6fe361..9dc0fe3 100644
> --- a/src/Core/Banshee.Core/Banshee.IO/DemuxVfs.cs
> +++ b/src/Core/Banshee.Core/Banshee.IO/DemuxVfs.cs
> @@ -42,5 +42,10 @@ namespace Banshee.IO
>          {
>              return TagLib.File.Create (Provider.CreateDemuxVfs (file));
>          }
> +
> +        public static IDemuxVfs CreateDemuxVfs (string file)
> +        {
> +            return Provider.CreateDemuxVfs (file);
> +        }
>      }
>  }

I'm thinking we should try to use a different API layout here. If you look at this DemuxVfs.cs file, you will find just 2 static methods that encapsulate some functionality which internally uses Provider.CreateDemuxVfs(). This method is internal. And by making a higher level method that the only thing that it does is call an internal method means that a simpler change would simply be converting Provider.CreateDemuxVfs() to public.

But, do we want that?
I'm not sure. I know that IDemuxVfs interface is public, but I don't like the fact that it inherits from an interface which is defined in an external library (taglib-sharp). I think IDemuxVfs, just by looking at its name, should be internal, because it's really not clear what's the purpose of it, until you look inside.

I'm thinking that a better approach might be this:

1) create a new interface called IFileSystemEntryPermissions:

    public interface IFileSystemEntryPermissions
    {
        bool IsReadable { get; }
        bool IsWritable { get; }
    }

2) change IDemuxVfs this way:

diff --git a/src/Core/Banshee.Core/Banshee.IO/IDemuxVfs.cs b/src/Core/Banshee.Co
index f2532ae..5acab4a 100644
--- a/src/Core/Banshee.Core/Banshee.IO/IDemuxVfs.cs
+++ b/src/Core/Banshee.Core/Banshee.IO/IDemuxVfs.cs
@@ -28,9 +28,7 @@
 
 namespace Banshee.IO
 {
-    public interface IDemuxVfs : TagLib.File.IFileAbstraction
+    public interface IDemuxVfs : IFileSystemEntryPermissions, TagLib.File.IFile
     {
-        bool IsReadable { get; }
-        bool IsWritable { get; }
     }
 }

3) Add a public static method to Banshee.IO.File class that receives a SafeUri and returns IFileSystemEntryPermissions (this method would internally call to Provider.CreateDemuxVfs().


> diff --git a/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseImportManager.cs b/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseImportManager.cs
> index 6ae854d..49a5f49 100644
> --- a/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseImportManager.cs
> +++ b/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseImportManager.cs
> @@ -171,6 +171,13 @@ namespace Banshee.Collection.Database
>                      Path.GetFileName (uri.LocalPath)));
>              }
>  
> +            var demux = Banshee.IO.DemuxVfs.CreateDemuxVfs (uri.IsLocalPath ? uri.LocalPath : uri.AbsoluteUri);
> +            if (!demux.IsReadable) {

With the proposed API above, this would become much simpler, something like:

if (!Banshee.IO.File.GetPermissions(uri).IsReadable) {

Isn't it much nicer? :)

> +                throw new InvalidFileException (String.Format (
> +                    Catalog.GetString ("File is not readable so it could not be imported: {0}"),
> +                    Path.GetFileName (uri.LocalPath)));
> +            }
> +
>              DatabaseTrackInfo track = new DatabaseTrackInfo () { Uri = uri };
>              using (var file = StreamTagger.ProcessUri (uri)) {
>                  StreamTagger.TrackInfoMerge (track, file, false, true, true);
> -- 
> 1.8.3.2
>

Created attachment 271359 [details] [review]
Importing: bail for nonreadable files

Of course, it's much better! :)

Comment on attachment 271359 [details] [review]
Importing: bail for nonreadable files

> Subject: [PATCH] Importing: bail for nonreadable files (bgo#725185)
> 
> ---

I didn't tell you to remove the paragraph after the first line of the commit message ;) I've added one and committed, thanks!

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 fix!