CarPlay Support w/ llim1 base#1516
Conversation
a7c7134 to
b2a397f
Compare
llim1
left a comment
There was a problem hiding this comment.
Some comments, also a few notes:
- known issue "flutter_carplay loads images from web URIs but file URIs are broken despite file paths being listed as supported" is marked as fixed but issue is still present in my environment. Some artwork shows a '?' image where visible
- jellyfin api related changes not reviewed
- siri search not tested
|
Thanks for the review @llim1
I misunderstood this originally, I am now able to recreate it, when there were spaces encoded in the url it broke. I've added a fix for this. |
Confirmed, images issue is fixed. No further comments from me 👍 |
|
@APIUM you mentioned on Discord that we can go back to using the upstream plugin instead of your fork, right? |
Yep I’ll make this change in here |
I’m waiting on Apple to approve my personal account, they look about 3 days to ask for ID, and it’s been over a week since then. They say it’s a 2 day process…. I’m all set up in the team so I’ll be able to test very quickly once Apple approves. |
|
really looking forward to this in Finamp. Thanks for working on it! Can I cheekily request having an instant mix button available on CarPlay. Would love if it was easy to start a good mix (especially for those who use the new audiomuse-ai backend). |
Hi @Haar thanks for having a look. Shuffle all is just a standard shuffle of all the music in your library. @networkoctopus see above for the current Instant Mix situation Harr please let me know if you find a way to test on real hardware, I am having trouble with profiles and certificates. Still working this out and it's why this PR has stalled. |
525c5d6 to
e053219
Compare
c9842c1 to
660c558
Compare
0a9c32f to
62bc78d
Compare
|
@APIUM seems like this is coming along nicely! If you resolve the merge conflicts you'll also get CI runs again :) |
Use AlbumArtist endpoint instead of generic Artist endpoint so that performing-only artists (who have no albums) don't appear in the list.
62bc78d to
622ef6a
Compare
Done! Needs approval to run |
|
also, can you update the todo list at the top to reflect the current status? |
Sorry, didn't see this. Have just pushed the formatting fix. |
Chaphasilor
left a comment
There was a problem hiding this comment.
Okay, found some things that should probably be updated, but most things look good from my perspective!
| static const androidStopForegroundOnPause = true; | ||
| static const onlyShowFavorites = false; | ||
| static const trackShuffleItemCount = 250; | ||
| static const quickShuffleItemCount = 30; |
There was a problem hiding this comment.
maybe add a description of what this is used for? as in, why do we need this.
| return; | ||
| } | ||
|
|
||
| // Fetch 1 random track and start continuous radio from it. |
There was a problem hiding this comment.
If you want to enable radio, then please rename the method. "Instant Mix" is a specific Jellyfin feature and pre-fetches similar tracks in advance. Radio fetches tracks just-in-time, and the Continuous radio mode does not yield the same results as an instant mix. For that, you should use the "Similar" mode instead.
There was a problem hiding this comment.
On the other hand, if this is for the "Instant Mix" quick action, then you should just fetch the items from the correct API instead, or add an itemCount parameter to the existing instant mix method of the audio service helper.
There was a problem hiding this comment.
Yeah that's fair. It was for 'Instant Mix' on the UI. I would like to avoid the queue size issue again so I have renamed this Radio Mix
There was a problem hiding this comment.
There's no "Radio Mix" - we have "Instant Mix" and "Start Radio" in the UI.
I'm guessing you can't access the translation tokens from CarPlay? If you can, just use the same token that's used in the track or album menu ^^
| List<FinampQueueItem> getRecentPlays({int limit = 5}) { | ||
| return _queueService.peekQueue(previous: limit); | ||
| } |
There was a problem hiding this comment.
where is this used? If the queue is replaced, this might be an empty list. The playback history service could be another source.
There was a problem hiding this comment.
Yeah you're right. It's used on the CarPlay home screen. I have changed it to that.
There was a problem hiding this comment.
I didn't really pick up on this, the CarPlay home screen is only built once and there aren't any tracks in there, so it doesn't update. I think I'd rather just make it better in the updated Home Screen so I'm going to leave this as-is (still changing to the playback history service)
| case TabContentType.genres: | ||
| showAlbumsTemplate(tabType: parentId.contentType); | ||
| case TabContentType.artists: | ||
| showArtistsTemplate(); |
There was a problem hiding this comment.
traditionally genres in Finamp were closer to artists than to albums. What made you use the albums template for them here?
There was a problem hiding this comment.
Do you mean you'd get a list of tracks rather than albums? I picked albums as it gives the user a lot more choice with the constrained list length on CarPlay
There was a problem hiding this comment.
So am I wrong in assuming that these templates refer to a screen? So that an album template shows the content of a specific album, and an artist template that of a specific artists?
Because in that case, an album would contain tracks, and an artist could contain a mix of tracks and albums, with an emphasis on albums. That's what I meant by "traditionally": before the redesign, the artist screen in Finamp was just a list of albums, and so was the genre screen.
But if this template refers to how list items are rendered, then of course the genres tab should could show genres that look like "albums" when rendered.
There was a problem hiding this comment.
Yeah it was a confusing name (since renamed) - yes it's just the list template, not a specific screen. I'd previously only used it for Albums but now it's 'showBrowsableListTemplate' instead.
29acc9d to
f4d8177
Compare
| /// Returns a map of imageId -> local file URI string. | ||
| /// Items sharing the same imageId are downloaded only once. | ||
| Future<Map<String, String>> _preloadCarPlayImages(List<BaseItemDto> items) async { | ||
| final Map<String, String> imageUriMap = {}; |
There was a problem hiding this comment.
I think it's better to resolve the URI with something more like _providers.read(albumImageProvider(AlbumImageRequest(item: item, maxHeight: 200, maxWidth: 200))).uri, which is what android auto does. This covers things like the player image cache, which you missed with this logic. By the way, have we verified that carplay doesn't have an internal cache if we give identical URIs repeatedly?
| try mutableDir.setResourceValues(values) | ||
| } | ||
|
|
||
| // TODO: This is a workaround because audio_service doesn't set MPNowPlayingInfoCenter.playbackState on iOS. |
There was a problem hiding this comment.
It looks like there's already a PR fixing this ryanheise/audio_service#1140, so it might be easier to strip this code out and just switch to that fork until it's merged?
There was a problem hiding this comment.
I'm worried it won't get merged, it's 7 months old. I'm happy to make a ticket for this and we can update and remove the workaround when it's merged?
There was a problem hiding this comment.
If this is not a ton of messy code that could be avoided by this, it's probably better to keep tracking the regular audio_service repo. I'd like to stay up to date with any other fixes there.
|
|
||
| /// Maximum items to fetch from server for CarPlay lists. | ||
| /// Keeps UI responsive and avoids memory issues on car displays. | ||
| const _carPlayOnlineLimit = 250; |
There was a problem hiding this comment.
I notice a lot of the lists seem to just cutoff at this limit, and 250 isn't crazy high. Is there a way to add pagination, or at least an indicator that we've cutoff the list?
There was a problem hiding this comment.
Another I'd like to improve later. CarPlay starts to get laggy above around this - AA is the same. I want to be able to load as we go with pagination.
There was a problem hiding this comment.
That was the core concept I was planning to use too - when that's merged then we should have the functions I was missing when I was trying to get this in that made me push it out to a follow up PR. I'll put a link to it in my issue.
There was a problem hiding this comment.
Ah I didn't see they were using NameStartsWith - I was having trouble for this use, I've now forgotten as I think there were a couple of issues, but one is that in CarPlay we display by SortName, but that API call doesn't give SortName it gives the regular name so things would show up in the wrong spot. I think there was a bigger showstopper though, it might have been that this causes issues with special characters?
There was a problem hiding this comment.
I wouldn't be surprised. Also haven't tested that PR yet.
|
@UnicornsOnLSD is anything needed on the App Store side if we release this? I'd like to avoid being rejected because we forgot to change some configuration here or in the store settings 😅 |
|
There shouldn't be, main thing is getting the entitlement, which we do have (and seems to work in TestFlight builds, which is promising :) ) |
8b4d3a8 to
b5ebb17
Compare

Adds CarPlay support using flutter_carplay package.
This PR is based on the work from llim1 #1474 adding the finishing touches to get it ready to merge.
Todo before merging
search tab (likely requires changes to flutter_carplay)- Replaced this with Siri support, this can come in a follow up PR (Done in this PR now)recently played tab- follow up PRreplace static text with localizationsrich 'now-playing' screen inc. queue management + album art(likely requires changes to flutter_carplay) - follow up PRnow-playing icon on list entries- follow up PRKnown issues
Will cross off issues here once they are addressed.
Related Issues
Closes #24
Original PR #1474