After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 589196 - [PATCH] Edited metadata on DAP tracks not saved
[PATCH] Edited metadata on DAP tracks not saved
Status: RESOLVED WONTFIX
Product: banshee
Classification: Other
Component: Device (general)
git master
Other Linux
: Normal normal
: ---
Assigned To: Banshee Maintainers
Banshee Maintainers
gnome[unmaintained]
: 559416 620181 (view as bug list)
Depends on: 609411 620826
Blocks: 389550
 
 
Reported: 2009-07-21 01:24 UTC by Andrés G. Aragoneses (IRC: knocte)
Modified: 2020-03-17 08:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Saves the rating when in manual mode (6.49 KB, patch)
2009-07-21 01:25 UTC, Andrés G. Aragoneses (IRC: knocte)
reviewed Details | Review
Patch v2 (9.07 KB, patch)
2010-05-31 10:06 UTC, Andrés G. Aragoneses (IRC: knocte)
needs-work Details | Review
Infrastructure to make metadata resync to DAP possible for Manual Sync mode (8.43 KB, patch)
2010-08-06 13:19 UTC, Andrés G. Aragoneses (IRC: knocte)
needs-work Details | Review
Metadata resync to DAP possible for Manual Sync mode: iPod support (2.58 KB, patch)
2010-08-06 13:21 UTC, Andrés G. Aragoneses (IRC: knocte)
needs-work Details | Review
(v2) Infrastructure to make metadata resync to DAP possible for Manual Sync mode (9.21 KB, patch)
2010-08-11 10:53 UTC, Andrés G. Aragoneses (IRC: knocte)
none Details | Review
(v3) Infrastructure to make metadata resync to DAP possible for Manual Sync mode (10.51 KB, patch)
2010-08-11 17:41 UTC, Andrés G. Aragoneses (IRC: knocte)
committed Details | Review
(v2) Metadata resync to DAP possible for Manual Sync mode: iPod support (5.01 KB, patch)
2010-08-11 19:45 UTC, Andrés G. Aragoneses (IRC: knocte)
none Details | Review
(v1) Metadata resync to DAP possible for Manual Sync mode: AppleDevice support (5.73 KB, patch)
2010-08-12 11:03 UTC, Andrés G. Aragoneses (IRC: knocte)
none Details | Review
(v1) Metadata resync to DAP possible for Manual Sync mode: AppleDevice support (fixed compilation) (5.17 KB, patch)
2010-08-13 14:53 UTC, Alex Launi
committed Details | Review
(v3) Metadata resync to DAP possible for Manual Sync mode: iPod support (5.36 KB, patch)
2010-09-14 12:26 UTC, Andrés G. Aragoneses (IRC: knocte)
committed Details | Review
(v1) Metadata resync to DAP possible for Manual Sync mode: Android support (8.84 KB, patch)
2010-12-27 20:13 UTC, Andrés G. Aragoneses (IRC: knocte)
none Details | Review
(v2) Metadata resync to DAP possible for Manual Sync mode: Android support (10.18 KB, patch)
2010-12-30 15:35 UTC, Andrés G. Aragoneses (IRC: knocte)
none Details | Review
(v3) Metadata resync to DAP possible for Manual Sync mode: Android support (10.11 KB, patch)
2010-12-30 16:06 UTC, Andrés G. Aragoneses (IRC: knocte)
reviewed Details | Review
(v4) Metadata resync to DAP possible for Manual Sync mode: MassStorage support (4.12 KB, patch)
2011-12-29 00:03 UTC, Andrés G. Aragoneses (IRC: knocte)
committed Details | Review

Description Andrés G. Aragoneses (IRC: knocte) 2009-07-21 01:24:18 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.
Comment 1 Andrés G. Aragoneses (IRC: knocte) 2009-07-21 01:25:45 UTC
Created attachment 138871 [details] [review]
Saves the rating when in manual mode

The patch is quite simple.
Comment 2 jgoerzen 2009-07-21 02:30:43 UTC
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
Comment 3 Andrés G. Aragoneses (IRC: knocte) 2009-07-21 03:02:40 UTC
(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.

Comment 4 jgoerzen 2009-07-24 14:45:46 UTC
Good news - it applied cleanly over 1.5.0 and does seem to fix the rating issue.  Thanks!
Comment 5 Andrés G. Aragoneses (IRC: knocte) 2009-07-24 15:19:04 UTC
Thanks for testing jgoerzen. I think we'll get a review of the patch next week.
Comment 6 Andrés G. Aragoneses (IRC: knocte) 2009-09-30 15:24:04 UTC
Can any maintainer/developer review the patch? Thanks.
Comment 7 Gabriel Burt 2009-10-06 16:38:30 UTC
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.
Comment 8 Gabriel Burt 2009-10-06 16:39:50 UTC
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?
Comment 9 Andrés G. Aragoneses (IRC: knocte) 2009-10-06 16:49:23 UTC
(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!
Comment 10 Andrés G. Aragoneses (IRC: knocte) 2009-10-06 16:50:41 UTC
(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 :)
Comment 11 Gabriel Burt 2009-10-27 20:18:37 UTC
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.
Comment 12 Andrés G. Aragoneses (IRC: knocte) 2010-02-28 00:03:30 UTC
(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.
Comment 13 Andrés G. Aragoneses (IRC: knocte) 2010-02-28 00:12:10 UTC
(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.
Comment 14 Andrés G. Aragoneses (IRC: knocte) 2010-05-31 10:06:52 UTC
Created attachment 162366 [details] [review]
Patch v2

New approach.
Comment 15 Andrés G. Aragoneses (IRC: knocte) 2010-05-31 10:10:33 UTC
The new patch takes care of any metadata, not only rating, so I'm updating the summary.
Comment 16 Gabriel Burt 2010-06-02 18:01:38 UTC
*** Bug 620181 has been marked as a duplicate of this bug. ***
Comment 17 Gabriel Burt 2010-06-02 18:02:51 UTC
*** Bug 559416 has been marked as a duplicate of this bug. ***
Comment 18 Alexander Kojevnikov 2010-06-03 00:47:01 UTC
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`.
Comment 19 techtonik 2010-06-03 07:07:16 UTC
(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.
Comment 20 Andrés G. Aragoneses (IRC: knocte) 2010-06-03 10:22:29 UTC
(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 21 Andrés G. Aragoneses (IRC: knocte) 2010-06-03 10:23:55 UTC
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.
Comment 22 Philip Gillißen 2010-07-15 18:04:27 UTC
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...
Comment 23 Andrés G. Aragoneses (IRC: knocte) 2010-07-15 21:01:13 UTC
(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!
Comment 24 Philip Gillißen 2010-07-29 17:15:26 UTC
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
Comment 25 Andrés G. Aragoneses (IRC: knocte) 2010-07-31 17:01:25 UTC
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.
Comment 26 Andrés G. Aragoneses (IRC: knocte) 2010-08-05 16:07:13 UTC
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.
Comment 27 Andrés G. Aragoneses (IRC: knocte) 2010-08-06 13:19:22 UTC
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.
Comment 28 Andrés G. Aragoneses (IRC: knocte) 2010-08-06 13:21:50 UTC
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.
Comment 29 Philip Gillißen 2010-08-07 08:34:07 UTC
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
Comment 30 Andrés G. Aragoneses (IRC: knocte) 2010-08-09 11:12:40 UTC
(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).
Comment 31 Gabriel Burt 2010-08-11 05:24:59 UTC
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.
Comment 32 Gabriel Burt 2010-08-11 05:29:42 UTC
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?
Comment 33 Gabriel Burt 2010-08-11 05:30:37 UTC
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.
Comment 34 Andrés G. Aragoneses (IRC: knocte) 2010-08-11 10:53:13 UTC
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.
Comment 35 Andrés G. Aragoneses (IRC: knocte) 2010-08-11 17:41:44 UTC
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
Comment 36 Andrés G. Aragoneses (IRC: knocte) 2010-08-11 19:45:22 UTC
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.
Comment 37 Andrés G. Aragoneses (IRC: knocte) 2010-08-11 20:01:45 UTC
(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.
Comment 38 Andrés G. Aragoneses (IRC: knocte) 2010-08-12 11:03:24 UTC
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!
Comment 39 Alex Launi 2010-08-13 14:53:33 UTC
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)
Comment 40 Alex Launi 2010-08-13 15:17:48 UTC
Just tested these patches with an iPod classic and the AppleDevice extension (gio-hardware) branch. Worked!
Comment 41 Philip Gillißen 2010-08-30 19:57:50 UTC
Hi Andrés!

Could provide some information how to test this patch/these patches?
I would like to test them.

Greetings, Philip
Comment 42 Gabriel Burt 2010-08-31 19:10:45 UTC
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.
Comment 43 Andrés G. Aragoneses (IRC: knocte) 2010-08-31 20:01:18 UTC
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.
Comment 44 Andrés G. Aragoneses (IRC: knocte) 2010-08-31 20:05:10 UTC
(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).
Comment 45 Gabriel Burt 2010-08-31 22:10:56 UTC
(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.
Comment 46 Andrés G. Aragoneses (IRC: knocte) 2010-09-14 12:26:07 UTC
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).
Comment 47 Andrés G. Aragoneses (IRC: knocte) 2010-09-19 12:25:38 UTC
(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).
Comment 48 Andrés G. Aragoneses (IRC: knocte) 2010-12-27 20:13:37 UTC
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.
Comment 49 Andrés G. Aragoneses (IRC: knocte) 2010-12-30 15:35:51 UTC
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.
Comment 50 Andrés G. Aragoneses (IRC: knocte) 2010-12-30 16:06:23 UTC
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.
Comment 51 Christophe 2011-01-31 13:28:54 UTC
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
Comment 52 Gabriel Burt 2011-03-22 16:31:04 UTC
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?
Comment 53 Andrés G. Aragoneses (IRC: knocte) 2011-04-04 17:07:28 UTC
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).
Comment 54 Andrés G. Aragoneses (IRC: knocte) 2011-12-29 00:03:02 UTC
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.)
Comment 55 Andrés G. Aragoneses (IRC: knocte) 2011-12-29 00:04:27 UTC
s/v2/v4/
Comment 56 Andrés G. Aragoneses (IRC: knocte) 2011-12-29 00:07:44 UTC
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.
Comment 57 Alexander Kojevnikov 2012-01-17 11:53:08 UTC
Review of attachment 204306 [details] [review]:

Looks good.
Comment 58 Andrés G. Aragoneses (IRC: knocte) 2012-01-17 12:57:14 UTC
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.
Comment 59 Samuel Gyger (IRC: thinkabout) 2012-03-12 22:03:37 UTC
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.
Comment 60 André Klapper 2020-03-17 08:24:59 UTC
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.