GNOME Bugzilla – Bug 589196
[PATCH] Edited metadata on DAP tracks not saved
Last modified: 2020-03-17 08:24:59 UTC
Steps to reproduce: 1. Have some tracks in your iPod (manual sync mode). 2. Open ipod library. 3. Click on the rating of a song, to modify it. 4. Disconnect the iPod. Current results: The song doesn't have the new rating on the iPod. Expected results: The new rating should have been saved.
Created attachment 138871 [details] [review] Saves the rating when in manual mode The patch is quite simple.
Thanks! Following up to your comment #22 in bug 389550: I will test this on 1.5.0 tomorrow and let you know. Will this also fix the other issues documented in comment #3 in bug 580632 -- namely that Compilation Artist (and presumably several other fields) don't get written to the iPod when modified in manual mode? (The rating was just one of the fields that had this problem). To reiterate, the problem is with a manually-managed iPod, when I edit the iPod track in banshee. Thanks! Feels like we're really close now! -- John
(In reply to comment #2) > Thanks! > > Following up to your comment #22 in bug 389550: > > I will test this on 1.5.0 tomorrow and let you know. Beware, if you want the patch to be guaranteed for applying without errors, use always upstream, I mean, git master. All developers use git master (trunk when we used subversion) for any development. If the patch applies to 1.5.0 it's just a matter of luck. > Will this also fix the other issues documented in comment #3 in bug 580632 -- > namely that Compilation Artist (and presumably several other fields) don't get > written to the iPod when modified in manual mode? (The rating was just one of > the fields that had this problem). Mmmm, I guess not, I will take a look at that when these patches begin to get committed/reviewed. I'm really excited about the progress we're doing here.
Good news - it applied cleanly over 1.5.0 and does seem to fix the rating issue. Thanks!
Thanks for testing jgoerzen. I think we'll get a review of the patch next week.
Can any maintainer/developer review the patch? Thanks.
Comment on attachment 138871 [details] [review] Saves the rating when in manual mode This patch won't work when the user sets the rating for many songs at once. I don't like adding the TrackInfo argument to the TracksChanged, either - seems very hacky, and all to half-solve a pretty minor issue. I think ratings should be synced to (and from) DAPs, along with playcount info, ideally in a DAP-generic way, using the database/queries/MetadataHash/LastUpdatedStamp/etc values to do it fast/efficiently.
Additionally, I think this is a bit of a silly issue, since if you change *any* field, not just rating, in your Library it is not synced. Why should the rating get synced but not the artist name, etc?
(In reply to comment #8) > Additionally, I think this is a bit of a silly issue, since if you change *any* > field, not just rating, in your Library it is not synced. Why should the > rating get synced but not the artist name, etc? It's not "in your Library", but on the iPod source. That being said, I'm not sure how cleanly the comment#7 applies to the patch :) (for instance, because MetadataHash is used on auto-sync, not manual sync). But I agree it's a bit hacky so I'll look at another approach. Thanks for reviewing!
(In reply to comment #9) > (In reply to comment #8) > > Additionally, I think this is a bit of a silly issue, since if you change *any* > > field, not just rating, in your Library it is not synced. Why should the > > rating get synced but not the artist name, etc? > > It's not "in your Library", but on the iPod source. In case this is still not clear: re-read step 2 of comment#0 :)
Bulk changing the assignee to banshee-maint@gnome.bugs to make it easier for people to get updated on all banshee bugs by following that address. It's usually quite apparent who is working on a given bug by the comments and/or patches attached.
(In reply to comment #7) > (From update of attachment 138871 [details] [review]) > This patch won't work when the user sets the rating for many songs at once. I > don't like adding the TrackInfo argument to the TracksChanged, either - seems > very hacky, and all to half-solve a pretty minor issue. > > I think ratings should be synced to (and from) DAPs, along with playcount info, > ideally in a DAP-generic way, using the > database/queries/MetadataHash/LastUpdatedStamp/etc values to do it > fast/efficiently. After proposing a patch for bug 609411, I see what you meant here, so disregard my last comments. As I'll do a similar approach to that patch, I'm marking that bug as dependent to this one.
(In reply to comment #12) > As I'll do a similar approach to that patch, I'm marking that bug as dependent > to this one. I meant *this bug* dependent on that bug, as it's possible I'll reuse some of the infrastructure bits from that patch.
Created attachment 162366 [details] [review] Patch v2 New approach.
The new patch takes care of any metadata, not only rating, so I'm updating the summary.
*** Bug 620181 has been marked as a duplicate of this bug. ***
*** Bug 559416 has been marked as a duplicate of this bug. ***
Review of attachment 162366 [details] [review]: I don't have an Ipod to test that part. For a mass storage device, the patch doesn't change anything, it's not possible to edit metadata (including rating) with or without the patch. ::: src/Dap/Banshee.Dap.Ipod/Banshee.Dap.Ipod/IpodTrackInfo.cs @@ +112,3 @@ ReleaseDate = podcast_info.ReleaseDate; + throw new ArgumentException ("Shouldn't update an IpodTrackInfo from an IpodTrackInfo"); + } else { It would look more clear if you revert the check. Put `if (track is IpodTrackInfo) ...` at the beginning of the method and get rid of the `else`.
(In reply to comment #18)> > For a mass storage device, the patch doesn't change anything It is slightly confusing to see Component-Ipod in this ticket, because for example I need this for Android.
(In reply to comment #18) > Review of attachment 162366 [details] [review]: > > I don't have an Ipod to test that part. For a mass storage device, the patch > doesn't change anything, it's not possible to edit metadata (including rating) > with or without the patch. Yes sorry, I plan to cover more devices in later patches. The first one will take care of Ipod+ManualSync. > ::: src/Dap/Banshee.Dap.Ipod/Banshee.Dap.Ipod/IpodTrackInfo.cs > @@ +112,3 @@ > ReleaseDate = podcast_info.ReleaseDate; > + throw new ArgumentException ("Shouldn't update an > IpodTrackInfo from an IpodTrackInfo"); > + } else { > > It would look more clear if you revert the check. Put `if (track is > IpodTrackInfo) ...` at the beginning of the method and get rid of the `else`. I know :) But I did it this way because otherwise the patch would be much cluttered (to move a bunch of lines to a lower level of identation). So, to see more clearly what this patch does, I propose to commit this as is, and then refactor later in a 2nd patch with the code flow you advice. (In reply to comment #19) > (In reply to comment #18)> > > For a mass storage device, the patch doesn't change anything > > It is slightly confusing to see Component-Ipod in this ticket, because for > example I need this for Android. Yes because there's no "Device - Dap" generic component :) And I plan to fix iPod support first.
Comment on attachment 162366 [details] [review] Patch v2 Marking as needs-work anyway because there are still some things I'm not convinced about in this patch.
I tested the patch but I'm afraid to tell you, that it didn't work out. Steps I followed: 1. Cloned banshee.git (* master 64f4fdd) 2. Applied the attached patch (attachment 162366 [details] [review]) 3. Three-Jump ./autogen.sh && make && make run 4. Connected iPod (successfully detected and all neat stuff) and switched music to manual mode 5. Changed a rating of an already rated track 6. Ejected iPod: Old rating was there. 7. Connected again 8. Changed a rating of an not-yet-rated track 9. Synchronized (!) with manual mode on! 10. Ejected iPod: No rating was there. 11. Connected again 12. Changed a rating of an not-yet-rated track 14. Ejected iPod: No rating was there. I made another test: 1. Rate an unrated track via the iPod 2. Connect the iPod, Banshee shows the new rating in the iPod library 3. Full sync with iPod 4. No rating on Music Library, but still on iPod library and iPod. Hope my tests helped...
(In reply to comment #22) > I tested the patch but I'm afraid to tell you, that it didn't work out. Thanks for your efforts but the patch of this bug is marked as "needs-work" because it's not ready yet. I marked bug 620826 as a dependency because somehow it affects this one and I want that one to be landed before. Let's fix that bug and then I'll create a new patch here. Thanks again!
Hi! I just would like to know, if there is any progress on this bug. Is there a new patch to be tested? Greetings, Philip
Hey Philip! Thanks for your interest. I'm actually working on it, but finding some difficulties to make the perfect solution, and having some trouble testing some scenarios... Maybe I can drop you an email soon so we can help each other. Stay tuned.
While working on this (and thanks to the help of Philip for providing me with some material to test) I found bug 626113 which may not actually be a blocker for this bug but at least is very related. Help on determining the expected results of that bug and if it affects other DAPs different than iPod, is appreciated.
Created attachment 167249 [details] [review] Infrastructure to make metadata resync to DAP possible for Manual Sync mode There you go, I'm finally happy with this solution. I've decided to split this work in smaller patches so they are easier to review. Next patch I'm posting is the support for iPod re-sync that uses the infrastructure done in this first patch.
Created attachment 167250 [details] [review] Metadata resync to DAP possible for Manual Sync mode: iPod support With the previous patch and this one combined together, iPod has gained support for editing metadata in manual mode. Please test! And review. Next thing I'm gonna work on is Android. After Android, patches to other DAP devices are welcome. After that, I'll work on patches for the AutoSync mode.
Hi Andrés! Thanks for your patches, I will test them asap. Is it possible to test them separately? I don't really understand, what the first patch from comment #27 will do. Thanks in advance! Greetings, Philip
(In reply to comment #29) > Hi Andrés! > > Thanks for your patches, I will test them asap. Thank for your help! > Is it possible to test them separately? I don't really understand, what the > first patch from comment #27 will do. Let me try to explain. First patch (let's call it "A") is the base/common support for this feature for every device. The second patch (let's call it "B") adds the iPod layer. I'm now working on the Android layer (let's call it "C"). So if you wanted to test this on your iPod you would need to apply A & B. If you wanted to test this on your Android you would need to apply A & C. I guess it's clear now? I could have merged A & B in just one patch but it's better this way for reviewing purposes and to separate them logically in different commits (for better log history).
Review of attachment 167249 [details] [review]: Can you attach patches formatted with `git format-patch` from now on? Having a nice summary commit message is useful. ::: src/Dap/Banshee.Dap/Banshee.Dap/SyncPlaylist.cs @@ +24,3 @@ +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. +using System; Add newline above this ::: src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataJob.cs @@ +62,2 @@ string condition = String.Format ( + @"(DateUpdatedStamp > LastSyncedStamp OR Is this change related to the rest of this patch? ::: src/Core/Banshee.Services/Banshee.Sources/PrimarySource.cs @@ +317,3 @@ } + public virtual void UpdateMetadata (DatabaseTrackInfo track) Could we put this in the DapSource class instead of here? PrimarySource is used all over, where this UpdateMetadata call/method isn't.
Review of attachment 167250 [details] [review]: ::: src/Dap/Banshee.Dap.Ipod/Banshee.Dap.Ipod/IpodSource.cs @@ +411,3 @@ + + ipod_track.UpdateInfo (track); + ipod_track.CommitToIpod (IpodDevice); Our current sync only does this CommitToIpod in the sync thread -- it probably shouldn't happen directly here, synchronously. Should instead add a tracks_to_update list, and perform the update in the PerformSyncThreadCycle method. @@ +706,3 @@ + get { + // we want child sources to be able to edit metadata and the + // savetrackmetadataservice to take in account this source what do you mean have the savetrackmetadataservice take this source into account? It's going to save metadata to file somehow or something?
I don't think I have the energy to put much more time into this, since we're hopefully soon to replace this entire extension with the libgpod based one.
Created attachment 167575 [details] [review] (v2) Infrastructure to make metadata resync to DAP possible for Manual Sync mode Thanks for the reviews! New patch attached. See inline: (In reply to comment #31) > Review of attachment 167249 [details] [review]: > > Can you attach patches formatted with `git format-patch` from now on? Having a > nice summary commit message is useful. Sure. BTW, can you add that guideline to: http://banshee.fm/contribute/write-code/ Or at least a link to a doc that says how to do it (because I always forget the argument comes after 'format-patch') like: http://live.gnome.org/Git/Developers > > ::: src/Dap/Banshee.Dap/Banshee.Dap/SyncPlaylist.cs > @@ +24,3 @@ > +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > +// THE SOFTWARE. > +using System; > > Add newline above this Done. > ::: src/Core/Banshee.Services/Banshee.Metadata/SaveTrackMetadataJob.cs > @@ +62,2 @@ > string condition = String.Format ( > + @"(DateUpdatedStamp > LastSyncedStamp OR > > Is this change related to the rest of this patch? Yes. Without this change, even having SaveTrackMetadataJob take in account the DapSource, changes woulnd't be flushed through TagLib# because the tracks in a DapSource have never been synced before, so they have NULL in their values. > ::: src/Core/Banshee.Services/Banshee.Sources/PrimarySource.cs > @@ +317,3 @@ > } > > + public virtual void UpdateMetadata (DatabaseTrackInfo track) > > Could we put this in the DapSource class instead of here? PrimarySource is > used all over, where this UpdateMetadata call/method isn't. I thought about this too, but I can't because UpdateMetadata(_) is called in DatabaseTrackInfo, which cannot reference Dap project, because Dap is an extension. (In reply to comment #33) > I don't think I have the energy to put much more time into this, since we're > hopefully soon to replace this entire extension with the libgpod based one. I guess this applies only to the iPodSource patch, not the rest. Remember I'm implementing Android support now (been a bit slow because I'm having troubles with syncing, hopefully I can fix them too, I guess I'm hitting edge cases), and of course I will look into implementing this also in the AppleDevice addin when it lands, because at some point I want to start using it too.
Created attachment 167645 [details] [review] (v3) Infrastructure to make metadata resync to DAP possible for Manual Sync mode v2 was missing a small bit from v1
Created attachment 167653 [details] [review] (v2) Metadata resync to DAP possible for Manual Sync mode: iPod support (In reply to comment #32) > Review of attachment 167250 [details] [review]: > > ::: src/Dap/Banshee.Dap.Ipod/Banshee.Dap.Ipod/IpodSource.cs > @@ +411,3 @@ > + > + ipod_track.UpdateInfo (track); > + ipod_track.CommitToIpod (IpodDevice); > > Our current sync only does this CommitToIpod in the sync thread -- it probably > shouldn't happen directly here, synchronously. Should instead add a > tracks_to_update list, and perform the update in the PerformSyncThreadCycle > method. Oh, true, I actually did this for another old patch (bug ) but forgot now! I have fixed it in this new patch. Thanks! > @@ +706,3 @@ > + get { > + // we want child sources to be able to edit metadata and the > + // savetrackmetadataservice to take in account this source > > what do you mean have the savetrackmetadataservice take this source into > account? It's going to save metadata to file somehow or something? Yes, it flushes the changes to the file too. Look at the longer description of the commit in this new patch for a better explanation.
(In reply to comment #36) > > Oh, true, I actually did this for another old patch (bug ) but forgot now!... Forgot bug number :) It's bug 389550.
Created attachment 167716 [details] [review] (v1) Metadata resync to DAP possible for Manual Sync mode: AppleDevice support So I checked out the gio-hardware branch today and found out that the AppleDevice extension (libgpod based) is quite similar to the iPod's one, so I just ported my changes to this addin, and here's the diff. Hopefully it can work out of the box, but I couldn't test it (I couldn't even compile it, because I don't meet the dependencies!). So testing is welcome!
Created attachment 167813 [details] [review] (v1) Metadata resync to DAP possible for Manual Sync mode: AppleDevice support (fixed compilation) (v1) Metadata resync to DAP possible for Manual Sync mode: AppleDevice support (fixed compilation)
Just tested these patches with an iPod classic and the AppleDevice extension (gio-hardware) branch. Worked!
Hi Andrés! Could provide some information how to test this patch/these patches? I would like to test them. Greetings, Philip
When a track has been added to the iPod, but before the sync occurs, the URI of the DatabaseTrackInfo under the DAP points to the in-library URI. Given that, I think we need to think carefully about the SaveTrackMetadataJob. The user could edit the track on the iPod, but it would end up writing the changes to the file inside their library. Besides that issue, these patches look pretty good to me. It doesn't account for rating changes triggered via the Edit or context menus or via the ListView, but that's probably OK for now - we can work on that in a separate bug/patch.
Thanks for your comments Gabriel. (In reply to comment #42) > When a track has been added to the iPod, but before the sync occurs, the URI of > the DatabaseTrackInfo under the DAP points to the in-library URI. Given that, Oh, this is why this bug was depending on bug 620826, which I fixed for the iPod extension. Can you adapt that patch for the AppleDevice extension? Or else I can do it myself as soon as I get the dependencies needed for the extension to work on my system (I guess it's easier that I wait for the packages of the next release...). > I think we need to think carefully about the SaveTrackMetadataJob. The user > could edit the track on the iPod, but it would end up writing the changes to > the file inside their library. > > Besides that issue, these patches look pretty good to me. It doesn't account > for rating changes triggered via the Edit or context menus or via the ListView, > but that's probably OK for now - we can work on that in a separate bug/patch. Are you sure? I fixed the enabled/disabled state of that in this commit:http://mail.gnome.org/archives/commits-list/2010-May/msg09689.html Shouldn't that also trigger a Save() operation and thus an UpdateMetadata() call? Haven't tested it though.
(In reply to comment #41) > Hi Andrés! > > Could provide some information how to test this patch/these patches? > I would like to test them. > > Greetings, Philip Hey Philip! Very simple, as always, just do what you see in comment#0! Although I guess this time is not so much needed because Alex Launi already tested the AppleDevice patch and it worked (and I have tested the iPod patch quite a lot).
(In reply to comment #43) > Thanks for your comments Gabriel. > > (In reply to comment #42) > > When a track has been added to the iPod, but before the sync occurs, the URI of > > the DatabaseTrackInfo under the DAP points to the in-library URI. Given that, > > Oh, this is why this bug was depending on bug 620826, which I fixed for the > iPod extension. OK, maybe it's not as likely to happen as I thought. But I don't think you've fixed it in that patch, which AFAICT only modifies the IpodTrackInfo.CommitToIpod method. So there is still a window of time 1) after the user adds the track to the device and 2) before it's been CommitToIpod'd where the user could edit it. I'm not sure how to fix this though -- I had an idea about checking if the updated track was in the tracks_to_add queue, but that won't suffice I don't think, since the update/save happens regardless, and there's no way to say some tracks are readonly and others aren't.
Created attachment 170239 [details] [review] (v3) Metadata resync to DAP possible for Manual Sync mode: iPod support Added an additional change to SyncNeeded property that I realised when writing the AppleDevice patch (so that patch already includes it).
(In reply to comment #42) > Besides that issue, these patches look pretty good to me. Since Gabriel brought up that issue, bug 620826 was reopened and it has just been closed today after committing 2 patches, one for each extension. So given that issue is not with us anymore, I'll assume "pretty good" as a sign-off and I'll start committing these 3 patches. > It doesn't account > for rating changes triggered via the Edit or context menus or via the ListView, > but that's probably OK for now - we can work on that in a separate bug/patch. I'll work on this, and on Android support, later on (hopefully I will have it ready before the 1.8.0 release).
Created attachment 177103 [details] [review] (v1) Metadata resync to DAP possible for Manual Sync mode: Android support Christmas gift for Android users. If you want to test with a MassStorageDevice instead of Android, you just have to change the line with: s/ms_device is Android/true/ Feedback welcome! Thanks.
Created attachment 177264 [details] [review] (v2) Metadata resync to DAP possible for Manual Sync mode: Android support Improved a bit the last patch so we are less verbose in MassStorageSource.cs and more verbose in the new helper subtype M3uElement, removing some code duplication too.
Created attachment 177266 [details] [review] (v3) Metadata resync to DAP possible for Manual Sync mode: Android support Oops, fixed some mistakes: style and namespace issues in the last patch.
Hello, I am also interested in having a fix for this bug and (also for bug 389550). I see that no new comment has been added since a month. Do you need any help for testing ? (I have an ipod but no android device) Thanks. Christophe
Review of attachment 177266 [details] [review]: 1) Why only support Android devices, why not all MassStorge devices? 2) I'm very concerned about the playlist rewriting logic b/c it A) gets called/rewritten every time a single track is changed, and iterates over every playlist and every track in it B) loads the entire playlist into memory w/ the List<M3uElement> usage Why not change (2) to just flag the playlists as dirty, and before unmount/eject write them out again?
Thanks for the review. (In reply to comment #52) > Review of attachment 177266 [details] [review]: > > 1) Why only support Android devices, why not all MassStorge devices? Well, because I haven't tested with MassStorage devices. I actually already asked for testers in comment#48 when I said "If you want to test with a MassStorageDevice instead of Android, you just have to change the line with: s/ms_device is Android/true/" > 2) I'm very concerned about the playlist rewriting logic b/c it > A) gets called/rewritten every time a single track is changed, and iterates > over every playlist and every track in it > B) loads the entire playlist into memory w/ the List<M3uElement> usage > Why not change (2) to just flag the playlists as dirty, and before > unmount/eject write them out again? I was a bit concerned by this too, and I remember at the time I wrote the patch that I wanted to do the playlist update in just one go after all files have been updated, at the end of the sync cycle (instead of when ejecting the device, just in case the user forgets to do this), however when I tried to find the equivalent of the function IPodSource.PerformSyncThreadCycle(), to put it at the end of such a function, I realised the sync process was completely different and I recall I thought it was a big change. Sadly, four months after I wrote this patch, I don't remember much about these details so I cannot post an opinionated reply. And unfortunately too, in this 4 months my Android device happened to break, so I cannot work on this patch anymore. I'm happy if anyone continues my work though (not sure when I'll get a new phone, and not sure yet if it will be an Android or not).
Created attachment 204306 [details] [review] (v4) Metadata resync to DAP possible for Manual Sync mode: MassStorage support (In reply to comment #53) > I was a bit concerned by this too, and I remember at the time I wrote the patch > that I wanted to do the playlist update in just one go after all files have > been updated, at the end of the sync cycle (instead of when ejecting the > device, just in case the user forgets to do this), however when I tried to find > the equivalent of the function IPodSource.PerformSyncThreadCycle(), to put it > at the end of such a function, I realised the sync process was completely > different and I recall I thought it was a big change. I was kind of wrong here. I mean, what I said is not false but, thing is, the sync operation for MassStorage devices works a bit differently than on the AppleDevice extension. It turns out that the latter has a way of knowing when the transfer of files was completed, to then be able to sync playlists. This doesn't happen in Dap.MassStorage extension, so a user needs to hit the "Disconnect" (Eject) option on the device for the playlists to be written in that moment. This is problematic because: a) A user may quit Banshee expecting his changes to playlists to be synced to the devices (already reported in bug 666696 and proposed a patch). b) A user may accidentally remove the device without disconnecting it via Banshee, which would cause the data loss. (I still need to open a bug for this) However, as Banshee currently works with problem (b), I've decided to write v2 of this patch taking this into account (baby steps, different patches for different bugs). So the metadata changes to playlists will take in place when the user hits the Disconnect command in the context menu. And this way Gabriel's concern is addressed. (I also addressed his 1st concern because I now managed to test with a non-Android device, and it works.)
s/v2/v4/
Small typo in the commit msg, it should read "This is to avoid that the sync operation rewrites each playlist once per track (in case of once per playlist)." I'd correct this before pushing.
Review of attachment 204306 [details] [review]: Looks good.
Comment on attachment 204306 [details] [review] (v4) Metadata resync to DAP possible for Manual Sync mode: MassStorage support (In reply to comment #57) > Review of attachment 204306 [details] [review]: > > Looks good. Thanks, committed. In the following weeks I'll concentrate on creating patches for automatic sync mode. And then we will be able to close this bug I guess.
I tried banshee master, and when I change the rating in a song (Ipod), it's still not applied in the Ipod. (Ipod Nano 3g). The bug report was about the Ipod, so I'm just curious if it should be fixed or not.
Banshee is not under active development anymore and had its last code changes more than three years ago. Its codebase has been archived. Closing this report as WONTFIX as part of Bugzilla Housekeeping to reflect reality. Please feel free to reopen this ticket (or rather transfer the project to GNOME Gitlab, as GNOME Bugzilla is being shut down) if anyone takes the responsibility for active development again. See https://gitlab.gnome.org/Infrastructure/Infrastructure/issues/264 for more info.