I agree.

Me too.

I agree if Banshee is to progress as a media player.

It's now possible to set the location of the Video library, but it still isn't possible to set File Policies ("Copy files to media folders when importing") separately for the video library.

I like that Banshee manages my music library for me, but I don't want Banshee to automatically group my videos into folders based on "Artist" and "Album," which don't even apply for most videos.

I'm going to update the title of this to be a bit more descriptive.

Created attachment 174324 [details] [review]
Allows to decide for each Source

This should work, the problem is I can't test it with importing from CD or IPod.
And I don't know how and where to migrate the old global setting.

Any Progress on this patch?

Requesting review

Created attachment 182205 [details] [review]
Added Upgrade Routine

I added an upgrade Routine.

Problems:
 - The import is also shown at the AudioBookSource (Extension Enabled) and the PodcastSource
 - Still no Video Directory Structure

For the Video Directory Structure, what would be a useful structur for storing videos?

Created attachment 182389 [details] [review]
Import Settings only on Video and Audio Source

Now only the filepolicies are missing.

Any progress on this patch?

I really would like to see it in 2.0 as I think it's a quite important feature.

The sorting should be handled by a xbmc style engine, allowing movies and tv shows to always be separate, in both banshee and folder structure. Folder structure should be Show Name/Season X/ with another setting for file name. At no point should tags be expected to be present for sorting, as most people already have a folder structure of some sort where the names should be taken from initially.

If the folder structure is already there, importing shouldn't do anything to the videos. Not copying them to any folders, just keep them where they are.

That's what my patch would be for.

Any progress on this patch?

Review of attachment 182389 [details] [review]:

Minor bits

style, lacks space after //

+                //Get out, Not a local Library 

Likely needs update for dbus-sharp

+      <Package>ndesk-dbus-glib-1.0</Package>

Has hyena commit

--- a/src/Hyena
+++ b/src/Hyena
@@ -1 +1 @@
-Subproject commit abb28069ee64f9597abf69c3f7528cd2feb1295d
+Subproject commit b0a0a9c96f52f141f84e0e6bdd8100b26535b900

Created attachment 209424 [details] [review]
Fixes the code style and brings it up to master/head

I'm sorry for the "late" response.
I brought the patch up to date, I hope it could still be applied.

Anything on this, I would still like to be able not to import my Video Library.

Comment on attachment 209424 [details] [review]
Fixes the code style and brings it up to master/head

Hi Samuel! Thanks for the patch, and sorry it took so long to review.

I'll do a small review in-line, but it will be mostly questions or small nitpicks:


> diff --git a/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs b/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs
> index 6b8fe23..d7e066f 100644
> --- a/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs
> +++ b/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs
> @@ -731,6 +731,12 @@ namespace Banshee.Collection.Database
>          {
>              bool copy_success = true;
>  
> +            LibrarySource library_source = PrimarySource as LibrarySource;
> +            if (library_source == null) {
> +                // Get out, not a local Library
> +                return false;
> +            }
> +
>              SafeUri old_uri = this.Uri;
>              if (old_uri == null) {
>                  // Get out quick, no URI set yet.
> @@ -739,7 +745,7 @@ namespace Banshee.Collection.Database
>  
>              bool in_library = old_uri.IsLocalPath ? old_uri.AbsolutePath.StartsWith (PrimarySource.BaseDirectoryWithSeparator) : false;
>  
> -            if (!in_library && (LibrarySchema.CopyOnImport.Get () || force_copy)) {
> +            if (!in_library && (library_source.CopyOnImport || force_copy)) {

If you're already doing the  check above to see if the PrimarySource property is of LibrarySource type, then I guess the "in_library" variable is not needed anymore and can be removed?


>                  string new_filename = PathPattern.BuildFull (PrimarySource.BaseDirectory, this, Path.GetExtension (old_uri.ToString ()));
>                  SafeUri new_uri = new SafeUri (new_filename);
>  
> @@ -757,7 +763,7 @@ namespace Banshee.Collection.Database
>                                  new_filename = String.Format ("{0} ({1}){2}", filename_no_ext, duplicate_index, extension);
>                                  new_uri = new SafeUri (new_filename);
>                                  duplicate_index++;
> -                          }
> +                            }
>                          }
>                      }

Don't include unrelated changes to the patch please.
(I've committed this small indentation one right now, as a separate commit, so it doesn't bother us anymore)



> diff --git a/src/Core/Banshee.Services/Banshee.Library/LibrarySchema.cs b/src/Core/Banshee.Services/Banshee.Library/LibrarySchema.cs
> index efc5da4..d79dbf9 100644
> --- a/src/Core/Banshee.Services/Banshee.Library/LibrarySchema.cs
> +++ b/src/Core/Banshee.Services/Banshee.Library/LibrarySchema.cs
> @@ -34,6 +34,9 @@ namespace Banshee.Configuration.Schema
>  {
>      public static class LibrarySchema
>      {
> +        // Deprecated, don't use in new Code
> +        internal static readonly SchemaEntry<bool> CopyOnImport = new SchemaEntry<bool> ("library", "copy_on_import",false, null, null);
> +
>          public static readonly SchemaEntry<string> FolderPattern = new SchemaEntry<string>(
>              "library", "folder_pattern",
>              "%album_artist%%path_sep%%album%",
> @@ -55,13 +58,6 @@ namespace Banshee.Configuration.Schema
>                  "%album_artist_initial%, %artist% (deprecated, use %album_artist%)."
>          );
>  
> -        public static readonly SchemaEntry<bool> CopyOnImport = new SchemaEntry<bool>(
> -            "library", "copy_on_import",
> -            false,
> -            "Copy music on import",
> -            "Copy and rename music to banshee music library directory when importing"
> -        );

It's great that you add a comment noting that this SchemaEntry is deprecated, however, why moving it to the top? Cannot you leave it as is? And actually, why not removing it completely? (I just checked, and you're removing in the patch all the places where it was called, so I guess you can safely remove it.)


>          public static readonly SchemaEntry<bool> MoveOnInfoSave = new SchemaEntry<bool>(
>              "library", "move_on_info_save",
>              false,
> diff --git a/src/Core/Banshee.Services/Banshee.Library/LibrarySource.cs b/src/Core/Banshee.Services/Banshee.Library/LibrarySource.cs
> index 6e3c03d..6ce0dd0 100644
> --- a/src/Core/Banshee.Services/Banshee.Library/LibrarySource.cs
> +++ b/src/Core/Banshee.Services/Banshee.Library/LibrarySource.cs
> @@ -111,6 +120,18 @@ namespace Banshee.Library
>  
>          public abstract string DefaultBaseDirectory { get; }
>  
> +        public bool CopyOnImport {
> +            get { return copy_on_import.Get (); }
> +
> +            protected set {
> +                copy_on_import.Set (value);
> +            }
> +        }
> +
> +        protected virtual bool HasCopyOnImport{
	
You're missing a space between the brace and the property name.


> diff --git a/src/Core/Banshee.Services/Banshee.Library/MusicLibrarySource.cs b/src/Core/Banshee.Services/Banshee.Library/MusicLibrarySource.cs
> index e1432c2..516b6d2 100644
> --- a/src/Core/Banshee.Services/Banshee.Library/MusicLibrarySource.cs
> +++ b/src/Core/Banshee.Services/Banshee.Library/MusicLibrarySource.cs
> @@ -110,6 +117,10 @@ namespace Banshee.Library
>              get { return non_default_smart_playlists; }
>          }
>  
> +        protected override bool HasCopyOnImport{

You're missing a space between the brace and the property name.

> diff --git a/src/Core/Banshee.Services/Banshee.Library/VideoLibrarySource.cs b/src/Core/Banshee.Services/Banshee.Library/VideoLibrarySource.cs
> index 64e2f14..58d13a3 100644
> --- a/src/Core/Banshee.Services/Banshee.Library/VideoLibrarySource.cs
> +++ b/src/Core/Banshee.Services/Banshee.Library/VideoLibrarySource.cs
> @@ -63,8 +64,19 @@ namespace Banshee.Library
>                    </column>
>                  </column-controller>
>              ", Catalog.GetString ("Produced By")));
> -        }
>  
> +            // Migrate the old import settings, if necessary
> +            if (DatabaseConfigurationClient.Client.Get<int> ("VideoImportSettingsMigrated", 0) != 1) {
> +                bool oldImportSettings = OldImportSetting.Get ();
> +                CopyOnImport = oldImportSettings;
> +                DatabaseConfigurationClient.Client.Set<int> ("VideoImportSettingsMigrated", 1);
> +            }
> +            /* TODO Create VideoFileNamePattern

Mmmm, if this patch doesn't implement the pattern for VideoLibrary yet, cannot we rather remove all the CopyOnImport details related to the Video from it?

The patch would be much simpler if we do this, because we could still reuse the CopyOnImport setting, but apply it only to elements imported into the Music library. (Because the fact that video files were organized in Album names and the like was a bug on its own, no need to
code a feature to prevent a bug, if we can just prevent the bug! And yes, the part of this patch that moves the setting to the SourceSpecific tab would still be valid.)


So, thanks again for the patch! Waiting for your comments.

Created attachment 214966 [details] [review]
Missing Spaces, Library Schema is gone.

Sorry for the time, had some projects to hand in at university.

(In reply to comment #16)
> >              bool in_library = old_uri.IsLocalPath ? old_uri.AbsolutePath.StartsWith (PrimarySource.BaseDirectoryWithSeparator) : 
> If you're already doing the  check above to see if the PrimarySource property
> is of LibrarySource type, then I guess the "in_library" variable is not needed
> anymore and can be removed?
If I read the code here correctly, it's not about if the library is local,
it's somehow also about that, but additionally it checks if the file is already
in the directory of the library, (Then no copy is needed).

> Don't include unrelated changes to the patch please.
> (I've committed this small indentation one right now, as a separate commit, so
> it doesn't bother us anymore)
I didn't realise that I did that. Sorry I check the patch file more carefully the next time.

> It's great that you add a comment noting that this SchemaEntry is deprecated,
> however, why moving it to the top? Cannot you leave it as is? And actually, why
> not removing it completely? (I just checked, and you're removing in the patch
> all the places where it was called, so I guess you can safely remove it.)
I saw the practice with the OldLocationSchema, so I thought I put it at the same place and leave it there. I removed it now.

 
> Mmmm, if this patch doesn't implement the pattern for VideoLibrary yet, cannot
> we rather remove all the CopyOnImport details related to the Video from it?
> 
> The patch would be much simpler if we do this, because we could still reuse the
> CopyOnImport setting, but apply it only to elements imported into the Music
> library. (Because the fact that video files were organized in Album names and
> the like was a bug on its own, no need to
> code a feature to prevent a bug, if we can just prevent the bug! And yes, the
> part of this patch that moves the setting to the SourceSpecific tab would still
> be valid.)
There is not a lot of code in there that would be removed if we don't have patterns, because we would still need the import setting for video or?

Oh I forgot, I added the VideoPattern, but just the same as the music pattern.

(In reply to comment #17)
> Created an attachment (id=214966) [details] [review]
> Missing Spaces, Library Schema is gone.
> 
> Sorry for the time, had some projects to hand in at university.

No problem! Thanks for the second patch.


> If I read the code here correctly, it's not about if the library is local,
> it's somehow also about that, but additionally it checks if the file is already
> in the directory of the library, (Then no copy is needed).

Ah, you may be right.


> There is not a lot of code in there that would be removed if we don't have
> patterns, because we would still need the import setting for video or?
(In reply to comment #18)
> Oh I forgot, I added the VideoPattern, but just the same as the music pattern.

My point is: why have a VideoPattern that is the same as the music pattern? It doesn't make sense to me. Until we have a proper VideoPattern (based on ShowName, Season, etc. like other user suggested), I don't see the point of seeing it there. Or, in any case, turn it off by default (without importing the setting, because even if the setting was ON, it was not very useful for video files).

I'm just saying that the 2nd patch that implements the proper VideoPattern (if anyone steps up to do it) could be the one in which we enable the setting for the video source; until then, I don't see much value on it.

Oh and one nit:
+            get { return copy_on_import.Get (); }
+
+            protected set {
+                copy_on_import.Set (value);
+            }

Put the set{} in just one line, and no blank line between the set and the get.

Thanks

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

Question if Patch changes behaviour for Audiobooks and Podcasts?
But Audiobooks don't move files anyway (#608381), at least thats my experience after trying it now with several files.

Created attachment 216624 [details] [review]
This removes the VideoFileNamePattern, but still allows import Settings dependend on Source

As talked with knocte in IRC, this patch implements the "Import Setting" part, so one can defined if he wants to import in the Library folder or not.

(In reply to comment #22)
> Created an attachment (id=216624) [details] [review]
> This removes the VideoFileNamePattern, but still allows import Settings
> dependend on Source
> 
> As talked with knocte in IRC, this patch implements the "Import Setting" part,
> so one can defined if he wants to import in the Library folder or not.

This looks good in general.

Just tested the patch and I have 1 more question: I'm guessing we missed the "Update file and folder names" preference, which I guess uses the settings "Folder hierarchy" and "File name". As we're moving the latter to be source specific, this should be source specific too, no?

Thanks Samuel for being so patient!

It's of course possible :) But the default Setting should be to true or? Otherwise you have to go through every Source and change this Setting, normally if you want import, you also want to have your files renamed or?

Created attachment 217606 [details] [review]
Integrates also the "Rename File and Folders" option. Fixes gettext missing.

This should work out now. Please test.

Created attachment 217616 [details] [review]
Sorry for this other Patch, Importer now respects HasCopyOnImport Bool

Committed with minor style fixes. Thank you Samuel!

Alex, thanks for committing and for the review, but I'm afraid Samuel's patch doesn't fix this bug completely. If we take in account only the title of the bug "File Policies (copy files to folders) should be source-specific", yes, it fixes it. But Maciej, in comment#0, asked for a Video-oriented structured.

Well, anyway, I guess it's better that we track the next steps in a new bug! Maciej: could you open it?

Thanks again Samuel.

Oops, re-marking as FIXED.

Yes, please open a new bug for the Video library organisation options.