Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

User favorite status synced across app (fixes #659) #731

Merged
merged 3 commits into from
Apr 11, 2016

Conversation

dgattey
Copy link
Contributor

@dgattey dgattey commented Apr 11, 2016

So, user liking of a song was pretty inconsistent before this PR. I made two large changes to make this better, inspired by #659, which this PR fixes part of.

First of all, if a user had liked a song before playing it, that status never showed up in the player at the bottom. It always showed a non-red heart until the user liked it (again). I get around this by using the util function for updating user_favorite when playing a new song.

Second, if you had a song playing and you liked it from the queue, stream, playlist, etc, it wouldn't update the heart in the player. And if you liked it from the player, it wouldn't change state on the stream, playlist, etc. There was no sharing of state or knowledge between views. I added a $broadcast notification on any like or dislike of a song to fix this. The song directive handles changing the status of user_favorite when it gets it, and root scope handles removing/adding to the likes cache in utilService.

This was extensively tested, from every state I could think of, and it seems to work consistently! State is shared for likes. This would all be much easier if there was one type of song data model that had state, and we saved that song model somewhere, only ever using that one object to read or write changes to. Something to think about for the future.

One more state-related bug this fix exposed is that liking a song from the player/queue doesn't update the count of the likes on the stream/playlist/etc page itself, as those pages use a $scope.count variable to keep track of counts (see favoriteSongDirective to see where this is being changed). IMO, we should be using the data's count property itself on those pages and then the notification handler in the song directive can also modify count values, rather than changing $scope.count where we call the API in favoriteSongDirective. But that requires a bit of refactoring, so it will be done in a future commit.

@dgattey
Copy link
Contributor Author

dgattey commented Apr 11, 2016

Also fixes the last comment of #722 (not the main issue of 722)

@dgattey
Copy link
Contributor Author

dgattey commented Apr 11, 2016

Fixes #709

@dgattey
Copy link
Contributor Author

dgattey commented Apr 11, 2016

Fixes #607

@weblancaster
Copy link
Member

Awesome work @dgattey and thanks for putting everything clear.. that helped me to review much faster.

@weblancaster weblancaster merged commit bbd24b3 into Soundnode:master Apr 11, 2016
@dgattey dgattey deleted the fix-659 branch April 11, 2016 16:06
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants