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

Album Shuffle Mode Implementation #201

Merged
merged 14 commits into from Oct 26, 2017

Conversation

@willcoughlin
Copy link
Contributor

commented Oct 12, 2017

I've implemented this feature according to suggestions by @clockler in the issue thread.

For a small overview:

  • ShuffleMode now includes ALBUMS, and ON has been changed to TRACKS.

  • Most items referring to shuffle generally now specify track shuffle. Analogues of track shuffling functionality in MusicService, MusicUtils, and elsewhere now exist for album shuffling.

  • New UI elements for album shuffle, including ShuffleAlbumsView, uses a layout similar to the list item shuffle element that comes before the list of songs.

@timusus

This comment has been minimized.

Copy link
Owner

commented Oct 12, 2017

Wow, thanks so much for this, I really appreciate your work.

I'll have a look over it shortly. I'm excited to test out album shuffle!

So, I haven't looked at the code yet, but what happens if you've just queued up some random list of songs, and then you turn album shuffle on?

@willcoughlin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2017

It's a pleasure to have the opportunity to contribute to something I use so regularly. Whatever list of songs you have queued will mapped to an Album. The key set from this map is put into a list, and then shuffled. This shuffled list is iterated through, and the album shuffle list is created by adding the List<Song> corresponding to each album.

@timusus
Copy link
Owner

left a comment

I'm really happy with this PR - I can tell you've implemented these changes quite thoroughly, and you've made an effort to stick to the existing code style. I really appreciate the work!

} else if (shuffleMode == ShuffleMode.ALBUMS) {
len = albumShuffleList.size();
q.setLength(0);
for (int i = 0; i < len; i++) {

This comment has been minimized.

Copy link
@timusus

timusus Oct 12, 2017

Owner

This could be factored out into a method, since building the list of reverse-hex numbers is now done in the same fashion for three separate lists.

}
}

private static List<Song> albumShuffleify(List<Song> songs) {

This comment has been minimized.

Copy link
@timusus

timusus Oct 12, 2017

Owner

This method could use some comments on what it does, as it's not clear from the method name alone.

'Takes a list of songs, groups them by album, then shuffles the albums - returning the resultant 'album-shuffled' list'

Perhaps the method could also be renamed to albumShuffleSongs (though it's not a huge improvement!)

@@ -129,7 +135,8 @@ public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle sa
layoutManager.setSpanSizeLookup(new GridLayoutManager.SpanSizeLookup() {
@Override
public int getSpanSize(int position) {

This comment has been minimized.

Copy link
@timusus

timusus Oct 12, 2017

Owner

There's a class in Shuttle which I'm not actually using at the moment, which simplifies this.

See com.simplecityapps.recycler_adapter.recyclerview.SpanSizeLookup. If you attach this to the LayoutManager, then the span size is automatically gleaned from the ViewModel.getSpanSize() method - so we no longer have to manually implement the span count.

So, we'd just have layoutManager.setSpanSizeLookup(new SpanSizeLookup(spanCount));

The default spanSize is spanCount, meaning if you have a grid with a span of '2', by default a ViewModel will span 2 columns. You just have to ensure that getSpanSize() has been overridden in any ViewModel which is supposed to only span one column (such as AlbumView).

If this sounds complicated, let me know and I'll make this change myself.

This comment has been minimized.

Copy link
@willcoughlin

willcoughlin Oct 12, 2017

Author Contributor

Sounds simple enough, I will take a look at it.

import com.simplecity.amp_library.R;
import com.simplecity.amp_library.ui.adapters.ViewType;

public class ShuffleAlbumsView extends ShuffleView {

This comment has been minimized.

Copy link
@timusus

timusus Oct 12, 2017

Owner

We could probably get away with just giving the ShuffleView icon drawable a public setter (or a constructor overload to pass in the drawable resource), since the only difference here is the icon being used. It's not a big deal though - I'm glad to see someone using Shuttle's modelviews stuff, seemingly without too much trouble!

@@ -30,35 +30,35 @@
int NEW_PLAYLIST = 2;
}

/**
* Passes along a list of songs, wrapped in a Single obj, to be played, starting at position 0.

This comment has been minimized.

Copy link
@timusus

timusus Oct 12, 2017

Owner

Better to use a javadoc link:

/**
*  Passes along a list of songs, wrapped in an {@link Single}, to be played, starting at position 0.
**/
public static void playAll(Single<List<Song>> songsSingle, UnsafeConsumer<String> onEmpty) {
songsSingle.observeOn(AndroidSchedulers.mainThread())
.subscribe(songs -> playAll(songs, onEmpty));

This comment has been minimized.

Copy link
@timusus

timusus Oct 12, 2017

Owner

This feels like a subtle looking but intentional change which actually changes Shuttle's default queuing/shuffling behaviour (for the better) - shuffle will now turn off when selecting a new list of songs to play, is that correct?

In other words, if you have shuffle on, and then you hit 'play' on an album, shuffle will be turned off.

I agree with this change (something I've been meaning to do for ages). Is this something that was required for album-shuffle, or was it just an aside / something else you wanted to fix? (Just curious!)

This comment has been minimized.

Copy link
@willcoughlin

willcoughlin Oct 12, 2017

Author Contributor

It was just an aside. I thought it made more sense working that way.

@@ -0,0 +1,49 @@
<?xml version="1.0" encoding="utf-8"?>
<vector xmlns:android="http://schemas.android.com/apk/res/android"

This comment has been minimized.

Copy link
@timusus

timusus Oct 12, 2017

Owner

This icon is quite dense, and doesn't fit in too well next to the other icons, IMO. I'll see if I can come up with an alternative.

This comment has been minimized.

Copy link
@willcoughlin

willcoughlin Oct 12, 2017

Author Contributor

By all means. This was my first ever experience with designing vector graphics and using Inkscape.

}
playPos = 0;
}
}

public void makeAlbumShuffleList() {
synchronized (this) {
if (albumShuffleList.size() > 0) {

This comment has been minimized.

Copy link
@timusus

timusus Oct 12, 2017

Owner

Unless we need to init the albumShuffleList here even in early-return cases, I would put this after the early-return (playlist == null) check. Just a code style thing - it reads better to me if early return code happens first.

@timusus

This comment has been minimized.

Copy link
Owner

commented Oct 12, 2017

If you merge in the latest dev changes, the Travis build issue should go away.

@timusus

This comment has been minimized.

Copy link
Owner

commented Oct 12, 2017

I've had a look over the code, and left a few comments for potential changes. Overall, it's really great, thanks.

I'm wondering - do you think album-shuffle mode should arrange the songs in track order, or should the songs just be random? I feel like the use case for album shuffle is that you want to hear whole albums - in which case you probably want to hear them in track-order. If not, perhaps this should be optional.. And if it was, I wonder how we might implement the option to change the order of tracks in album-shuffle mode. I'd be keen to hear your thoughts.

@willcoughlin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2017

I was conflicted over the album shuffle arrangement as well. The question is whether it should be an album shuffle or a song shuffle, grouped by album, and I went with the latter. Creating a whole new flavor of album shuffle, along with a distinct icon, seemed too tedious. I do think it would be a good idea to allow for this be a user option.

@timusus

This comment has been minimized.

Copy link
Owner

commented Oct 12, 2017

Yeah, I definitely don't think the solution is an additional shuffle mode. I might change it to album shuffle (sorted by track number) as that's my personal preference, and see what the beta community think.

@willcoughlin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2017

Thank you for the feedback! I will work on these changes as soon as I get the chance.

@timusus

This comment has been minimized.

Copy link
Owner

commented Oct 13, 2017

There seems to be an issue when 'track shuffle' is on, where selecting a track to play ends up playing the wrong track.

  1. Turn 'track shuffle' on
  2. Click on an album to go to the 'album detail' screen
  3. Choose a song
  4. Observe that a song other than the chosen one plays.

I haven't looked into the cause, but I've noticed that this can also cause the 'mini player' to disappear (perhaps because the MusicService thinks the queue is empty?)

willcoughlin and others added some commits Oct 13, 2017

Fixed a crash when changing RecyclerView span count
When switching from grid to list, and vise-versa, the SpanSizeLookup spanCount was not updated - so the expected spans for an item versus the actual span were out of sync, causing the item to span an incorrect number of columns, or the RV to crash.

Resolves #202

Accidentally approved.

@timusus
Copy link
Owner

left a comment

Changes requested - see comments.

tkashkin and others added some commits Oct 15, 2017

Added bluetooth permission..
Resolves issue where app doesn’t receive connect/disconnect intents - looks like Google changed something under the hood, so blueooth permission is now required to receive such intents.

Resolves #205
Fixed a crash when changing RecyclerView span count
When switching from grid to list, and vise-versa, the SpanSizeLookup spanCount was not updated - so the expected spans for an item versus the actual span were out of sync, causing the item to span an incorrect number of columns, or the RV to crash.

Resolves #202

@willcoughlin willcoughlin force-pushed the willcoughlin:shuffle-albums branch from 3e330e0 to cd241a2 Oct 16, 2017

@willcoughlin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2017

All requested changes have been addressed.

@timusus
Copy link
Owner

left a comment

Thanks for your work.

It's getting close. There are a couple of tricky RX issues which need to be fixed up.


return newShuffleList;

// if (doPreShuffle) Collections.shuffle(songs);

This comment has been minimized.

Copy link
@timusus

timusus Oct 22, 2017

Owner

Need to remove/cleanup out-commented code

}

private static List<Song> albumShuffleify(List<Song> songs) {
/** Shuffles a list of songs, grouping by album and arranging songs in that album in order. */
private List<Song> albumShuffleSongs(List<Song> songs, boolean isAddingToQueue, boolean doPreShuffle) {

This comment has been minimized.

Copy link
@timusus

timusus Oct 22, 2017

Owner

doPreShuffle is unused

if (startAtFront) {
playPos = 0;
} else {
playPos = currentSong.getAlbum().getSongsSingle().blockingGet().indexOf(currentSong);

This comment has been minimized.

Copy link
@timusus

timusus Oct 22, 2017

Owner

Calling blockingGet() is something I really advise against here - with reactive programming, I think it's always good to assume a reactive stream could potentially run for longer than you expect - this will block the entire thread until completion. It's almost never correct to call blocking on RX, in my opinion.

I don't have a good alternative off the top of my head. The makeAlbumShuffleList() method (and the other, similar methods) might need to be refactored to return a List via a handler - but then we'll have to ensure any code that needs to be executed after makeAlbumShuffleList() is executed within the handler.

If you're having trouble refactoring this, let me know and I'll take a look myself.

There's also the possibility that currentSong is null, so we need to add a null check.

@@ -1696,57 +1613,48 @@ public void makeTrackShuffleList() {
}
}

public void makeAlbumShuffleList() {
public void makeAlbumShuffleList(boolean startAtFront) {

This comment has been minimized.

Copy link
@timusus

timusus Oct 22, 2017

Owner

What is startAtFront? This method could do with some javadoc I reckon.

@@ -1085,6 +1029,72 @@ void saveQueue(final boolean full) {
editor.apply();
}

/** Converts a playlist to a String which can be saved to SharedPrefs */

This comment has been minimized.

Copy link
@timusus

timusus Oct 22, 2017

Owner

Perfect, thanks so much for this!

albumShuffleList.addAll(trackShuffleListAlbumMap.get(album));
}
private List<Song> albumQueueIntersect(Album album, List<Song> albumSongsInQueue) {
List<Song> albumSongsInOrder = new ArrayList<>(album.getSongsSingle().blockingGet());

This comment has been minimized.

Copy link
@timusus

timusus Oct 22, 2017

Owner

Another blockingGet() call which should be refactored, or we'll block the entire thread. I would make albumQueueIntersect return a Single<List<Song>> instead of List<Song>.

@timusus timusus merged commit 1cd1002 into timusus:dev Oct 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.