GNOME Bugzilla – Bug 602159
Split the preference to save ratings and play counts
Last modified: 2011-09-22 18:17:06 UTC
With just one setting, the meta-data is updated on each track change.
Yes, I highly agree. An example being: If you rsync to backup, then hundreds of songs are transfered when all that's changed is the playcount... The rating however, for me personally is worth backing up. cheers ;)
I hope for this change as well! This is my "Why Story": I am using Ubuntu with Ubuntu One Music Store and therefore automatically the Ubuntu One Cloud. Since I use several computers I am happy with having my songs automatically synchronized to and from the cloud when I change general metadata or specifically the rating. But what happens now is automatically uploading of each track I just have played - because the play count changed! Play count is of minor importance to me, so it would be good to be able to skip storing play count into the files - but on the opposite, the rating is very important to me! My internet upload is contantly busy when listening to music now... this is not optimal.
Created attachment 195258 [details] [review] Splits Ratings and Playcount into two functions I have tested this on ID3v2 but i do not have access to OGG files.
Comment on attachment 195258 [details] [review] Splits Ratings and Playcount into two functions Thanks for the patch, looking good! Please can you take a moment to fix these issues I found in it? (In reply to comment #3) > Created an attachment (id=195258) [details] [review] > Splits Ratings and Playcount into two functions > > I have tested this on ID3v2 but i do not have access to OGG files. Review inline: > From 832d7eb37b82d346a4deb6f439a7b6087cd5f690 Mon Sep 17 00:00:00 2001 > From: William Witt <william@witt-family.net> William Witt or Kevin Anthony? > Date: Tue, 30 Aug 2011 18:11:23 +0100 > Subject: [PATCH] Dap.MassStorage: decode URI before using it as playlist name Wrong subject. > Playlists are pulled from the filename in the uri, failure to > decode them causes playlists to be saved back with URL encoding, > so spaces or other characters where shown as weird characters in > the UI. Fixes bgo#647917. Wrong commit message :) > --- > .../Banshee.Streaming/StreamRatingTagger.cs | 98 +++++++++++++++----- > .../Banshee.Core/Banshee.Streaming/StreamTagger.cs | 7 +- > .../DatabaseTrackInfo.cs | 12 ++- > .../Banshee.Library/LibrarySchema.cs | 14 ++- > .../Banshee.Metadata/SaveTrackMetadataJob.cs | 7 +- > .../Banshee.Metadata/SaveTrackMetadataService.cs | 25 +++-- > .../Banshee.Preferences/PreferenceService.cs | 3 +- > .../Banshee.Gui/TrackActions.cs | 1 + > .../Banshee.Dap.MassStorage/MassStorageSource.cs | 8 +- > .../Banshee.Fixup/Banshee.Fixup/FixSource.cs | 5 + > 10 files changed, 130 insertions(+), 50 deletions(-) > > diff --git a/src/Core/Banshee.Core/Banshee.Streaming/StreamRatingTagger.cs b/src/Core/Banshee.Core/Banshee.Streaming/StreamRatingTagger.cs > index 714e4ff..acbd88c 100644 > --- a/src/Core/Banshee.Core/Banshee.Streaming/StreamRatingTagger.cs > +++ b/src/Core/Banshee.Core/Banshee.Streaming/StreamRatingTagger.cs > @@ -115,8 +115,7 @@ namespace Banshee.Streaming > // Overwrites all POPM frames with the new rating and playcount. > // If no *known-compatible* frames are found, a new "Banshee"-authored > // frame is also created to store this information. > - public static void StoreRatingAndPlayCount (int rating, int playcount, > - TagLib.File to_file) > + public static void StoreRating (int rating,TagLib.File to_file) Please add a space after any comma. > { > TagLib.Id3v2.Tag id3v2tag = GetTag (to_file); > if (id3v2tag == null) { > @@ -132,10 +131,8 @@ namespace Banshee.Streaming > } > > popm.Rating = BansheeToPopm (rating); > - popm.PlayCount = (ulong)playcount; > - Hyena.Log.DebugFormat ("Exporting ID3v2 Rating={0}({1}) and Playcount={2}({3}) to File \"{4}\" as Creator \"{5}\"", > + Hyena.Log.DebugFormat ("Exporting ID3v2 Rating={0}({1}) to File \"{2}\" as Creator \"{3}\"", > rating, popm.Rating, > - playcount, popm.PlayCount, > to_file.Name, popm.User); > } > > @@ -145,13 +142,43 @@ namespace Banshee.Streaming > POPM_our_creator_name, > true); > popm.Rating = BansheeToPopm (rating); > - popm.PlayCount = (ulong)playcount; > - Hyena.Log.DebugFormat ("Exporting ID3v2 Rating={0}({1}) and Playcount={2}({3}) to File \"{4}\" as Creator \"{5}\"", > + Hyena.Log.DebugFormat ("Exporting ID3v2 Rating={0}({1}) to File \"{2}\" as Creator \"{3}\"", > rating, popm.Rating, > - playcount, popm.PlayCount, > to_file.Name, POPM_our_creator_name); > } > } > + public static void StorePlayCount (int playcount, TagLib.File to_file) > + { > + TagLib.Id3v2.Tag id3v2tag = GetTag (to_file); > + if (id3v2tag == null) { > + return; > + } > + > + bool known_frames_found = false; > + foreach (TagLib.Id3v2.PopularimeterFrame popm in > + id3v2tag.GetFrames<TagLib.Id3v2.PopularimeterFrame> ()) { > + if (System.Array.IndexOf (POPM_known_creator_list, popm.User) >= 0) { > + // Found a known-good POPM frame, don't need to create a "Banshee" frame. > + known_frames_found = true; > + } > + > + popm.PlayCount = (ulong)playcount; > + Hyena.Log.DebugFormat ("Exporting ID3v2 Playcount={0}({1}) to File \"{2}\" as Creator \"{3}\"", > + playcount, popm.PlayCount, > + to_file.Name, popm.User); > + } > + > + if (!known_frames_found) { > + // No known-good frames found, create a new POPM frame (with creator string "Banshee") > + TagLib.Id3v2.PopularimeterFrame popm = TagLib.Id3v2.PopularimeterFrame.Get (id3v2tag, > + POPM_our_creator_name, > + true); > + popm.PlayCount = (ulong)playcount; > + Hyena.Log.DebugFormat ("Exporting ID3v2 Playcount={0}({1}) to File \"{2}\" as Creator \"{3}\"", > + playcount, popm.PlayCount, > + to_file.Name, popm.User); > + } > + } > > // Scans the file for *known-compatible* POPM frames, with priority given to > // frames at the top of the known creator list. > @@ -308,8 +335,7 @@ namespace Banshee.Streaming > > // Scans the file for ogg rating/playcount tags as defined by the Quod Libet standard > // All applicable tags are overwritten with the new values, regardless of tag author > - public static void StoreRatingAndPlayCount (int rating, int playcount, > - TagLib.File to_file) > + public static void StoreRating (int rating, TagLib.File to_file) > { > TagLib.Ogg.XiphComment xiphtag = GetTag (to_file); > if (xiphtag == null) { > @@ -317,23 +343,17 @@ namespace Banshee.Streaming > } > > ArrayList rating_fieldnames = new ArrayList (); > - ArrayList playcount_fieldnames = new ArrayList (); > > - // Collect list of rating and playcount tags to be updated: > + // Collect list of rating tags to be updated: Two spaces in comment, use one space. > foreach (string fieldname in xiphtag) { > if (fieldname.ToUpper ().StartsWith (rating_prefix)) { > rating_fieldnames.Add (fieldname); > - } else if (fieldname.ToUpper ().StartsWith (playcount_prefix)) { > - playcount_fieldnames.Add (fieldname); > } > } > - // Add "BANSHEE" tags if no rating/playcount tags were found: > + // Add "BANSHEE" tags if no rating tags were found: > if (rating_fieldnames.Count == 0) { > rating_fieldnames.Add (rating_prefix+ogg_our_creator_name); > } > - if (playcount_fieldnames.Count == 0) { > - playcount_fieldnames.Add (playcount_prefix+ogg_our_creator_name); > - } > > string ogg_rating = BansheeToOgg (rating); > foreach (string ratingname in rating_fieldnames) { > @@ -343,6 +363,28 @@ namespace Banshee.Streaming > to_file.Name, > ratingname.Substring (rating_prefix.Length)); > } > + } > + > + public static void StorePlayCount (int playcount, TagLib.File to_file) > + { > + TagLib.Ogg.XiphComment xiphtag = GetTag (to_file); > + if (xiphtag == null) { > + return; > + } > + > + ArrayList playcount_fieldnames = new ArrayList (); > + > + // Collect list of playcount tags to be updated: > + foreach (string fieldname in xiphtag) { > + if (fieldname.ToUpper ().StartsWith (playcount_prefix)) { > + playcount_fieldnames.Add (fieldname); > + } > + } > + // Add "BANSHEE" tags if no playcount tags were found: > + if (playcount_fieldnames.Count == 0) { > + playcount_fieldnames.Add (playcount_prefix+ogg_our_creator_name); > + } > + > string ogg_playcount = playcount.ToString (); > foreach (string playcountname in playcount_fieldnames) { > xiphtag.SetField (playcountname, ogg_playcount); > @@ -352,6 +394,7 @@ namespace Banshee.Streaming > playcountname.Substring (playcount_prefix.Length)); > } > } > + This change seems unnecessary. > } > > public static class StreamRatingTagger > @@ -369,16 +412,23 @@ namespace Banshee.Streaming > } > } > > - public static void StoreRatingAndPlayCount (int rating, int playcount, > - TagLib.File to_file) > + public static void StoreRating (int rating, TagLib.File to_file) > + { > + if ((to_file.Tag.TagTypes & TagLib.TagTypes.Id3v2) != 0) { > + ID3v2RatingTagger.StoreRating (rating, to_file); > + } > + if ((to_file.Tag.TagTypes & TagLib.TagTypes.Xiph) != 0) { > + OggRatingTagger.StoreRating (rating, to_file); > + } > + } > + > + public static void StorePlayCount (int playcount, TagLib.File to_file) > { > if ((to_file.Tag.TagTypes & TagLib.TagTypes.Id3v2) != 0) { > - ID3v2RatingTagger.StoreRatingAndPlayCount (rating, playcount, > - to_file); > + ID3v2RatingTagger.StorePlayCount (playcount, to_file); > } > if ((to_file.Tag.TagTypes & TagLib.TagTypes.Xiph) != 0) { > - OggRatingTagger.StoreRatingAndPlayCount (rating, playcount, > - to_file); > + OggRatingTagger.StorePlayCount (playcount, to_file); > } Now that StorePlayCount is a method of its own, it would make sense to have two new classes called ID3v2PlayCountTagger and OggPlayCountTagger or, if that is too much trouble, just rename ID3v2RatingTagger to ID3v2Tagger? > } > } > diff --git a/src/Core/Banshee.Core/Banshee.Streaming/StreamTagger.cs b/src/Core/Banshee.Core/Banshee.Streaming/StreamTagger.cs > index f74b0b7..8ad7675 100644 > --- a/src/Core/Banshee.Core/Banshee.Streaming/StreamTagger.cs > +++ b/src/Core/Banshee.Core/Banshee.Streaming/StreamTagger.cs > @@ -282,7 +282,7 @@ namespace Banshee.Streaming > } catch {} > } > > - public static bool SaveToFile (TrackInfo track, bool write_metadata, bool write_rating_and_play_count) > + public static bool SaveToFile (TrackInfo track, bool write_metadata, bool write_rating,bool write_play_count) Space after comma. > { > // Note: this should be kept in sync with the metadata read in StreamTagger.cs > TagLib.File file = ProcessUri (track.Uri); > @@ -321,9 +321,10 @@ namespace Banshee.Streaming > SaveIsCompilation (file, track.IsCompilation); > } > > - if (write_rating_and_play_count) { > + if (write_play_count || write_play_count) { > // FIXME move StreamRatingTagger to taglib# > - StreamRatingTagger.StoreRatingAndPlayCount (track.Rating, track.PlayCount, file); > + StreamRatingTagger.StoreRating (track.Rating, file); > + StreamRatingTagger.StorePlayCount (track.PlayCount,file); > } I guess you had a typo there: you are using the OR operator against the same variable twice (write_play_count). But anyway even if you replace the second "write_play_count" with "write_rating", I guess it's still not correct because we want here to split this if{} in two, because if write_play_count==true but write_rating==false, we want StorePlayCount to be called, but we don't want that StorePlayCount is called. > > file.Save (); > diff --git a/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs b/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs > index 37686a9..84876b9 100644 > --- a/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs > +++ b/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs > @@ -89,15 +89,19 @@ namespace Banshee.Collection.Database > BansheeQuery.RatingField > }; > Action<Root> handler = delegate { > - if (SaveTrackMetadataService.WriteRatingsAndPlayCountsEnabled.Value) { > - transient_fields.Remove (BansheeQuery.PlayCountField); > + if (SaveTrackMetadataService.WriteRatingsEnabled.Value) { > transient_fields.Remove (BansheeQuery.RatingField); > } else { > - transient_fields.Add (BansheeQuery.PlayCountField); > transient_fields.Add (BansheeQuery.RatingField); > } > + if (SaveTrackMetadataService.WritePlayCountsEnabled.Value) { > + transient_fields.Remove (BansheeQuery.PlayCountField); > + }else{ Add a space before "{" and another space after "}". > + transient_fields.Add (BansheeQuery.PlayCountField); > + } > }; > - SaveTrackMetadataService.WriteRatingsAndPlayCountsEnabled.ValueChanged += handler; > + SaveTrackMetadataService.WritePlayCountsEnabled.ValueChanged += handler; > + SaveTrackMetadataService.WriteRatingsEnabled.ValueChanged += handler; > handler (null); > } > > diff --git a/src/Core/Banshee.Services/Banshee.Library/LibrarySchema.cs b/src/Core/Banshee.Services/Banshee.Library/LibrarySchema.cs > index 687ce85..1f6dec2 100644 > --- a/src/Core/Banshee.Services/Banshee.Library/LibrarySchema.cs > +++ b/src/Core/Banshee.Services/Banshee.Library/LibrarySchema.cs > @@ -78,12 +78,20 @@ namespace Banshee.Configuration.Schema > "Write metadata back to audio files", > "If enabled, metadata (tags) will be written back to audio files when using the track metadata editor." > ); > - > - public static readonly SchemaEntry<bool> WriteRatingsAndPlayCounts = new SchemaEntry<bool>( > + // I was thinking these should be indented and read something more akin to > + // also write playcount > + // also write ratings Leave comments for the commit message or for this bug unless they are essential to understand the code please. > + public static readonly SchemaEntry<bool> WriteRatings = new SchemaEntry<bool>( > "library", "write_rating", > false, > "Store ratings within supported files", > - "If enabled, rating and playcount metadata will be written back to audio files." > + "If enabled, rating metadata will be written back to audio files." > + ); > + public static readonly SchemaEntry<bool> WritePlayCounts = new SchemaEntry<bool>( > + "library", "write_count", > + false, > + "Store playcount within supported files", > + "If enabled, playcount metadata will be written back to audio files." > ); > > public static readonly SchemaEntry<bool> SortByAlbumYear = new SchemaEntry<bool>( > diff --git a/src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataJob.cs b/src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataJob.cs > index fefe51d..1ad75aa 100644 > --- a/src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataJob.cs > +++ b/src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataJob.cs > @@ -72,7 +72,8 @@ namespace Banshee.Metadata > } > > public bool WriteMetadataEnabled { get; set; } > - public bool WriteRatingsAndPlayCountsEnabled { get; set; } > + public bool WriteRatingsEnabled { get; set; } > + public bool WritePlayCountsEnabled { get; set; } > public bool RenameEnabled { get; set; } > > private HyenaSqliteCommand update_synced_at; > @@ -90,9 +91,9 @@ namespace Banshee.Metadata > bool wrote = false; > bool renamed = false; > try { > - if (WriteMetadataEnabled || WriteRatingsAndPlayCountsEnabled) { > + if (WriteMetadataEnabled || WriteRatingsEnabled || WritePlayCountsEnabled) { > Hyena.Log.DebugFormat ("Saving metadata for {0}", track); > - wrote = StreamTagger.SaveToFile (track, WriteMetadataEnabled, WriteRatingsAndPlayCountsEnabled); > + wrote = StreamTagger.SaveToFile (track, WriteMetadataEnabled, WriteRatingsEnabled,WritePlayCountsEnabled); Space after comma. Also, split the if in 2 like I brought up before. > } > > // Rename tracks only from the Music Library > diff --git a/src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataService.cs b/src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataService.cs > index 5604073..de3a937 100644 > --- a/src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataService.cs > +++ b/src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataService.cs > @@ -50,10 +50,14 @@ namespace Banshee.Metadata > Catalog.GetString ("Save tags and other metadata inside supported media files") > ); > > - public static SchemaPreference<bool> WriteRatingsAndPlayCountsEnabled = new SchemaPreference<bool> ( > - LibrarySchema.WriteRatingsAndPlayCounts, > - Catalog.GetString ("Write _ratings and play counts to files"), > - Catalog.GetString ("Enable this option to save rating and playcount metadata inside supported audio files")); > + public static SchemaPreference<bool> WriteRatingsEnabled = new SchemaPreference<bool> ( > + LibrarySchema.WriteRatings, > + Catalog.GetString ("Write _ratings to files"), > + Catalog.GetString ("Enable this option to save rating metadata inside supported audio files")); > + public static SchemaPreference<bool> WritePlayCountsEnabled = new SchemaPreference<bool> ( > + LibrarySchema.WritePlayCounts, > + Catalog.GetString ("Write play counts to files"), > + Catalog.GetString ("Enable this option to save playcount metadata inside supported audio files")); > > public static SchemaPreference<bool> RenameEnabled = new SchemaPreference<bool> ( > LibrarySchema.MoveOnInfoSave, > @@ -73,7 +77,8 @@ namespace Banshee.Metadata > { > Banshee.ServiceStack.Application.RunTimeout (10000, delegate { > WriteMetadataEnabled.ValueChanged += OnEnabledChanged; > - WriteRatingsAndPlayCountsEnabled.ValueChanged += OnEnabledChanged; > + WriteRatingsEnabled.ValueChanged += OnEnabledChanged; > + WritePlayCountsEnabled.ValueChanged += OnEnabledChanged; > RenameEnabled.ValueChanged += OnEnabledChanged; > > foreach (var source in ServiceManager.SourceManager.Sources) { > @@ -127,18 +132,20 @@ namespace Banshee.Metadata > > private void Save () > { > - if (!(WriteMetadataEnabled.Value || WriteRatingsAndPlayCountsEnabled.Value || RenameEnabled.Value)) > + if (!(WriteMetadataEnabled.Value || WriteRatingsEnabled.Value || WritePlayCountsEnabled.Value || RenameEnabled.Value)) > return; > > lock (sync) { > if (job != null) { > job.WriteMetadataEnabled = WriteMetadataEnabled.Value; > - job.WriteRatingsAndPlayCountsEnabled = WriteRatingsAndPlayCountsEnabled.Value; > + job.WriteRatingsEnabled = WriteRatingsEnabled.Value; > + job.WritePlayCountsEnabled = WritePlayCountsEnabled.Value; > job.RenameEnabled = RenameEnabled.Value; > } else { > var new_job = new SaveTrackMetadataJob () { > WriteMetadataEnabled = WriteMetadataEnabled.Value, > - WriteRatingsAndPlayCountsEnabled = WriteRatingsAndPlayCountsEnabled.Value, > + WriteRatingsEnabled = WriteRatingsEnabled.Value, > + WritePlayCountsEnabled = WritePlayCountsEnabled.Value, > RenameEnabled = RenameEnabled.Value > }; > new_job.Finished += delegate { lock (sync) { job = null; } }; > @@ -155,7 +162,7 @@ namespace Banshee.Metadata > > private void OnEnabledChanged (Root pref) > { > - if (WriteMetadataEnabled.Value || WriteRatingsAndPlayCountsEnabled.Value || RenameEnabled.Value) { > + if (WriteMetadataEnabled.Value || WriteRatingsEnabled.Value || WritePlayCountsEnabled.Value || RenameEnabled.Value) { > Save (); > } else { > if (job != null) { > diff --git a/src/Core/Banshee.Services/Banshee.Preferences/PreferenceService.cs b/src/Core/Banshee.Services/Banshee.Preferences/PreferenceService.cs > index 8ed6391..8d61651 100644 > --- a/src/Core/Banshee.Services/Banshee.Preferences/PreferenceService.cs > +++ b/src/Core/Banshee.Services/Banshee.Preferences/PreferenceService.cs > @@ -58,7 +58,8 @@ namespace Banshee.Preferences > Catalog.GetString ("Co_py files to media folders when importing"))); > > policies.Add (Banshee.Metadata.SaveTrackMetadataService.WriteMetadataEnabled); > - policies.Add (Banshee.Metadata.SaveTrackMetadataService.WriteRatingsAndPlayCountsEnabled); > + policies.Add (Banshee.Metadata.SaveTrackMetadataService.WriteRatingsEnabled); > + policies.Add (Banshee.Metadata.SaveTrackMetadataService.WritePlayCountsEnabled); > policies.Add (Banshee.Metadata.SaveTrackMetadataService.RenameEnabled); > > // Misc section > diff --git a/src/Core/Banshee.ThickClient/Banshee.Gui/TrackActions.cs b/src/Core/Banshee.ThickClient/Banshee.Gui/TrackActions.cs > index 8f6db16..34e58a6 100644 > --- a/src/Core/Banshee.ThickClient/Banshee.Gui/TrackActions.cs > +++ b/src/Core/Banshee.ThickClient/Banshee.Gui/TrackActions.cs > @@ -552,6 +552,7 @@ namespace Banshee.Gui > return; > > if (source != null && source.CanDeleteTracks) { > + Log.DebugFormat("Here: {0}",Selection.FirstIndex); This change seems not related to the bug. > source.DeleteTracks (Selection); > } > } > diff --git a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs > index 3be5b9e..1dc3324 100644 > --- a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs > +++ b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs > @@ -170,7 +170,8 @@ namespace Banshee.Dap.MassStorage > if (loaded_playlist == null) > continue; > > - PlaylistSource playlist = new PlaylistSource (System.IO.Path.GetFileNameWithoutExtension (playlist_path), this); > + string name = System.IO.Path.GetFileNameWithoutExtension (SafeUri.UriToFilename (playlist_path)); > + PlaylistSource playlist = new PlaylistSource (name, this); This change seems not related to the bug (beware, seems like a bad merge; when you pull with git I recommend you to use "git pull --rebase"). > playlist.Save (); > //Hyena.Data.Sqlite.HyenaSqliteCommand.LogAll = true; > foreach (Dictionary<string, object> element in loaded_playlist.Elements) { > @@ -514,8 +515,9 @@ namespace Banshee.Dap.MassStorage > if (track.LastSyncedStamp >= Hyena.DateTimeUtil.ToDateTime (track.FileModifiedStamp)) { > Log.DebugFormat ("Copying Metadata to File Since Sync time >= Updated Time"); > bool write_metadata = Metadata.SaveTrackMetadataService.WriteMetadataEnabled.Value; > - bool write_ratings_and_playcounts = Metadata.SaveTrackMetadataService.WriteRatingsAndPlayCountsEnabled.Value; > - Banshee.Streaming.StreamTagger.SaveToFile (copied_track, write_metadata, write_ratings_and_playcounts); > + bool write_ratings = Metadata.SaveTrackMetadataService.WriteRatingsEnabled.Value; > + bool write_playcounts = Metadata.SaveTrackMetadataService.WritePlayCountsEnabled.Value; > + Banshee.Streaming.StreamTagger.SaveToFile (copied_track, write_metadata, write_ratings, write_playcounts); > } > > copied_track.Save (false); > diff --git a/src/Extensions/Banshee.Fixup/Banshee.Fixup/FixSource.cs b/src/Extensions/Banshee.Fixup/Banshee.Fixup/FixSource.cs > index 25ed3bf..7901dd0 100644 > --- a/src/Extensions/Banshee.Fixup/Banshee.Fixup/FixSource.cs > +++ b/src/Extensions/Banshee.Fixup/Banshee.Fixup/FixSource.cs > @@ -38,6 +38,11 @@ using Hyena.Data.Sqlite; > using Banshee.ServiceStack; > using Banshee.Gui; > using Banshee.Sources; > +using Banshee.Sources.Gui; > +using Banshee.Preferences; > +using Banshee.MediaEngine; > +using Banshee.PlaybackController; > + > Also not related. > namespace Banshee.Fixup > { > -- > 1.7.4.1 >
Created attachment 195265 [details] [review] Corrected Patch
(In reply to comment #4) > (From update of attachment 195258 [details] [review]) > Thanks for the patch, looking good! > > Please can you take a moment to fix these issues I found in it? > Fixed all of them > > (In reply to comment #3) > > Created an attachment (id=195258) [details] [review] [details] [review] > > Splits Ratings and Playcount into two functions > > > > I have tested this on ID3v2 but i do not have access to OGG files. > > Review inline: > > > From 832d7eb37b82d346a4deb6f439a7b6087cd5f690 Mon Sep 17 00:00:00 2001 > > From: William Witt <william@witt-family.net> > > William Witt or Kevin Anthony? > > > > Date: Tue, 30 Aug 2011 18:11:23 +0100 > > Subject: [PATCH] Dap.MassStorage: decode URI before using it as playlist name > > Wrong subject. > > > Playlists are pulled from the filename in the uri, failure to > > decode them causes playlists to be saved back with URL encoding, > > so spaces or other characters where shown as weird characters in > > the UI. Fixes bgo#647917. > > Wrong commit message :) > > This is strange... i think i had a bad merge > > --- > > .../Banshee.Streaming/StreamRatingTagger.cs | 98 +++++++++++++++----- > > .../Banshee.Core/Banshee.Streaming/StreamTagger.cs | 7 +- > > .../DatabaseTrackInfo.cs | 12 ++- > > .../Banshee.Library/LibrarySchema.cs | 14 ++- > > .../Banshee.Metadata/SaveTrackMetadataJob.cs | 7 +- > > .../Banshee.Metadata/SaveTrackMetadataService.cs | 25 +++-- > > .../Banshee.Preferences/PreferenceService.cs | 3 +- > > .../Banshee.Gui/TrackActions.cs | 1 + > > .../Banshee.Dap.MassStorage/MassStorageSource.cs | 8 +- > > .../Banshee.Fixup/Banshee.Fixup/FixSource.cs | 5 + > > 10 files changed, 130 insertions(+), 50 deletions(-) > > > > diff --git a/src/Core/Banshee.Core/Banshee.Streaming/StreamRatingTagger.cs b/src/Core/Banshee.Core/Banshee.Streaming/StreamRatingTagger.cs > > index 714e4ff..acbd88c 100644 > > --- a/src/Core/Banshee.Core/Banshee.Streaming/StreamRatingTagger.cs > > +++ b/src/Core/Banshee.Core/Banshee.Streaming/StreamRatingTagger.cs > > @@ -115,8 +115,7 @@ namespace Banshee.Streaming > > // Overwrites all POPM frames with the new rating and playcount. > > // If no *known-compatible* frames are found, a new "Banshee"-authored > > // frame is also created to store this information. > > - public static void StoreRatingAndPlayCount (int rating, int playcount, > > - TagLib.File to_file) > > + public static void StoreRating (int rating,TagLib.File to_file) > > Please add a space after any comma. > > > > { > > TagLib.Id3v2.Tag id3v2tag = GetTag (to_file); > > if (id3v2tag == null) { > > @@ -132,10 +131,8 @@ namespace Banshee.Streaming > > } > > > > popm.Rating = BansheeToPopm (rating); > > - popm.PlayCount = (ulong)playcount; > > - Hyena.Log.DebugFormat ("Exporting ID3v2 Rating={0}({1}) and Playcount={2}({3}) to File \"{4}\" as Creator \"{5}\"", > > + Hyena.Log.DebugFormat ("Exporting ID3v2 Rating={0}({1}) to File \"{2}\" as Creator \"{3}\"", > > rating, popm.Rating, > > - playcount, popm.PlayCount, > > to_file.Name, popm.User); > > } > > > > @@ -145,13 +142,43 @@ namespace Banshee.Streaming > > POPM_our_creator_name, > > true); > > popm.Rating = BansheeToPopm (rating); > > - popm.PlayCount = (ulong)playcount; > > - Hyena.Log.DebugFormat ("Exporting ID3v2 Rating={0}({1}) and Playcount={2}({3}) to File \"{4}\" as Creator \"{5}\"", > > + Hyena.Log.DebugFormat ("Exporting ID3v2 Rating={0}({1}) to File \"{2}\" as Creator \"{3}\"", > > rating, popm.Rating, > > - playcount, popm.PlayCount, > > to_file.Name, POPM_our_creator_name); > > } > > } > > + public static void StorePlayCount (int playcount, TagLib.File to_file) > > + { > > + TagLib.Id3v2.Tag id3v2tag = GetTag (to_file); > > + if (id3v2tag == null) { > > + return; > > + } > > + > > + bool known_frames_found = false; > > + foreach (TagLib.Id3v2.PopularimeterFrame popm in > > + id3v2tag.GetFrames<TagLib.Id3v2.PopularimeterFrame> ()) { > > + if (System.Array.IndexOf (POPM_known_creator_list, popm.User) >= 0) { > > + // Found a known-good POPM frame, don't need to create a "Banshee" frame. > > + known_frames_found = true; > > + } > > + > > + popm.PlayCount = (ulong)playcount; > > + Hyena.Log.DebugFormat ("Exporting ID3v2 Playcount={0}({1}) to File \"{2}\" as Creator \"{3}\"", > > + playcount, popm.PlayCount, > > + to_file.Name, popm.User); > > + } > > + > > + if (!known_frames_found) { > > + // No known-good frames found, create a new POPM frame (with creator string "Banshee") > > + TagLib.Id3v2.PopularimeterFrame popm = TagLib.Id3v2.PopularimeterFrame.Get (id3v2tag, > > + POPM_our_creator_name, > > + true); > > + popm.PlayCount = (ulong)playcount; > > + Hyena.Log.DebugFormat ("Exporting ID3v2 Playcount={0}({1}) to File \"{2}\" as Creator \"{3}\"", > > + playcount, popm.PlayCount, > > + to_file.Name, popm.User); > > + } > > + } > > > > // Scans the file for *known-compatible* POPM frames, with priority given to > > // frames at the top of the known creator list. > > @@ -308,8 +335,7 @@ namespace Banshee.Streaming > > > > // Scans the file for ogg rating/playcount tags as defined by the Quod Libet standard > > // All applicable tags are overwritten with the new values, regardless of tag author > > - public static void StoreRatingAndPlayCount (int rating, int playcount, > > - TagLib.File to_file) > > + public static void StoreRating (int rating, TagLib.File to_file) > > { > > TagLib.Ogg.XiphComment xiphtag = GetTag (to_file); > > if (xiphtag == null) { > > @@ -317,23 +343,17 @@ namespace Banshee.Streaming > > } > > > > ArrayList rating_fieldnames = new ArrayList (); > > - ArrayList playcount_fieldnames = new ArrayList (); > > > > - // Collect list of rating and playcount tags to be updated: > > + // Collect list of rating tags to be updated: > > Two spaces in comment, use one space. > > > > foreach (string fieldname in xiphtag) { > > if (fieldname.ToUpper ().StartsWith (rating_prefix)) { > > rating_fieldnames.Add (fieldname); > > - } else if (fieldname.ToUpper ().StartsWith (playcount_prefix)) { > > - playcount_fieldnames.Add (fieldname); > > } > > } > > - // Add "BANSHEE" tags if no rating/playcount tags were found: > > + // Add "BANSHEE" tags if no rating tags were found: > > if (rating_fieldnames.Count == 0) { > > rating_fieldnames.Add (rating_prefix+ogg_our_creator_name); > > } > > - if (playcount_fieldnames.Count == 0) { > > - playcount_fieldnames.Add (playcount_prefix+ogg_our_creator_name); > > - } > > > > string ogg_rating = BansheeToOgg (rating); > > foreach (string ratingname in rating_fieldnames) { > > @@ -343,6 +363,28 @@ namespace Banshee.Streaming > > to_file.Name, > > ratingname.Substring (rating_prefix.Length)); > > } > > + } > > + > > + public static void StorePlayCount (int playcount, TagLib.File to_file) > > + { > > + TagLib.Ogg.XiphComment xiphtag = GetTag (to_file); > > + if (xiphtag == null) { > > + return; > > + } > > + > > + ArrayList playcount_fieldnames = new ArrayList (); > > + > > + // Collect list of playcount tags to be updated: > > + foreach (string fieldname in xiphtag) { > > + if (fieldname.ToUpper ().StartsWith (playcount_prefix)) { > > + playcount_fieldnames.Add (fieldname); > > + } > > + } > > + // Add "BANSHEE" tags if no playcount tags were found: > > + if (playcount_fieldnames.Count == 0) { > > + playcount_fieldnames.Add (playcount_prefix+ogg_our_creator_name); > > + } > > + > > string ogg_playcount = playcount.ToString (); > > foreach (string playcountname in playcount_fieldnames) { > > xiphtag.SetField (playcountname, ogg_playcount); > > @@ -352,6 +394,7 @@ namespace Banshee.Streaming > > playcountname.Substring (playcount_prefix.Length)); > > } > > } > > + > > This change seems unnecessary. > > This change is necessary to support both OGG and id3v2 files > > } > > > > public static class StreamRatingTagger > > @@ -369,16 +412,23 @@ namespace Banshee.Streaming > > } > > } > > > > - public static void StoreRatingAndPlayCount (int rating, int playcount, > > - TagLib.File to_file) > > + public static void StoreRating (int rating, TagLib.File to_file) > > + { > > + if ((to_file.Tag.TagTypes & TagLib.TagTypes.Id3v2) != 0) { > > + ID3v2RatingTagger.StoreRating (rating, to_file); > > + } > > + if ((to_file.Tag.TagTypes & TagLib.TagTypes.Xiph) != 0) { > > + OggRatingTagger.StoreRating (rating, to_file); > > + } > > + } > > + > > + public static void StorePlayCount (int playcount, TagLib.File to_file) > > { > > if ((to_file.Tag.TagTypes & TagLib.TagTypes.Id3v2) != 0) { > > - ID3v2RatingTagger.StoreRatingAndPlayCount (rating, playcount, > > - to_file); > > + ID3v2RatingTagger.StorePlayCount (playcount, to_file); > > } > > if ((to_file.Tag.TagTypes & TagLib.TagTypes.Xiph) != 0) { > > - OggRatingTagger.StoreRatingAndPlayCount (rating, playcount, > > - to_file); > > + OggRatingTagger.StorePlayCount (playcount, to_file); > > } > > Now that StorePlayCount is a method of its own, it would make sense to have two > new classes called ID3v2PlayCountTagger and OggPlayCountTagger or, if that is > too much trouble, just rename ID3v2RatingTagger to ID3v2Tagger? > > I renamed it ID3v2Tagger, not because it was too much trouble to split it out, but there were other methods, that would also need there own classes, and having three classes each with one method seems excessive > > } > > } > > diff --git a/src/Core/Banshee.Core/Banshee.Streaming/StreamTagger.cs b/src/Core/Banshee.Core/Banshee.Streaming/StreamTagger.cs > > index f74b0b7..8ad7675 100644 > > --- a/src/Core/Banshee.Core/Banshee.Streaming/StreamTagger.cs > > +++ b/src/Core/Banshee.Core/Banshee.Streaming/StreamTagger.cs > > @@ -282,7 +282,7 @@ namespace Banshee.Streaming > > } catch {} > > } > > > > - public static bool SaveToFile (TrackInfo track, bool write_metadata, bool write_rating_and_play_count) > > + public static bool SaveToFile (TrackInfo track, bool write_metadata, bool write_rating,bool write_play_count) > > Space after comma. > > > > { > > // Note: this should be kept in sync with the metadata read in StreamTagger.cs > > TagLib.File file = ProcessUri (track.Uri); > > @@ -321,9 +321,10 @@ namespace Banshee.Streaming > > SaveIsCompilation (file, track.IsCompilation); > > } > > > > - if (write_rating_and_play_count) { > > + if (write_play_count || write_play_count) { > > // FIXME move StreamRatingTagger to taglib# > > - StreamRatingTagger.StoreRatingAndPlayCount (track.Rating, track.PlayCount, file); > > + StreamRatingTagger.StoreRating (track.Rating, file); > > + StreamRatingTagger.StorePlayCount (track.PlayCount,file); > > } > > I guess you had a typo there: you are using the OR operator against the same > variable twice (write_play_count). > But anyway even if you replace the second "write_play_count" with > "write_rating", I guess it's still not correct because we want here to split > this if{} in two, because if write_play_count==true but write_rating==false, we > want StorePlayCount to be called, but we don't want that StorePlayCount is > called. > > I split this up into two if's, this was a type on my part. > > > > file.Save (); > > diff --git a/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs b/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs > > index 37686a9..84876b9 100644 > > --- a/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs > > +++ b/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs > > @@ -89,15 +89,19 @@ namespace Banshee.Collection.Database > > BansheeQuery.RatingField > > }; > > Action<Root> handler = delegate { > > - if (SaveTrackMetadataService.WriteRatingsAndPlayCountsEnabled.Value) { > > - transient_fields.Remove (BansheeQuery.PlayCountField); > > + if (SaveTrackMetadataService.WriteRatingsEnabled.Value) { > > transient_fields.Remove (BansheeQuery.RatingField); > > } else { > > - transient_fields.Add (BansheeQuery.PlayCountField); > > transient_fields.Add (BansheeQuery.RatingField); > > } > > + if (SaveTrackMetadataService.WritePlayCountsEnabled.Value) { > > + transient_fields.Remove (BansheeQuery.PlayCountField); > > + }else{ > > Add a space before "{" and another space after "}". > > > > + transient_fields.Add (BansheeQuery.PlayCountField); > > + } > > }; > > - SaveTrackMetadataService.WriteRatingsAndPlayCountsEnabled.ValueChanged += handler; > > + SaveTrackMetadataService.WritePlayCountsEnabled.ValueChanged += handler; > > + SaveTrackMetadataService.WriteRatingsEnabled.ValueChanged += handler; > > handler (null); > > } > > > > diff --git a/src/Core/Banshee.Services/Banshee.Library/LibrarySchema.cs b/src/Core/Banshee.Services/Banshee.Library/LibrarySchema.cs > > index 687ce85..1f6dec2 100644 > > --- a/src/Core/Banshee.Services/Banshee.Library/LibrarySchema.cs > > +++ b/src/Core/Banshee.Services/Banshee.Library/LibrarySchema.cs > > @@ -78,12 +78,20 @@ namespace Banshee.Configuration.Schema > > "Write metadata back to audio files", > > "If enabled, metadata (tags) will be written back to audio files when using the track metadata editor." > > ); > > - > > - public static readonly SchemaEntry<bool> WriteRatingsAndPlayCounts = new SchemaEntry<bool>( > > + // I was thinking these should be indented and read something more akin to > > + // also write playcount > > + // also write ratings > > Leave comments for the commit message or for this bug unless they are essential > to understand the code please. > > > + public static readonly SchemaEntry<bool> WriteRatings = new SchemaEntry<bool>( > > "library", "write_rating", > > false, > > "Store ratings within supported files", > > - "If enabled, rating and playcount metadata will be written back to audio files." > > + "If enabled, rating metadata will be written back to audio files." > > + ); > > + public static readonly SchemaEntry<bool> WritePlayCounts = new SchemaEntry<bool>( > > + "library", "write_count", > > + false, > > + "Store playcount within supported files", > > + "If enabled, playcount metadata will be written back to audio files." > > ); > > > > public static readonly SchemaEntry<bool> SortByAlbumYear = new SchemaEntry<bool>( > > diff --git a/src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataJob.cs b/src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataJob.cs > > index fefe51d..1ad75aa 100644 > > --- a/src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataJob.cs > > +++ b/src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataJob.cs > > @@ -72,7 +72,8 @@ namespace Banshee.Metadata > > } > > > > public bool WriteMetadataEnabled { get; set; } > > - public bool WriteRatingsAndPlayCountsEnabled { get; set; } > > + public bool WriteRatingsEnabled { get; set; } > > + public bool WritePlayCountsEnabled { get; set; } > > public bool RenameEnabled { get; set; } > > > > private HyenaSqliteCommand update_synced_at; > > @@ -90,9 +91,9 @@ namespace Banshee.Metadata > > bool wrote = false; > > bool renamed = false; > > try { > > - if (WriteMetadataEnabled || WriteRatingsAndPlayCountsEnabled) { > > + if (WriteMetadataEnabled || WriteRatingsEnabled || WritePlayCountsEnabled) { > > Hyena.Log.DebugFormat ("Saving metadata for {0}", track); > > - wrote = StreamTagger.SaveToFile (track, WriteMetadataEnabled, WriteRatingsAndPlayCountsEnabled); > > + wrote = StreamTagger.SaveToFile (track, WriteMetadataEnabled, WriteRatingsEnabled,WritePlayCountsEnabled); > > Space after comma. > Also, split the if in 2 like I brought up before. > > In this case, this is correct, this isn't making the decision which to write, but rather if we should write. if any of the three are checked, we need to write something out to the file. SaveToFile actually makes the decision on what that is. Note we pass it Boolean values. > > } > > > > // Rename tracks only from the Music Library > > diff --git a/src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataService.cs b/src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataService.cs > > index 5604073..de3a937 100644 > > --- a/src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataService.cs > > +++ b/src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataService.cs > > @@ -50,10 +50,14 @@ namespace Banshee.Metadata > > Catalog.GetString ("Save tags and other metadata inside supported media files") > > ); > > > > - public static SchemaPreference<bool> WriteRatingsAndPlayCountsEnabled = new SchemaPreference<bool> ( > > - LibrarySchema.WriteRatingsAndPlayCounts, > > - Catalog.GetString ("Write _ratings and play counts to files"), > > - Catalog.GetString ("Enable this option to save rating and playcount metadata inside supported audio files")); > > + public static SchemaPreference<bool> WriteRatingsEnabled = new SchemaPreference<bool> ( > > + LibrarySchema.WriteRatings, > > + Catalog.GetString ("Write _ratings to files"), > > + Catalog.GetString ("Enable this option to save rating metadata inside supported audio files")); > > + public static SchemaPreference<bool> WritePlayCountsEnabled = new SchemaPreference<bool> ( > > + LibrarySchema.WritePlayCounts, > > + Catalog.GetString ("Write play counts to files"), > > + Catalog.GetString ("Enable this option to save playcount metadata inside supported audio files")); > > > > public static SchemaPreference<bool> RenameEnabled = new SchemaPreference<bool> ( > > LibrarySchema.MoveOnInfoSave, > > @@ -73,7 +77,8 @@ namespace Banshee.Metadata > > { > > Banshee.ServiceStack.Application.RunTimeout (10000, delegate { > > WriteMetadataEnabled.ValueChanged += OnEnabledChanged; > > - WriteRatingsAndPlayCountsEnabled.ValueChanged += OnEnabledChanged; > > + WriteRatingsEnabled.ValueChanged += OnEnabledChanged; > > + WritePlayCountsEnabled.ValueChanged += OnEnabledChanged; > > RenameEnabled.ValueChanged += OnEnabledChanged; > > > > foreach (var source in ServiceManager.SourceManager.Sources) { > > @@ -127,18 +132,20 @@ namespace Banshee.Metadata > > > > private void Save () > > { > > - if (!(WriteMetadataEnabled.Value || WriteRatingsAndPlayCountsEnabled.Value || RenameEnabled.Value)) > > + if (!(WriteMetadataEnabled.Value || WriteRatingsEnabled.Value || WritePlayCountsEnabled.Value || RenameEnabled.Value)) > > return; > > > > lock (sync) { > > if (job != null) { > > job.WriteMetadataEnabled = WriteMetadataEnabled.Value; > > - job.WriteRatingsAndPlayCountsEnabled = WriteRatingsAndPlayCountsEnabled.Value; > > + job.WriteRatingsEnabled = WriteRatingsEnabled.Value; > > + job.WritePlayCountsEnabled = WritePlayCountsEnabled.Value; > > job.RenameEnabled = RenameEnabled.Value; > > } else { > > var new_job = new SaveTrackMetadataJob () { > > WriteMetadataEnabled = WriteMetadataEnabled.Value, > > - WriteRatingsAndPlayCountsEnabled = WriteRatingsAndPlayCountsEnabled.Value, > > + WriteRatingsEnabled = WriteRatingsEnabled.Value, > > + WritePlayCountsEnabled = WritePlayCountsEnabled.Value, > > RenameEnabled = RenameEnabled.Value > > }; > > new_job.Finished += delegate { lock (sync) { job = null; } }; > > @@ -155,7 +162,7 @@ namespace Banshee.Metadata > > > > private void OnEnabledChanged (Root pref) > > { > > - if (WriteMetadataEnabled.Value || WriteRatingsAndPlayCountsEnabled.Value || RenameEnabled.Value) { > > + if (WriteMetadataEnabled.Value || WriteRatingsEnabled.Value || WritePlayCountsEnabled.Value || RenameEnabled.Value) { > > Save (); > > } else { > > if (job != null) { > > diff --git a/src/Core/Banshee.Services/Banshee.Preferences/PreferenceService.cs b/src/Core/Banshee.Services/Banshee.Preferences/PreferenceService.cs > > index 8ed6391..8d61651 100644 > > --- a/src/Core/Banshee.Services/Banshee.Preferences/PreferenceService.cs > > +++ b/src/Core/Banshee.Services/Banshee.Preferences/PreferenceService.cs > > @@ -58,7 +58,8 @@ namespace Banshee.Preferences > > Catalog.GetString ("Co_py files to media folders when importing"))); > > > > policies.Add (Banshee.Metadata.SaveTrackMetadataService.WriteMetadataEnabled); > > - policies.Add (Banshee.Metadata.SaveTrackMetadataService.WriteRatingsAndPlayCountsEnabled); > > + policies.Add (Banshee.Metadata.SaveTrackMetadataService.WriteRatingsEnabled); > > + policies.Add (Banshee.Metadata.SaveTrackMetadataService.WritePlayCountsEnabled); > > policies.Add (Banshee.Metadata.SaveTrackMetadataService.RenameEnabled); > > > > // Misc section > > diff --git a/src/Core/Banshee.ThickClient/Banshee.Gui/TrackActions.cs b/src/Core/Banshee.ThickClient/Banshee.Gui/TrackActions.cs > > index 8f6db16..34e58a6 100644 > > --- a/src/Core/Banshee.ThickClient/Banshee.Gui/TrackActions.cs > > +++ b/src/Core/Banshee.ThickClient/Banshee.Gui/TrackActions.cs > > @@ -552,6 +552,7 @@ namespace Banshee.Gui > > return; > > > > if (source != null && source.CanDeleteTracks) { > > + Log.DebugFormat("Here: {0}",Selection.FirstIndex); > > This change seems not related to the bug. > > > source.DeleteTracks (Selection); > > } > > } > > diff --git a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs > > index 3be5b9e..1dc3324 100644 > > --- a/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs > > +++ b/src/Dap/Banshee.Dap.MassStorage/Banshee.Dap.MassStorage/MassStorageSource.cs > > @@ -170,7 +170,8 @@ namespace Banshee.Dap.MassStorage > > if (loaded_playlist == null) > > continue; > > > > - PlaylistSource playlist = new PlaylistSource (System.IO.Path.GetFileNameWithoutExtension (playlist_path), this); > > + string name = System.IO.Path.GetFileNameWithoutExtension (SafeUri.UriToFilename (playlist_path)); > > + PlaylistSource playlist = new PlaylistSource (name, this); > > This change seems not related to the bug (beware, seems like a bad merge; when > you pull with git I recommend you to use "git pull --rebase"). > > > playlist.Save (); > > //Hyena.Data.Sqlite.HyenaSqliteCommand.LogAll = true; > > foreach (Dictionary<string, object> element in loaded_playlist.Elements) { > > @@ -514,8 +515,9 @@ namespace Banshee.Dap.MassStorage > > if (track.LastSyncedStamp >= Hyena.DateTimeUtil.ToDateTime (track.FileModifiedStamp)) { > > Log.DebugFormat ("Copying Metadata to File Since Sync time >= Updated Time"); > > bool write_metadata = Metadata.SaveTrackMetadataService.WriteMetadataEnabled.Value; > > - bool write_ratings_and_playcounts = Metadata.SaveTrackMetadataService.WriteRatingsAndPlayCountsEnabled.Value; > > - Banshee.Streaming.StreamTagger.SaveToFile (copied_track, write_metadata, write_ratings_and_playcounts); > > + bool write_ratings = Metadata.SaveTrackMetadataService.WriteRatingsEnabled.Value; > > + bool write_playcounts = Metadata.SaveTrackMetadataService.WritePlayCountsEnabled.Value; > > + Banshee.Streaming.StreamTagger.SaveToFile (copied_track, write_metadata, write_ratings, write_playcounts); > > } > > > > copied_track.Save (false); > > diff --git a/src/Extensions/Banshee.Fixup/Banshee.Fixup/FixSource.cs b/src/Extensions/Banshee.Fixup/Banshee.Fixup/FixSource.cs > > index 25ed3bf..7901dd0 100644 > > --- a/src/Extensions/Banshee.Fixup/Banshee.Fixup/FixSource.cs > > +++ b/src/Extensions/Banshee.Fixup/Banshee.Fixup/FixSource.cs > > @@ -38,6 +38,11 @@ using Hyena.Data.Sqlite; > > using Banshee.ServiceStack; > > using Banshee.Gui; > > using Banshee.Sources; > > +using Banshee.Sources.Gui; > > +using Banshee.Preferences; > > +using Banshee.MediaEngine; > > +using Banshee.PlaybackController; > > + > > > > Also not related. > > > namespace Banshee.Fixup > > { > > -- > > 1.7.4.1 > >
(In reply to comment #6) > (In reply to comment #4) > > (From update of attachment 195258 [details] [review] [details]) > > Thanks for the patch, looking good! > > > > Please can you take a moment to fix these issues I found in it? > > > Fixed all of them Thanks! Some more discussion in line: > > > + > > > string ogg_playcount = playcount.ToString (); > > > foreach (string playcountname in playcount_fieldnames) { > > > xiphtag.SetField (playcountname, ogg_playcount); > > > @@ -352,6 +394,7 @@ namespace Banshee.Streaming > > > playcountname.Substring (playcount_prefix.Length)); > > > } > > > } > > > + > > > > This change seems unnecessary. > > > > > > This change is necessary to support both OGG and id3v2 files I was just referring to the "+" that adds a new line :) I see you already corrected in the new patch. > > > > Now that StorePlayCount is a method of its own, it would make sense to have two > > new classes called ID3v2PlayCountTagger and OggPlayCountTagger or, if that is > > too much trouble, just rename ID3v2RatingTagger to ID3v2Tagger? > > > > > > I renamed it ID3v2Tagger, not because it was too much trouble to split it out, > but there were other methods, that would also need there own classes, and > having three classes each with one method seems excessive Cool thanks. > I feel that instead of the UI looking like: > <CB> Write metadata to files > <CB> Write ratings To files > <CB> Write playcounts to files > > it would look better like this > > <CB> Write metadata to files > <CB> Include ratings > <CB> Include playcounts > > But this is design decision, and would imply that inorder for ratings or > playcounts to be writen, you would have to have metadata enabled The patch looks fine now, however I still needs to test it, and anyway it cannot land banshee before 2.2 version is released, because we're in bugfix mode now (and this is a feature request). In the meantime, you could take a look at changing the UI as well like you mention above, as it seems a fair point to me. But if you do this, please: - Do the work in a separate patch (you can commit your 2nd patch on top of your first one, and still use git format-patch AFAIK, but I guess you would need to state the the 2nd patch depends on the 1st one). - Make it so the checkboxes 'Include {ratings|playcounts}' get disabled if the "Write metadata to files is not checked". (Also, you would get Bonus points too if, after these 2 patches get committed to Banshee, you go ahead and fix the FIXME that is in there, about moving all that StreamTagger functionality to TagLibSharp. But as always, a separate patch after these 2 :) ) Thanks!
I hope it is allowed to say THANX for this work within the comments here... :)
Comment on attachment 195265 [details] [review] Corrected Patch Tried to test this patch but it doesn't compile for me: Making all in Banshee.Services MCS ../../../bin/Banshee.Services.dll ./Banshee.Metadata/Tests/TaglibReadWriteTests.cs(188,39): error CS1501: No overload for method `SaveToFile' takes `3' arguments /home/knocte/banshee/bin/Banshee.Core.dll (Location of the symbol related to previous error) Compilation failed: 1 error(s), 0 warnings Kevin, to reproduce it I think you need to autogen.sh with the parameter "--enable-tests" Can you fix this and post a new patch please?
Created attachment 197119 [details] [review] Fixes Test problem
Review of attachment 197119 [details] [review]: Look good to me, just make sure to add a few lines of explanation in the commit message.
Comment on attachment 197119 [details] [review] Fixes Test problem Committed in http://git.gnome.org/browse/banshee/commit/?id=f2fcfefe796512b0e571932a370bf133dd4546e3 Thanks Kevin!
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.