{"action":"opened","number":196,"pull_request":{"url":"https://api.github.com/repos/tsquillario/Jamstash/pulls/196","id":27561586,"html_url":"https://github.com/tsquillario/Jamstash/pull/196","diff_url":"https://github.com/tsquillario/Jamstash/pull/196.diff","patch_url":"https://github.com/tsquillario/Jamstash/pull/196.patch","issue_url":"https://api.github.com/repos/tsquillario/Jamstash/issues/196","number":196,"state":"open","locked":false,"title":"Major refactoring: modular player","user":{"login":"Hyzual","id":2051507,"avatar_url":"https://avatars.githubusercontent.com/u/2051507?v=3","gravatar_id":"","url":"https://api.github.com/users/Hyzual","html_url":"https://github.com/Hyzual","followers_url":"https://api.github.com/users/Hyzual/followers","following_url":"https://api.github.com/users/Hyzual/following{/other_user}","gists_url":"https://api.github.com/users/Hyzual/gists{/gist_id}","starred_url":"https://api.github.com/users/Hyzual/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Hyzual/subscriptions","organizations_url":"https://api.github.com/users/Hyzual/orgs","repos_url":"https://api.github.com/users/Hyzual/repos","events_url":"https://api.github.com/users/Hyzual/events{/privacy}","received_events_url":"https://api.github.com/users/Hyzual/received_events","type":"User","site_admin":false},"body":"Hi again !\r\n\r\nSecond giant PR. I wanted to separate this from the Grunt & Karma things, so that it's easier to understand and merge. I kinda wish I could have made it smaller, but I didn't want to submit a PR with half-working things...\r\n\r\n**TL;DR** : I rewrote the player part from scratch in order to make it more modular and therefore make it easier for us to add or remove functionalities.\r\n\r\nSee the result on my server : http://jamstash.furinax.com\r\n\r\nIt's mostly a tech version, on the user-side a few bugs should be fixed but there are no new features.\r\nThe PR fixes different problems :\r\n\r\n## Simplifying error-handling when making requests to Subsonic\r\n\r\n- Adds a generic `subsonicRequest()` that handles protocols and errors. It makes `getRandomSongs`, `getStarredSongs`, `scrobble`, etc. very light : they only deal with getting songs (or scrobbling).\r\n- Fixes #192 . While I was writing this \"wrapper\" to make it easier for me to deal with protocols and errors, I noticed that `ping()` didn't use `$http.JSONP()`. That's probably why it didn't work with protocol JSONP : you have to use that method so that angular can create a callback for Subsonic to use. Anyway, now it should work with any protocol and display errors such as a bad username or missing password to the user.\r\n\r\n## Separation of concerns :  a better architecture through modularity\r\n\r\nI had taken a look at issue #184, and I found it crazily difficult to untangle player-service.js. Everything was mixed in the player service : we used jquery, jplayer, manipulated DOM, called other services, made HTTP requests. We need to keep these separate so we don't end up with 50 dependencies and can add or remove functionnalities more easily.\r\n\r\nIn order to do this, I did the following :\r\n\r\n- New rewritten player service. It manages the queue and no longer depends on the rootScope. If someone wants to do anything like play a song or fill the queue, they can use the player service. The player service does not do anything HTTP-related. Scrobbling songs is delegated to the subsonic service, since we use subsonic to scrobble for us.\r\n- Added a separate controller for the queue. It uses the player service and adds display-related things.\r\n- Added a separate controller for the player controls. it uses the player service.\r\n- Added a separate directive that controls jplayer. We should only manipulate DOM and use jplayer there. We never have to even know jplayer anywhere else. This will make it a lot easier to write a complete jukebox mode : the player service behaves the same way, but talks to a different directive.\r\n- Added a persistence service. its job is to store the playing queue and current song in localStorage. I also added `angular-locker` as a dependency. It performs the checks and error-handling for us, so we can focus on feeding it info and retrieving it.\r\n- Mostly rewritten notifications service. `showNotification()` now takes a song as parameter and it builds the notification itself, so the player-directive doesn't have to depend on utils.\r\n- Added a Page service. It controls the tab title in order to display (or scroll) a song's title. The directive doesn't have to care how the title works, it justs tells it to display a song.\r\n- Removed many things from main-controller, utils, settings, etc. They are all moved somewhere else where it makes more sense\r\n- Added unit-tests for everything above. We currently stand at about 30% of the entire JS code unit-tested. The player controllers, directive, service, etc. are all about 80% unit-tested. Some of the remaining code does not need to be tested (getters, random, etc.). Unit-tests will make it incredibly easier to change or remove things without breaking the rest. If you use `grunt serve`, thanks to Karma you'll always know if something player-related stops working.\r\n- Updated bower dependencies: jplayer, jquery and jquery-ui\r\n\r\nIt fixes various bugs :\r\n- The \"playing\" icon sometimes did not disappear from songs, so we ended up having more than one song marked as \"playing\". I use a different system where instead of modifying the song, I store the current song in the player. Since there can be only one, this bug won't happen.\r\n- The starred auto playlist now correctly appears \"selected\" when clicked. It was something I had forgotten to modify.\r\n\r\n### Functionalities\r\n\r\nHere are the functionalities I rewrote and tested : \r\n- play using the service\r\n- play / pause in the directive\r\n- mute / unmute\r\n- previous track\r\n- next track\r\n- shuffle queue\r\n- empty queue\r\n- load a song but do not play it\r\n- play a song after loading it\r\n- add songs to the queue\r\n- remove songs from the queue\r\n- scrobble a song\r\n- repeat a song\r\n- repeat the playlist\r\n- auto-play playlist : using subsonic to fill the queue with random songs\r\n- load the queue from localStorage\r\n- load track at given position from localStorage\r\n- save queue to localStorage\r\n- save track position to localStorage\r\n- delete queue from localStorage\r\n- delete track from localStorage\r\n- replay if error in jPlayer\r\n- scroll queue to playing song (in queue controller)\r\n- notify the playing song\r\n- play different formats (oga, m4a)\r\n- display the current song as playing in the queue\r\n- scroll the song's title in the tab bar\r\n- open the next album art in fancybox when changing song\r\n- scrobble again when the song is restarted\r\n- play a song in jukebox mode\r\n- pause a song in jukebox mode\r\n\r\nEverything should work the way it did before. I spent a lot of time testing everything, but it's always possible that I missed something. I have removed some debug code too, like writing the song specs to HTML near jplayer. I don't really know if it was useful, but in any case let me know if I removed something important.\r\n\r\nTake all the time you need testing this. I'll keep using it on my server too.\r\nBest regards !","created_at":"2015-01-17T20:20:32Z","updated_at":"2015-01-17T20:20:32Z","closed_at":null,"merged_at":null,"merge_commit_sha":null,"assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/tsquillario/Jamstash/pulls/196/commits","review_comments_url":"https://api.github.com/repos/tsquillario/Jamstash/pulls/196/comments","review_comment_url":"https://api.github.com/repos/tsquillario/Jamstash/pulls/comments/{number}","comments_url":"https://api.github.com/repos/tsquillario/Jamstash/issues/196/comments","statuses_url":"https://api.github.com/repos/tsquillario/Jamstash/statuses/ae44a0826254e7eeb97a5004321d72ac1e77260f","head":{"label":"Hyzual:rewrite_player","ref":"rewrite_player","sha":"ae44a0826254e7eeb97a5004321d72ac1e77260f","user":{"login":"Hyzual","id":2051507,"avatar_url":"https://avatars.githubusercontent.com/u/2051507?v=3","gravatar_id":"","url":"https://api.github.com/users/Hyzual","html_url":"https://github.com/Hyzual","followers_url":"https://api.github.com/users/Hyzual/followers","following_url":"https://api.github.com/users/Hyzual/following{/other_user}","gists_url":"https://api.github.com/users/Hyzual/gists{/gist_id}","starred_url":"https://api.github.com/users/Hyzual/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Hyzual/subscriptions","organizations_url":"https://api.github.com/users/Hyzual/orgs","repos_url":"https://api.github.com/users/Hyzual/repos","events_url":"https://api.github.com/users/Hyzual/events{/privacy}","received_events_url":"https://api.github.com/users/Hyzual/received_events","type":"User","site_admin":false},"repo":{"id":19375158,"name":"Jamstash","full_name":"Hyzual/Jamstash","owner":{"login":"Hyzual","id":2051507,"avatar_url":"https://avatars.githubusercontent.com/u/2051507?v=3","gravatar_id":"","url":"https://api.github.com/users/Hyzual","html_url":"https://github.com/Hyzual","followers_url":"https://api.github.com/users/Hyzual/followers","following_url":"https://api.github.com/users/Hyzual/following{/other_user}","gists_url":"https://api.github.com/users/Hyzual/gists{/gist_id}","starred_url":"https://api.github.com/users/Hyzual/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Hyzual/subscriptions","organizations_url":"https://api.github.com/users/Hyzual/orgs","repos_url":"https://api.github.com/users/Hyzual/repos","events_url":"https://api.github.com/users/Hyzual/events{/privacy}","received_events_url":"https://api.github.com/users/Hyzual/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/Hyzual/Jamstash","description":"HTML5 Music Streamer for Subsonic","fork":true,"url":"https://api.github.com/repos/Hyzual/Jamstash","forks_url":"https://api.github.com/repos/Hyzual/Jamstash/forks","keys_url":"https://api.github.com/repos/Hyzual/Jamstash/keys{/key_id}","collaborators_url":"https://api.github.com/repos/Hyzual/Jamstash/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/Hyzual/Jamstash/teams","hooks_url":"https://api.github.com/repos/Hyzual/Jamstash/hooks","issue_events_url":"https://api.github.com/repos/Hyzual/Jamstash/issues/events{/number}","events_url":"https://api.github.com/repos/Hyzual/Jamstash/events","assignees_url":"https://api.github.com/repos/Hyzual/Jamstash/assignees{/user}","branches_url":"https://api.github.com/repos/Hyzual/Jamstash/branches{/branch}","tags_url":"https://api.github.com/repos/Hyzual/Jamstash/tags","blobs_url":"https://api.github.com/repos/Hyzual/Jamstash/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/Hyzual/Jamstash/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/Hyzual/Jamstash/git/refs{/sha}","trees_url":"https://api.github.com/repos/Hyzual/Jamstash/git/trees{/sha}","statuses_url":"https://api.github.com/repos/Hyzual/Jamstash/statuses/{sha}","languages_url":"https://api.github.com/repos/Hyzual/Jamstash/languages","stargazers_url":"https://api.github.com/repos/Hyzual/Jamstash/stargazers","contributors_url":"https://api.github.com/repos/Hyzual/Jamstash/contributors","subscribers_url":"https://api.github.com/repos/Hyzual/Jamstash/subscribers","subscription_url":"https://api.github.com/repos/Hyzual/Jamstash/subscription","commits_url":"https://api.github.com/repos/Hyzual/Jamstash/commits{/sha}","git_commits_url":"https://api.github.com/repos/Hyzual/Jamstash/git/commits{/sha}","comments_url":"https://api.github.com/repos/Hyzual/Jamstash/comments{/number}","issue_comment_url":"https://api.github.com/repos/Hyzual/Jamstash/issues/comments/{number}","contents_url":"https://api.github.com/repos/Hyzual/Jamstash/contents/{+path}","compare_url":"https://api.github.com/repos/Hyzual/Jamstash/compare/{base}...{head}","merges_url":"https://api.github.com/repos/Hyzual/Jamstash/merges","archive_url":"https://api.github.com/repos/Hyzual/Jamstash/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/Hyzual/Jamstash/downloads","issues_url":"https://api.github.com/repos/Hyzual/Jamstash/issues{/number}","pulls_url":"https://api.github.com/repos/Hyzual/Jamstash/pulls{/number}","milestones_url":"https://api.github.com/repos/Hyzual/Jamstash/milestones{/number}","notifications_url":"https://api.github.com/repos/Hyzual/Jamstash/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/Hyzual/Jamstash/labels{/name}","releases_url":"https://api.github.com/repos/Hyzual/Jamstash/releases{/id}","created_at":"2014-05-02T12:49:45Z","updated_at":"2015-01-17T18:51:11Z","pushed_at":"2015-01-17T18:51:09Z","git_url":"git://github.com/Hyzual/Jamstash.git","ssh_url":"git@github.com:Hyzual/Jamstash.git","clone_url":"https://github.com/Hyzual/Jamstash.git","svn_url":"https://github.com/Hyzual/Jamstash","homepage":"http://jamstash.com","size":2745,"stargazers_count":0,"watchers_count":0,"language":"JavaScript","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"tsquillario:develop","ref":"develop","sha":"3d367ab8edcbd7c1ba3526aaa8915d4f0c20d90d","user":{"login":"tsquillario","id":333300,"avatar_url":"https://avatars.githubusercontent.com/u/333300?v=3","gravatar_id":"","url":"https://api.github.com/users/tsquillario","html_url":"https://github.com/tsquillario","followers_url":"https://api.github.com/users/tsquillario/followers","following_url":"https://api.github.com/users/tsquillario/following{/other_user}","gists_url":"https://api.github.com/users/tsquillario/gists{/gist_id}","starred_url":"https://api.github.com/users/tsquillario/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tsquillario/subscriptions","organizations_url":"https://api.github.com/users/tsquillario/orgs","repos_url":"https://api.github.com/users/tsquillario/repos","events_url":"https://api.github.com/users/tsquillario/events{/privacy}","received_events_url":"https://api.github.com/users/tsquillario/received_events","type":"User","site_admin":false},"repo":{"id":2207402,"name":"Jamstash","full_name":"tsquillario/Jamstash","owner":{"login":"tsquillario","id":333300,"avatar_url":"https://avatars.githubusercontent.com/u/333300?v=3","gravatar_id":"","url":"https://api.github.com/users/tsquillario","html_url":"https://github.com/tsquillario","followers_url":"https://api.github.com/users/tsquillario/followers","following_url":"https://api.github.com/users/tsquillario/following{/other_user}","gists_url":"https://api.github.com/users/tsquillario/gists{/gist_id}","starred_url":"https://api.github.com/users/tsquillario/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tsquillario/subscriptions","organizations_url":"https://api.github.com/users/tsquillario/orgs","repos_url":"https://api.github.com/users/tsquillario/repos","events_url":"https://api.github.com/users/tsquillario/events{/privacy}","received_events_url":"https://api.github.com/users/tsquillario/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/tsquillario/Jamstash","description":"HTML5 Music Streamer for Subsonic","fork":false,"url":"https://api.github.com/repos/tsquillario/Jamstash","forks_url":"https://api.github.com/repos/tsquillario/Jamstash/forks","keys_url":"https://api.github.com/repos/tsquillario/Jamstash/keys{/key_id}","collaborators_url":"https://api.github.com/repos/tsquillario/Jamstash/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/tsquillario/Jamstash/teams","hooks_url":"https://api.github.com/repos/tsquillario/Jamstash/hooks","issue_events_url":"https://api.github.com/repos/tsquillario/Jamstash/issues/events{/number}","events_url":"https://api.github.com/repos/tsquillario/Jamstash/events","assignees_url":"https://api.github.com/repos/tsquillario/Jamstash/assignees{/user}","branches_url":"https://api.github.com/repos/tsquillario/Jamstash/branches{/branch}","tags_url":"https://api.github.com/repos/tsquillario/Jamstash/tags","blobs_url":"https://api.github.com/repos/tsquillario/Jamstash/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/tsquillario/Jamstash/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/tsquillario/Jamstash/git/refs{/sha}","trees_url":"https://api.github.com/repos/tsquillario/Jamstash/git/trees{/sha}","statuses_url":"https://api.github.com/repos/tsquillario/Jamstash/statuses/{sha}","languages_url":"https://api.github.com/repos/tsquillario/Jamstash/languages","stargazers_url":"https://api.github.com/repos/tsquillario/Jamstash/stargazers","contributors_url":"https://api.github.com/repos/tsquillario/Jamstash/contributors","subscribers_url":"https://api.github.com/repos/tsquillario/Jamstash/subscribers","subscription_url":"https://api.github.com/repos/tsquillario/Jamstash/subscription","commits_url":"https://api.github.com/repos/tsquillario/Jamstash/commits{/sha}","git_commits_url":"https://api.github.com/repos/tsquillario/Jamstash/git/commits{/sha}","comments_url":"https://api.github.com/repos/tsquillario/Jamstash/comments{/number}","issue_comment_url":"https://api.github.com/repos/tsquillario/Jamstash/issues/comments/{number}","contents_url":"https://api.github.com/repos/tsquillario/Jamstash/contents/{+path}","compare_url":"https://api.github.com/repos/tsquillario/Jamstash/compare/{base}...{head}","merges_url":"https://api.github.com/repos/tsquillario/Jamstash/merges","archive_url":"https://api.github.com/repos/tsquillario/Jamstash/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/tsquillario/Jamstash/downloads","issues_url":"https://api.github.com/repos/tsquillario/Jamstash/issues{/number}","pulls_url":"https://api.github.com/repos/tsquillario/Jamstash/pulls{/number}","milestones_url":"https://api.github.com/repos/tsquillario/Jamstash/milestones{/number}","notifications_url":"https://api.github.com/repos/tsquillario/Jamstash/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/tsquillario/Jamstash/labels{/name}","releases_url":"https://api.github.com/repos/tsquillario/Jamstash/releases{/id}","created_at":"2011-08-15T00:18:05Z","updated_at":"2015-01-16T14:30:38Z","pushed_at":"2015-01-11T06:13:15Z","git_url":"git://github.com/tsquillario/Jamstash.git","ssh_url":"git@github.com:tsquillario/Jamstash.git","clone_url":"https://github.com/tsquillario/Jamstash.git","svn_url":"https://github.com/tsquillario/Jamstash","homepage":"http://jamstash.com","size":6110,"stargazers_count":173,"watchers_count":173,"language":"JavaScript","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":58,"mirror_url":null,"open_issues_count":20,"forks":58,"open_issues":20,"watchers":173,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/tsquillario/Jamstash/pulls/196"},"html":{"href":"https://github.com/tsquillario/Jamstash/pull/196"},"issue":{"href":"https://api.github.com/repos/tsquillario/Jamstash/issues/196"},"comments":{"href":"https://api.github.com/repos/tsquillario/Jamstash/issues/196/comments"},"review_comments":{"href":"https://api.github.com/repos/tsquillario/Jamstash/pulls/196/comments"},"review_comment":{"href":"https://api.github.com/repos/tsquillario/Jamstash/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/tsquillario/Jamstash/pulls/196/commits"},"statuses":{"href":"https://api.github.com/repos/tsquillario/Jamstash/statuses/ae44a0826254e7eeb97a5004321d72ac1e77260f"}},"merged":false,"mergeable":null,"mergeable_state":"unknown","merged_by":null,"comments":0,"review_comments":0,"commits":39,"additions":2835,"deletions":1415,"changed_files":40}}