Skip to content
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

Playing Directly from Artist Page #493

Merged
merged 6 commits into from
Oct 2, 2023
Merged

Playing Directly from Artist Page #493

merged 6 commits into from
Oct 2, 2023

Conversation

BobcatNoah
Copy link
Contributor

Implements #424

Adds a button to play all albums of an artist in both offline mode and online mode. Previously, the artist view in offline mode would just show up as a white screen if albums from multiple artists were downloaded. I had to add a bit of code to music_screen_tab_view.dart to make sure only the albums corresponding with the artist show up. Otherwise all downloaded albums in offline mode would show up in each individual artist view.

Also, how do I setup the environment for the redesign branch? I would have written the code in the redesign branch, but I couldn't get that branch to build. I suspect I just set up the environment incorrectly.

Before:
IMG_8149

After:
IMG_8148

@jmshrv
Copy link
Owner

jmshrv commented Sep 4, 2023

Nice, this has been a little thing that should have been added years ago 🙃

How does it work in offline mode? The code looks like you're figuring it out from downloaded parents, which is a pretty clever way around the lack of "real" downloaded artists.

As for the redesign branch, what error are you getting? A new Flutter version may have broken things, in theory it should "just work" as main does

@BobcatNoah
Copy link
Contributor Author

BobcatNoah commented Sep 4, 2023

First of all, it iterates through the downloaded songs, selects those by the current artist in view, and stores them in the songs list. It then groups these songs by album ID in a map called groupedSongs. It sorts the songs within each album by their index number. Finally, it converts the map into a list, which the queue is replaced with when you click the play button.

The error I'm getting is this.

Because finamp depends on flutter_localizations from sdk which depends on intl
  0.17.0, intl 0.17.0 is required.
So, because finamp depends on intl ^0.18.0, version solving failed.
pub get failed

Here is the output of flutter --version.

Flutter 3.7.12 • channel stable • https://github.com/flutter/flutter.git
Framework • revision 4d9e56e694 (5 months ago) • 2023-04-17 21:47:46 -0400
Engine • revision 1a65d409c7
Tools • Dart 2.19.6 • DevTools 2.20.1

@BobcatNoah
Copy link
Contributor Author

So, I got the redesign branch to build using flutter 3.10.5. However, whenever I open the music player, the app freezes and flutter: 80008 -3.4227457425910472 is printed to the console thousands of times per second with that first value increasing.

@Chaphasilor
Copy link
Collaborator

@BobcatNoah just switch to dark mode in settings :)
The contrast calculation is currently borked and never finishes in light mode 😅

@BobcatNoah
Copy link
Contributor Author

@Chaphasilor Thanks. That fixed it for me.

@jmshrv
Copy link
Owner

jmshrv commented Sep 6, 2023

That's a pretty clever solution to showing artists :)

Do you plan on doing something similar to show the artists list in offline mode? Currently, the offline functionality of the artist play button won't be reachable.

@BobcatNoah
Copy link
Contributor Author

It wasn't my original plan to implement the artists list in offline mode, but I guess I could try implementing it. I can't guarantee that I'll be able to do it in a timely manner.

Also, it technically is reachable by going to an album and clicking on the artist's name.

@jmshrv
Copy link
Owner

jmshrv commented Sep 6, 2023

I wouldn't mind working on it myself, it's been a while since I've seriously worked on Finamp

@BobcatNoah
Copy link
Contributor Author

Btw, I'm done with this pull request and I'm not planning to add any more code to it.

@jmshrv
Copy link
Owner

jmshrv commented Sep 29, 2023

Would this be mergable then? Sounds like it's in a good state

@Chaphasilor
Copy link
Collaborator

Chaphasilor commented Sep 29, 2023

Just left a review, if you feel like making some changes. Otherwise I'll take a look at it myself tomorrow and make some minor improvements :)

@jmshrv
Copy link
Owner

jmshrv commented Sep 30, 2023

Just left a review, if you feel like making some changes. Otherwise I'll take a look at it myself tomorrow and make some minor improvements :)

I'm not seeing it in the Files Changed tab, did you remember to submit it? 🙃

Comment on lines 47 to 60
final List<BaseItemDto>songs = [];

for (DownloadedSong item in downloadsHelper.downloadedItems) {
if (item.song.albumArtist == widget.artist.name) {
songs.add(item.song);
}
}

// We have to sort by hand in offline mode because a downloadedParent for artists hasn't been implemented
Map<String, List<BaseItemDto>> groupedSongs = {};
for (BaseItemDto song in songs) {
groupedSongs.putIfAbsent((song.albumId ?? 'unknown'), () => []);
groupedSongs[song.albumId]!.add(song);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be done in a single loop? There could be 10k+ downloaded songs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this is a non-issue. The code only iterates through the entire library one. After that, the code uses the songs variable which only contains a list of songs by the artist (so the songs variable will only ever hold at most a few hundred songs). I also just renamed the variable to better reflect that.

Comment on lines 62 to 69
groupedSongs.forEach((album, albumSongs) {
albumSongs.sort((a, b) => (a.indexNumber ?? 0).compareTo(b.indexNumber ?? 0));
});

final List<BaseItemDto> sortedSongs = [];
groupedSongs.values.forEach((albumSongs) {
sortedSongs.addAll(albumSongs);
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for this. After sorting, add the albumSongs to sortedSongs right away

@@ -28,6 +29,7 @@ class ArtistScreen extends StatelessWidget {
title: Text(artist.name ?? "Unknown Name"),
actions: [
// this screen is also used for genres, which can't be favorited
if (artist.type != "MusicGenre") ArtistPlayButton(artist: artist),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding a shuffle button as well?

@Chaphasilor
Copy link
Collaborator

@jmshrv thanks for letting me know, I used the mobile app and didn't notice the review wasn't submitted yet...

@jmshrv
Copy link
Owner

jmshrv commented Sep 30, 2023

btw I don't have push access to the fork, but the changes are relatively minor so it isn't the end of the world :)

@BobcatNoah
Copy link
Contributor Author

I added the shuffle button, however I noticed that it always will play a certain song first. Everything else is shuffled though.

@Chaphasilor
Copy link
Collaborator

I added the shuffle button, however I noticed that it always will play a certain song first. Everything else is shuffled though.

Hmm, this shouldn't be happening since #442 got merged, I wonder what this is about. Still, better than nothing ^^

@jmshrv
Copy link
Owner

jmshrv commented Oct 2, 2023

Yeah shuffling is a nightmare for some reason, I'll merge this since the code looks good (we're just nitpicking) :)

@jmshrv jmshrv merged commit 028b251 into jmshrv:main Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants