-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Android Auto #578
Android Auto #578
Conversation
This is awesome to see finally :) Is the 100 item limit just an awkward thing with Android Auto or a thing that specifically needs to be fixed here? Jellyfin luckily makes pagination very easy when fetching from the server. Artwork sounds like an annoying issue to fix though 🙃 |
I put that limit because someone reported Android Auto crashing on large libraries (1000+) I do have an idea on how to add pagination to it, so I'll probably work on that tonight.
Yeah, I'm gonna have to do some experimentation with that. |
Thanks for the PR! I can try to take a look at it in the coming days, I just need to find a car to test with ^^ I'm guessing there is no built-in pagination support in Android Auto, and I don't think it would be super useful anyway. Search and some voice interactions would definitely be useful down the road (hah), but if basic playback and browsing is working then I guess it qualifies for a beta. |
You can use the emulator! #24 (comment) It's very convenient for debugging too.
Yeah, I think I agree with you. Not sure that pagination should be a priority. I typically just use a playlist anyway lol.
Yeah, 100%. I wanted to incorporate that into this PR, but I was having an issue I didn't really understand where it crashes when searching. EDIT: fixed the crash, was a problem with changing the package name |
Yeah I know about the emulator and have used it last time, but in the end I want to make sure everything works on an actual car too ^^ Regarding voice, how flexible is that? Can it only be used for voice search, or could we enable custom actions, like saying "Start playing Hello by Adele" and it will start an instant mix? |
I know you can do stuff like that with app actions. There's also a version for Android Auto/Automotive. |
The second link you provided seems to be geared towards navigation/Points-of-Interest apps. Here's the one for media apps: https://developer.android.com/training/cars/media#support_voice Seems like it has everything we need. If the user indicates an item type (album, playlist, etc.) we can use that to deal with Jellyfin's poor search, and if they omit the type "Play some music" we could resort to shuffle all for now. Would you like to look into it? :) |
Sure, I have some time the next few days so I'll see if I can understand this and get something usable. |
Awesome. Let me know if you need help with anything! Edit: Just checked it out on the emulator, here are a few things I noticed:
|
Thanks for the feedback, I agree with your observations here.
I was thinking we could replace the genre tab with the home tab. Android Auto by default only allows 4 tabs, and genres don't have instant mixes (though we could shuffle albums within the genre).
I don't think this is possible, but a setting for the default would work.
I don't think I can do anything about these :(, they seem to be built into Android Auto.
Seems like this is related to that section, looks to be possible.
I've done a little troubleshooting, and I'm confused. It seems there may be a bug somewhere causing a conflict between just_audio and the new queue service?
The queue service expects finamp/lib/services/queue_service.dart Lines 94 to 101 in 2ea2156
finamp/lib/services/queue_service.dart Lines 148 to 151 in 2ea2156
I suspect somewhere in just_audio expects it to be the shuffled index and is sending it to the backend (ExoPlayer?), which results in the queue showing the wrong song as selected. To elaborate, the erroneous selected song in queue is in the position of the original (non-shuffled) index, which is why I suspect this. Changing music_player_background_task.dart - queueIndex: event.currentIndex,
+ queueIndex: _player.shuffleModeEnabled && (shuffleIndices?.isNotEmpty ?? false) && event.currentIndex != null ? shuffleIndices!.indexOf(event.currentIndex!) : event.currentIndex, queue_service.dart -int adjustedQueueIndex = (playbackOrder == FinampPlaybackOrder.shuffled &&
- _queueAudioSource.shuffleIndices.isNotEmpty)
- ? _queueAudioSource.shuffleIndices.indexOf(_queueAudioSourceIndex)
- : _queueAudioSourceIndex;
+ int adjustedQueueIndex = _queueAudioSourceIndex; |
We could reduce the number of tabs even further, with a home tab and a library tab? On the library tab we could have a grid with the different categories. Genre mixes should be possible, at least there's a way to add a genre to a mix on the phone 🤔 But maybe that just adds all individual albums, not sure. Can we change the sorting within Android Auto? If something like "Recently Added" is possible, then it should be possible to use the BaseItemDto's SortTitle too. I'll take a look at the modifications to the queue service. just_audio is really opinionated and the layered architecture doesn't make things easier. I've struggled with it for many monhs 🙃 Edit: Genre instant mixes should be a thing, at least it works in the Jellyfin app. No idea why it's hidden in Finamp... |
@puff I managed to fix up the queue based on your proposed changes. There were a few other things I needed to adjust, and I fixed a bug that I introduced a while ago (items in next up were not being reported to the external/OS queue). |
Yeah, I like that idea.
I didn't see a way already implemented in Finamp yet, but yeah it should be possible if the Jellyfin app has it.
Yes, we can sort the MediaItem list before returning from
Looks to be because the Jellyfin api is case-insensitive: I've fixed it and added sort to Android Auto. It uses whatever sort option is selected in the phone app. |
Thanks for fixing that. There are some sorting-related issues that need to be tackled at some point anyway, like #444 or #581, so if you feel like taking a look while you're dealing with sorting anyway, that would be much appreciated. |
I managed to get artwork to work with offline items and (kind've) Android Automotive using the android_content_provider package. The white icon in the middle is the placeholder art. Only problem is that for Android Automotive, we can't use |
Did you manage to take a look at the voice command stuff? Or is there anything that you need help with? :) |
Sorry for not responding earlier, I was a bit busy. Just tested it out and didn't manage to invoke the I guess we should skip this feature for now, including regular search. Search is due for a rewrite soon, and it makes little sense to integrate the old version now... |
Nothing in the logs sadly. Only logs I get are Here's what it looks like: I can try testing it with an actual car in the coming days, maybe even tomorrow. |
lib/services/queue_service.dart
Outdated
|
||
// replace with content uri or jellyfin api uri | ||
if (downloadedImage != null) { | ||
artUri = downloadedImage.file?.uri.replace(scheme: "content", host: "com.unicornsonlsd.finamp"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will all these content URIs work on iOS? Android seems to have a custom handler we've written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, should probably have a platform check here.
@@ -57,6 +57,7 @@ class _MusicScreenTabViewState extends State<MusicScreenTabView> | |||
|
|||
final _jellyfinApiHelper = GetIt.instance<JellyfinApiHelper>(); | |||
final _isarDownloader = GetIt.instance<DownloadsService>(); | |||
final _finampUserHelper = GetIt.instance<FinampUserHelper>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this line used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not anymore, at least according to the linter ^^
@@ -1263,3 +1264,41 @@ class PreviousTracksSectionHeader extends SliverPersistentHeaderDelegate { | |||
@override | |||
bool shouldRebuild(SliverPersistentHeaderDelegate oldDelegate) => false; | |||
} | |||
|
|||
class QueueTracksMask extends SingleChildRenderObjectWidget { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed.
lib/main.dart
Outdated
@@ -111,6 +112,21 @@ void main() async { | |||
: "en_US"; | |||
await initializeDateFormatting(localeString, null); | |||
|
|||
// Load the album image from assets and save it to the documents directory for use in Android Auto | |||
final documentsDirectory = await getApplicationDocumentsDirectory(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't put this in documents. Put it in application support. Also, this whole block should probably be in one of the submethods where we've still got the errorApp wrapper.
lib/main.dart
Outdated
@@ -111,6 +112,21 @@ void main() async { | |||
: "en_US"; | |||
await initializeDateFormatting(localeString, null); | |||
|
|||
// Load the album image from assets and save it to the documents directory for use in Android Auto | |||
final documentsDirectory = await getApplicationDocumentsDirectory(); | |||
final albumImageFile = File('${documentsDirectory.absolute.path}/images/album_white.png'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think constructing files from full pathstrings is considered good practice? I'm not sure how much that matters though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/images/album_white.png
could be moved to a constant. Not really sure what else to do here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I basically mean using the path_helper to combine the segments. Like I said, I'm not sure how much it matters though.
|
||
// triggers when skipping to specific item in android auto queue | ||
@override | ||
Future<void> skipToQueueItem(int index) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just rename skipToIndex to match the override?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think skipToIndex()
is clearer, and it doesn't really hurt to do it this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess not? It's pretty strange to have this pointless redirection though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't think so. There are some weird external methods that the OS can decide to call, but unifying this into a more sensible API seems like a good idea to me.
Basically, all we're doing is say "if Android Auto decides it needs to skip to a specific queue item, that just means we skip to the provided index, just like we do in some other cases". That redirection is basically some documentation/explanation saying that the two actions are equivalent, without there being any confusion...
|| tabContentType == TabContentType.artists || tabContentType == TabContentType.songs; | ||
} | ||
|
||
Future<MediaItem> _convertToMediaItem({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be unified with the method in queueService. Maybe the id string and playable bool should be optional parameters for that? Alternatively, it looks like this is only used for menu items, so maybe you just unify the art URI fetching and then cut this version down?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this?
Thanks for the review Komodo! The age of this PR is starting to show... I'll try to go through your comments later and improve the sections as best as I can :) |
Also before I forget, the way Symfonium handles playing in-order vs. shuffled is by making each "leaf" item a node/collection, which has two children, "Play in order" and "Play shuffled". I think that would also be a good solution for us, and we could even add more choices, like starting and instant mix or adding to Next Up. |
Okay, aside from breaking the whole Android Auto media browser, the review findings should be handled now 😅 |
There we go. Managed to fix it today :D |
Okay, the enhanced search is also working now. I'd like to get a like button onto the AA player screen / media notification because it's been a long-requested feature, and then this should be ready to merge |
Custom browse actions (https://developer.android.com/training/cars/media#custom_browse_actions) would be a much better solution though... |
This should be ready to merge now. All the basics are working reasonably well. If someone could confirm that this still works fine on iOS that would also be nice :) |
Okay, I'll merge this in now for the next update, since most things should be working fine. If not, we won't know without having users test it anyway. |
Thank you for getting this started looking into the documentation, etc. @puff! |
@puff do you have a working Automotive setup? I'm low on disk space, but it seem like we need screenshots from Automotive for our Play Store listing to pass the review. If you could grab some screenshots in landscape and portrait mode using the official demo server, that would be great! |
Hey guys, any update on this? |
@frandj99 this was merged 5 months ago and is in the redesign branch. You can install the beta using the instructions in the README.md. |
@frandj99 while this has indeed been merged into the beta for some time, I'm not sure if it actually works with Android Automotive. Please try it out and let us know! |
@Titaniumtown @Chaphasilor I was finally able to build the app (also I had to go through a private test on Google Play Console in order to install on my car, cuz sideloading is not allowed 🥲) I'm down to test anything, let me know if I'm missing something or so... |
@frandj99 is that Android Automotive? Does it install the app directly onto the car? 🤔 |
Yes, it's Android Automotive of my Volvo XC40. Sideloading is not allowed at all, I can't access the developer setting on Volvo AAOS "for security reasons", so the only way you can install any application is through the Play Store. Yeah, looong and boring way, I needed some help by a friend of mine to compile the app in Android studio but finally today I was able to see the Finamp icon on my car 😄 (but it doesn't work) |
Adds basic Android Auto and Android Automotive support.
Features:
Future features/TODO?
Currently, there is a hard limit of
100250 (online mode) or 1000 (offline mode) items displayed so the app doesn't crash on large libraries.// dev notes