GNOME Bugzilla – Bug 609411
Saving metadata from the FileSystemQueue doesn't work
Last modified: 2010-05-29 18:42:53 UTC
Created attachment 153323 [details] [review] Proposed patch - please review carefully because it also adds some infrastructure changes (patch based on a general idea discussed yesterday with gabaug on IRC) This bug is present from Banshee 1.5.0 to git-master (not sure about older versions, 1.4.3 doesn't build for me in my system). Steps to reproduce: 1. Have the option "Save metadata to files enabled". 2. Double-click a song to let it be opened by Banshee in the FileSystemQueue source. 3. Right-click -> Edit track information. 4. Change something, hit save. Current results: The data gets saved to the DB, but not to the file. Expected results: The file should be modified.
Created attachment 153324 [details] [review] Patch v2 (oops, previous patch wasn't fully tested)
Created attachment 154757 [details] [review] Patch v3 v2 had already become obsolete, so I'm updating the patch to git master Also, I made a small improvement: I now use Array.Copy() instead of my own foreach to copy the values, in HyenaSqliteCommand.cs. I would appreciate a review, I'd love to have this in 1.5.5. (It's a huge drawback for people who edit metadata before copying the songs into their main library source.) Thanks.
Review of attachment 154757 [details] [review]: ::: src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataJob.cs @@ +59,3 @@ + db_ids.Add (source.DbId); + foreach (var source in ServiceManager.Get <SaveTrackMetadataService> ().Sources) { + var db_ids = new List<int> (); Linq to the rescue: var db_ids = ServiceManager.Get <SaveTrackMetadataService> ().Sources. Select (s => s.DbId.ToString ()).ToArray (); @@ +64,3 @@ + db_ids.Add (source.DbId); + foreach (var source in ServiceManager.Get <SaveTrackMetadataService> ().Sources) { + var db_ids = new List<int> (); That's just ugly, you are already embedding ids into the query in SelectCommand, why do it different here? So: string range = String.Join (",", db_ids); @@ +73,1 @@ ); Use @"" for multiline strings: CountCommand = new HyenaSqliteCommand (String.Format (@" SELECT COUNT(*) FROM CoreTracks WHERE DateUpdatedStamp > LastSyncedStamp AND PrimarySourceID IN ({0})", range)); @@ +75,3 @@ + range = "(" + string.Join (",", range_items) + ")"; + range_items [i] = db_ids [i].ToString (); + for (int i = 0; i < db_ids.Count; i++) No need for this anymore. @@ +78,2 @@ SelectCommand = DatabaseTrackInfo.Provider.CreateFetchCommand (String.Format ( + "DateUpdatedStamp > LastSyncedStamp AND PrimarySourceID IN {0}", range) It becomes: SelectCommand = DatabaseTrackInfo.Provider.CreateFetchCommand (String.Format ( "DateUpdatedStamp > LastSyncedStamp AND PrimarySourceID IN ({0})", range)); ::: src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataService.cs @@ +68,3 @@ + get { return sources.ToArray (); } + public PrimarySource [] Sources { + private List<PrimarySource> sources = new List<PrimarySource> (); ToArray() will create a new array on each call which is a bit excessive: private readonly List<PrimarySource> sources = new List<PrimarySource> (); public IEnumerable<PrimarySource> Sources { get { return sources.AsReadOnly (); } } @@ +82,3 @@ + if (p != null && p.CanEditMetadata) { + PrimarySource p = source as PrimarySource; + foreach (var source in ServiceManager.SourceManager.Sources) { You should also handle SourceManager.SourceAdded/SourceRemoved events to (a) attach/detach OnTracksChanged if it's a primary source and (b) update the sources list. ::: src/Core/Banshee.Services/Banshee.ServiceStack/ServiceManager.cs @@ +317,3 @@ + services.Add (service.ServiceName, service); + { + private static void Add (IService service) Why is it needed? ::: src/Libraries/Hyena/Hyena.Data.Sqlite/HyenaSqliteCommand.cs @@ +185,3 @@ + param_values [0] is Array) { + param_values.Length == 1 && + if (param_values != null && Not needed anymore?
Created attachment 154906 [details] [review] Patch v4 Thanks for the review! Here's the new patch with your comments addressed. And my comments inline: (In reply to comment #3) > Review of attachment 154757 [details] [review]: > > ::: src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataJob.cs > @@ +59,3 @@ > + db_ids.Add (source.DbId); > + foreach (var source in ServiceManager.Get > <SaveTrackMetadataService> ().Sources) { > + var db_ids = new List<int> (); > > Linq to the rescue: > > var db_ids = ServiceManager.Get <SaveTrackMetadataService> > ().Sources. > Select (s => s.DbId.ToString ()).ToArray (); Cool, added. > @@ +64,3 @@ > + db_ids.Add (source.DbId); > + foreach (var source in ServiceManager.Get > <SaveTrackMetadataService> ().Sources) { > + var db_ids = new List<int> (); > > That's just ugly, you are already embedding ids into the query in > SelectCommand, why do it different here? Because I thought using "?"-like parameters was more secure. > So: > string range = String.Join (",", db_ids); > Done. > @@ +73,1 @@ > ); > > Use @"" for multiline strings: > > CountCommand = new HyenaSqliteCommand (String.Format (@" > SELECT COUNT(*) FROM CoreTracks > WHERE DateUpdatedStamp > LastSyncedStamp > AND PrimarySourceID IN ({0})", range)); Done. > @@ +75,3 @@ > + range = "(" + string.Join (",", range_items) + ")"; > + range_items [i] = db_ids [i].ToString (); > + for (int i = 0; i < db_ids.Count; i++) > > No need for this anymore. Ok, removed. > @@ +78,2 @@ > SelectCommand = DatabaseTrackInfo.Provider.CreateFetchCommand > (String.Format ( > + "DateUpdatedStamp > LastSyncedStamp AND PrimarySourceID IN > {0}", range) > > It becomes: > > SelectCommand = DatabaseTrackInfo.Provider.CreateFetchCommand > (String.Format ( > "DateUpdatedStamp > LastSyncedStamp AND PrimarySourceID IN > ({0})", range)); Done. > ::: src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataService.cs > @@ +68,3 @@ > + get { return sources.ToArray (); } > + public PrimarySource [] Sources { > + private List<PrimarySource> sources = new List<PrimarySource> (); > > ToArray() will create a new array on each call which is a bit excessive: > > private readonly List<PrimarySource> sources = new List<PrimarySource> > (); > public IEnumerable<PrimarySource> Sources { > get { return sources.AsReadOnly (); } > } Done. > @@ +82,3 @@ > + if (p != null && p.CanEditMetadata) { > + PrimarySource p = source as PrimarySource; > + foreach (var source in ServiceManager.SourceManager.Sources) { > > You should also handle SourceManager.SourceAdded/SourceRemoved events to (a) > attach/detach OnTracksChanged if it's a primary source and (b) update the > sources list. Done. > ::: src/Core/Banshee.Services/Banshee.ServiceStack/ServiceManager.cs > @@ +317,3 @@ > + services.Add (service.ServiceName, service); > + { > + private static void Add (IService service) > > Why is it needed? Because I'm using "ServiceManager.Get <SaveTrackMetadataService> ()" instead of "ServiceManager.Get <SaveTrackMetadataService> (serviceName)" (which I think is redundant and forces me to put the serviceName which is a string and thus not safely typed. Therefore, you need to add the services to the hashtable as their service type names as well, otherwise Get<T>() cannot work (I've explained it in a pair of comments). > ::: src/Libraries/Hyena/Hyena.Data.Sqlite/HyenaSqliteCommand.cs > @@ +185,3 @@ > + param_values [0] is Array) { > + param_values.Length == 1 && > + if (param_values != null && > > Not needed anymore? Right, but maybe it would still be useful for anyone in the future. However, following YAGNI I've removed from the patch for now.
s/typed./typed)./ s/removed from/removed it from/
Review of attachment 154906 [details] [review]: I have a problem with this. Do I understand correctly that when the "Save ratings and playcounts" option is switched on, the metadata of tracks in FSQ will be updated on each play? If so, this is a really bad thing to happen. FSQ is supposed to be a preview of the tracks, they should not be touched unless explicitly told. For example, when downloading music with bittorrent, modifying the shared files in any way is a big no-no. A few nitpicks: ::: src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataJob.cs @@ +58,3 @@ + + Select (s => s.DbId.ToString ()).ToArray (); + var db_ids = ServiceManager.Get<SaveTrackMetadataService> ().Sources. As I mentioned in the previous review, you can avoid concatenation here if you replace '{0}' with '({0})' in two queries below. Just looks a bit cleaner to me. @@ +90,3 @@ } + //not valid for tracks in FSQ: + if (RenameEnabled && In the future we could have sources with CanEditMetadata=true other than FSQ. Change the comment to: // Rename tracks only from the Music Library ...and put it before the 'if' to make it look less cluttered. ::: src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataService.cs @@ +103,3 @@ + PrimarySource p = s as PrimarySource; + { + private void AddPrimarySource (Source s) Why no `&& CanEditMetadata` here? Alternatively, you can do: if (p != null && sources.Remove (p)) { p.TracksChanged -= OnTracksChanged; } ::: src/Core/Banshee.Services/Banshee.ServiceStack/ServiceManager.cs @@ +315,3 @@ + services.Add (service.ServiceName, service); + { + private static void Add (IService service) I suggest that you keep this class unchanged. If you want to refactor it, please do it in another bug/patch. For now, either of two approaches should work for this patch: * Make SaveTrackMetadataService.ServiceName consistent with the type name ("SaveMetadataService" vs. "SaveTrackMetadataService") * Explicitly specify the service name when calling ServiceManager.Get()
(In reply to comment #7) > Review of attachment 154906 [details] [review]: > > I have a problem with this. Do I understand correctly that when the "Save > ratings and playcounts" option is switched on, the metadata of tracks in FSQ > will be updated on each play? > > If so, this is a really bad thing to happen. FSQ is supposed to be a preview of > the tracks, they should not be touched unless explicitly told. For example, > when downloading music with bittorrent, modifying the shared files in any way > is a big no-no. Ok, to fix that we need the capability that would be brought by the fix to the bug 602159, as we agreed on IRC.
(In reply to comment #8) > Ok, to fix that we need the capability that would be brought by the fix to the > bug 602159, as we agreed on IRC. Andrés, in case you missed my message on IRC, this piece of code should handle it: http://pastebin.com/A75Vc22K Bug 602159 is not a prerequisite for this, I might want to save both ratings and playcounts when listening to tracks from the music library, but I don't want my files to be touched when previewing them in FSQ.
(In reply to comment #9) > (In reply to comment #8) > > Ok, to fix that we need the capability that would be brought by the fix to the > > bug 602159, as we agreed on IRC. > > Andrés, in case you missed my message on IRC, this piece of code should handle > it: http://pastebin.com/A75Vc22K > > Bug 602159 is not a prerequisite for this, I might want to save both ratings > and playcounts when listening to tracks from the music library, but I don't > want my files to be touched when previewing them in FSQ. Oh ok! Yes I missed that msg on IRC. This is very helpful (and indeed, much better as you don't loose the PlayCount, but just postpone its writing). I'll work on a new version of the patch that includes this then. However, as I may not be quick enough to fix bug 589196 before 1.6 (which is similar to this one), can we get first the patch of bug 611923 committed, as a good temporary workaround for all these kind of bugs? Thanks for your reviews and your support!
Created attachment 161899 [details] [review] Patch v5 (In reply to comment #7) > (...) > A few nitpicks: > > ::: src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataJob.cs > @@ +58,3 @@ > + > + Select (s => s.DbId.ToString ()).ToArray (); > + var db_ids = ServiceManager.Get<SaveTrackMetadataService> > ().Sources. > > As I mentioned in the previous review, you can avoid concatenation here if you > replace '{0}' with '({0})' in two queries below. Just looks a bit cleaner to > me. Done. > @@ +90,3 @@ > } > + //not valid for tracks in FSQ: > + if (RenameEnabled && > > In the future we could have sources with CanEditMetadata=true other than FSQ. > Change the comment to: > > // Rename tracks only from the Music Library > > ...and put it before the 'if' to make it look less cluttered. Done. > ::: src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataService.cs > @@ +103,3 @@ > + PrimarySource p = s as PrimarySource; > + { > + private void AddPrimarySource (Source s) > > Why no `&& CanEditMetadata` here? Alternatively, you can do: > > if (p != null && sources.Remove (p)) { > p.TracksChanged -= OnTracksChanged; > } Done. > ::: src/Core/Banshee.Services/Banshee.ServiceStack/ServiceManager.cs > @@ +315,3 @@ > + services.Add (service.ServiceName, service); > + { > + private static void Add (IService service) > > I suggest that you keep this class unchanged. If you want to refactor it, > please do it in another bug/patch. > > For now, either of two approaches should work for this patch: > > * Make SaveTrackMetadataService.ServiceName consistent with the type name > ("SaveMetadataService" vs. "SaveTrackMetadataService") Done. > * Explicitly specify the service name when calling ServiceManager.Get() (In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > Ok, to fix that we need the capability that would be brought by the fix to the > > > bug 602159, as we agreed on IRC. > > > > Andrés, in case you missed my message on IRC, this piece of code should handle > > it: http://pastebin.com/A75Vc22K > > > > Bug 602159 is not a prerequisite for this, I might want to save both ratings > > and playcounts when listening to tracks from the music library, but I don't > > want my files to be touched when previewing them in FSQ. > > Oh ok! Yes I missed that msg on IRC. > > This is very helpful (and indeed, much better as you don't loose the PlayCount, > but just postpone its writing). > > I'll work on a new version of the patch that includes this then. Done.
Review of attachment 161899 [details] [review]: Looks good, please commit after fixing a nitpick below: ::: src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataJob.cs @@ +60,3 @@ + + Select (s => s.DbId.ToString ()).ToArray (); + var db_ids = ServiceManager.Get<SaveTrackMetadataService> ().Sources. Remove the space before `String.Format`
Comment on attachment 161899 [details] [review] Patch v5 Committed! (noting attribution to you about DatabaseTrackInfo.cs :) )
Marking FIXED (1.7.2 will contain the fix). Thanks for the review!