Support file url album art - Android
ClosedPublic

Authored by mtijink on Mar 4 2018, 1:13 PM.

Details

Summary

If the album art is a file url, the code will ask the remote device to transfer that file. It's then stored in the album art cache.

Test Plan

Works fine for me, even when quickly skipping songs.

Diff Detail

Repository
R225 KDE Connect - Android application
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mtijink requested review of this revision.Mar 4 2018, 1:13 PM
mtijink created this revision.
nicolasfella added a subscriber: nicolasfella.EditedMar 5 2018, 2:19 PM

When you quickly click next (either desktop or android), the code will ask for most of the album arts. But because the desktop does a check to see if the url belongs to one of its players, some fetches are invalid by that time. The android code will not retry fetching, since it thinks it's already fetching that album art. How do I fix that correctly?

Do you mean you switch between tracks with covers A, B, C, D, A and A won't get loaded because its already in the isFetchingList? What about sending a NACK/Failed packet back that removes the cover from the isFetchingList?

When you quickly click next (either desktop or android), the code will ask for most of the album arts. But because the desktop does a check to see if the url belongs to one of its players, some fetches are invalid by that time. The android code will not retry fetching, since it thinks it's already fetching that album art. How do I fix that correctly?

Do you mean you switch between tracks with covers A, B, C, D, A and A won't get loaded because its already in the isFetchingList?

Yes, exactly.

What about sending a NACK/Failed packet back that removes the cover from the isFetchingList?

I thought about that, but that seems fragile to me. The best I could come up with is a timer to remove the urls from the list.

I thought about that, but that seems fragile to me. The best I could come up with is a timer to remove the urls from the list.

Have you actually tried any of those?

I thought about that, but that seems fragile to me. The best I could come up with is a timer to remove the urls from the list.

Have you actually tried any of those?

No, I wanted to see some ideas first, if possible.

mtijink updated this revision to Diff 29746.Mar 17 2018, 1:04 PM

I fixed the problem: the code now only counts the the art as transferring once the remote actually replies with the art.

This could theoretically result in multiple requests for the same art, but no code requests the art very often, so I don't expect it to be a problem in practice. The actual art isn't transferred multiple times.

mtijink edited the test plan for this revision. (Show Details)Mar 17 2018, 1:05 PM
nicolasfella requested changes to this revision.Mar 17 2018, 2:28 PM

Please rebase to master

This revision now requires changes to proceed.Mar 17 2018, 2:28 PM
mtijink updated this revision to Diff 29782.Mar 17 2018, 8:14 PM

Rebased on master

albertvaka added inline comments.
src/org/kde/kdeconnect/Plugins/MprisPlugin/AlbumArtCache.java
361

I find it a bit weird to query all MPRIS plugins for the art, and then in askTransferAlbumArt check if you hit the right plugin... Isn't there a way to pass the plugin that you know has the art to this function?

mtijink added inline comments.Mar 20 2018, 7:01 PM
src/org/kde/kdeconnect/Plugins/MprisPlugin/AlbumArtCache.java
361

Good idea, I'll change some stuff to make that better.

mtijink updated this revision to Diff 30390.Mar 24 2018, 1:21 PM

Updated according to @albertvaka's suggestion.

albertvaka accepted this revision.Mar 25 2018, 11:35 AM
This revision was not accepted when it landed; it landed in state Needs Review.Mar 25 2018, 11:41 AM
This revision was automatically updated to reflect the committed changes.