GNOME Bugzilla – Bug 620826
URIs of synced files are not consistent
Last modified: 2010-09-19 11:35:13 UTC
Here's the repro: 01. Turn on auto-sync on iPod. 02. Have an audio file in /home/user/Music (let's say X.mp3). 03. Connect the iPod: result: X.mp3 is transferred to it. 04. Eject the iPod. 05. Add an Y.mp3 file to /home/user/Music. 06. Import media again to have the 2 files in the library. 07. Connect the iPod: result: Y.mp3 is transferred to it. 08. Open the iPod source, Music sub-source. 09. Right-click on X -> open containing folder. 10. Result: a folder in the iPod folder tree will open. 11. Right-click on Y -> open containing folder. Current results: The folder /home/user/Music will open. Expected results: For consistency, a similar folder as the one opened in step 10, should open (that is, inside iPod's file subtree structure). I have a patch in the works, will attach it ASAP.
Created attachment 163313 [details] [review] Patch v1 This fixes the problem.
Created attachment 164360 [details] [review] Patch v2 Gabriel told me to add a comment that explained a bit more the reason of the patch, and I'm adding it here. As a final note, what the patch does is basically two things: - Change the call to CreateTrack() by a call to CreateTrack(uri), because for the former parameterless method it's not clear that the assignment of the Uri property does the real copy-file operation and that its value changes after this happens. With CreateTrack(uri), the copy-file operation happens and the Uri property gets the location of where the file was copied to. - After the creation of the track, we save the Uri to banshee db (this happens only when the track was not already on the iPod).
Created attachment 164363 [details] [review] Same patch, but created with `git format-patch HEAD~1`
I tested the patch and it works as specified. iPod classic 120 GB Steps I followed: 1. Cloned banshee.git (* master 64f4fdd) 2. Applied the attached patch (attachment 164363 [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 not-yet-rated track on iPod library 6. Synchronized (!) with manual mode on 7. Ejected iPod: Rating is there. 8. Changed the rating on the iPod 11. Connected again 12. In the iPod library in Banshee, the new rating is there! 13. Synchronized (with manual and complete mode) 14. Rating is not in my Banshee library :( I hope these tests help!
(In reply to comment #4) > I tested the patch and it works as specified. I guess it worked because you applied both patches, this one and the one from bug 589196. But this is not what I intended to test. > Steps I followed: Thanks for being so verbose on what you did, but unfortunately I wasn't asking for this. You have to take in account that this bug is not related at all with Ratings or metadata synchronization (the only reason why bug 589196 depends on this one is because the fix for that bug depends on the fix for this one, nothing more). So, to properly test this, could you try again but now with these steps: a. Clone banshee git. b. Apply the patch in this bug. c. autogen and make and make run d. Do *all the steps* that you can find in comment#0 (from 01 to 11). e. Tell us if you get the "expected results" or the "current results" that are mentioned in comment#0. Thanks again!
Oh, if you don't do step (a) but use your already-cloned tree, make sure it's clean of patches by doing "git reset --hard".
After some problems with my git repo, I can now assume that Andrés' patch works. I synchronized my iPod with a new file (Binärpilot - Fuayfsilfm) in my Banshee library and if I let Banshee open the containing folder from the iPod library, it opens the correct folder /home/philip/Musik/Binärpilot/Defrag/ where the file "04. Fuayfsilfm.mp3" is shown. I hope this patch can be forwarded to master right on time. Andrés: Thanks for your support!
(In reply to comment #7) > After some problems with my git repo, I can now assume that Andrés' patch > works. Thanks for your testing. However, given the details you gave, I don't think you tested this correctly, but we are getting there!! > I synchronized my iPod with a new file (Binärpilot - Fuayfsilfm) in my Banshee > library and if I let Banshee open the containing folder from the iPod library, > it opens the correct folder /home/philip/Musik/Binärpilot/Defrag/ where the > file "04. Fuayfsilfm.mp3" is shown. Please read comment#0 again and pay attention carefully: showing a folder in /home is actually the wrong behaviour (current behaviour) instead of the expected behaviour after having applied the patch. Are you sure that you applied my patch? Thanks again!
Yes, I applied the patch via "git apply <patchname>" but I forgot to run autogen.sh again. I did this and after all steps in comment#0, the folder /media/IPOD/iPod_Control/Music/F07 opened. The new file is in the opened folder. I hope it helps.
Review of attachment 164363 [details] [review]: Looks good to me, please commit.
Comment on attachment 164363 [details] [review] Same patch, but created with `git format-patch HEAD~1` Thanks for the review!
Marking as fixed, thanks for all contributions (testing and reviewing).
Review of attachment 164363 [details] [review]: ::: src/Dap/Banshee.Dap.Ipod/Banshee.Dap.Ipod/IpodTrackInfo.cs @@ +208,3 @@ + if (!update) { + Uri = new SafeUri (track.Uri); + Save (); This Save () call worries me hugely. Assuming DatabaseTrackInfo.NotifySaved is true (which it is by default) then while the iPod sync is happening, you're triggering a model reload/view refresh for *every* song being added, which can be very expensive CPU/db wise.
Reopening because of the following reasons: * Issue found by Gabriel in the committed patch (comment#13). * Some issues I found myself too wrt progress reporting (more on that later, see message of the commit of the patch to be proposed). * Because we need a fix too for AppleDevice as a dependency for bug 589196. * Because, as commented on bug 589196 comment 45, the approach is not the best because there is still a window of time in which the URI is not correct. (In reply to comment #13) > Review of attachment 164363 [details] [review]: > > ::: src/Dap/Banshee.Dap.Ipod/Banshee.Dap.Ipod/IpodTrackInfo.cs > @@ +208,3 @@ > + if (!update) { > + Uri = new SafeUri (track.Uri); > + Save (); > > This Save () call worries me hugely. Assuming DatabaseTrackInfo.NotifySaved is > true (which it is by default) then while the iPod sync is happening, you're > triggering a model reload/view refresh for *every* song being added, which can > be very expensive CPU/db wise. I see, but if I use Save(false) the URI doesn't get updated in the UI. How can I trigger a unique redraw for when the sync has finished (so as to do it once instead of n, being n the number of tracks to sync)?
Created attachment 169817 [details] [review] New approach for the iPod extension I'm proposing this fix to the previous patch, which mainly fixes the previous concerns. However, this time I couldn't call Save(false) because with false instead of true I didn't get the songs added in the source graphically. How can this happen in the Adding process if the call used is Save(false) instead of Save(true)? (In reply to comment #14) > Reopening because of the following reasons: > * Issue found by Gabriel in the committed patch (comment#13). Not sure if now it's better, at least I've removed one call to Save() which may improve things. See the description of the patch. > * Some issues I found myself too wrt progress reporting (more on that later, > see message of the commit of the patch to be proposed). I explained this in the commit. > * Because we need a fix too for AppleDevice as a dependency for bug 589196. If the approach is accepted, I'll think of another patch for AppleDevice. > * Because, as commented on bug 589196 comment 45, the approach is not the best > because there is still a window of time in which the URI is not correct. This is fixed in this patch.
Created attachment 169819 [details] [review] New approach for the iPod extension, v2 Proposing new patch! (In reply to comment #15) > However, this time I couldn't call Save(false) because with false instead of > true I didn't get the songs added in the source graphically. How can this > happen in the Adding process if the call used is Save(false) instead of > Save(true)? I reply to myself: I need to use OnTracksAdded () and OnUserNotifyUpdated () to get the same effect. This also answers this question: (In reply to comment #14) > How can I trigger a unique redraw for when the sync has finished (so as to do > it once instead of n, being n the number of tracks to sync)? So we're good! (In reply to comment #15) > (In reply to comment #14) > > Reopening because of the following reasons: > > * Issue found by Gabriel in the committed patch (comment#13). > > Not sure if now it's better, at least I've removed one call to Save() which may > improve things. See the description of the patch. This time I've measured this patch and it's as good as performance wise as when the first patch of this bug wasn't committed. The only cosmetic issue about this patch is that it eliminates the beauty of seeing the total number of songs of the source be increased while the sync process is happening. It now suddenly goes from 0 to N as soon as the process finishes. But it's good to prevent the user edit anything until all is finished.
Created attachment 170233 [details] [review] New approach for the iPod extension, v3 I had a small review from Gabriel on IRC, he told me to calculate the total for tracks_to_add inside the lock() statement, and this patch has this fixed. And on the topic of 0 to N: (In reply to comment #16) > The only cosmetic issue about this patch is that it eliminates the beauty of > seeing the total number of songs of the source be increased while the sync > process is happening. It now suddenly goes from 0 to N as soon as the process > finishes. But it's good to prevent the user edit anything until all is > finished. I meant 0 to N as in the total of songs that appear on the source, in the source panel. We could alleviate this cosmetic issue by calling OnTracksAdded () and OnUserNotifyUpdated () every X number of songs (like 10?) as a compromise between the 0toN situation or the UI update for every song which is bad for performance. What do you think? As soon as this gets approved&committed, I'll hack a patch for the AppleDevice extension.
Created attachment 170298 [details] [review] New approach for the iPod extension, v4 I've reverted the commit that was done earlier in this bug and pushed, just in case there was no time to review the new patch and include it for the release. Now, this is the new patch adapted to master, which reintroduces the changes of the original patch, and combines with the new approach.
Created attachment 170363 [details] [review] Same new approach, but for the AppleDevice extension (v1) This is a mockup for the same patch as before, but applied to the AppleDevice extension. It's only a mockup because I cannot check if it compiles or works (because I still don't have the correct dependencies), but I got the information of the API and the behaviour from Alan on IRC. Any feedback is appreciated.
In the description of the last patch, typo: s/BGO#589196) land/BGO#589196 land)/
Patch was modified slightly before committing so that the correct path was set after syncing. It appears to work perfectly for me now.
Review of attachment 170363 [details] [review]: Patch modified slightly for correctness and committed.
Review of attachment 170298 [details] [review]: Looks good to me but I'd prefer someone to test it with an ipod to verify everything works as expected before committing it.
Comment on attachment 170298 [details] [review] New approach for the iPod extension, v4 Thanks. As discussed on IRC this was already tested (as opposed to the AppleDevice's one).