Fixing a lot of the logic behind play/pause button state.

Especially surrounding seeking it was really weird and would flicker at weird times. It should run a lot more smoothly now!
MeoMix · May 12, 2015 · 4f2ee5c · 4f2ee5c
1 parent 8a587c9
commit 4f2ee5c
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 21 deletions.
diff --git a/Streamus Chrome Extension.csproj b/Streamus Chrome Extension.csproj
@@ -80,6 +80,8 @@
     <Content Include="src\js\foreground\model\mediaSourceWrapper.js" />
     <Content Include="src\js\foreground\model\sourceBufferWrapper.js" />
     <Content Include="src\js\foreground\enum\keyboardKey.js" />
+    <Content Include="src\js\foreground\view\dialog\aboutStreamusDialogView.js" />
+    <Content Include="src\js\foreground\view\dialog\aboutStreamusView.js" />
     <Content Include="src\js\foreground\view\element\simpleMenuItemsView.js" />
     <Content Include="src\js\foreground\view\leftPane\activePlaylistAreaView.js" />
     <Content Include="src\js\foreground\view\leftPane\leftPaneRegion.js" />
@@ -90,9 +92,15 @@
     <Content Include="src\js\foreground\view\listItemButton\extraActionsButtonView.js" />
     <Content Include="src\js\foreground\view\video\videoRegion.js" />
     <Content Include="src\js\foreground\view\video\videoView.js" />
+    <Content Include="src\js\test\background\backgroundTests.js" />
+    <Content Include="src\js\test\background\collection\searchResultsTests.js" />
+    <Content Include="src\js\test\common\commonTests.js" />
+    <Content Include="src\js\test\foreground\foregroundTests.js" />
+    <Content Include="src\js\test\foreground\view\search\searchViewTests.js" />
     <Content Include="src\js\thirdParty\test\chai.js" />
     <Content Include="src\js\thirdParty\test\mocha.js" />
     <Content Include="src\js\thirdParty\test\sinon.js" />
+    <Content Include="src\template\dialog\aboutStreamus.html" />
     <Content Include="src\template\icon\closeIcon_24.svg" />
     <Content Include="src\template\icon\deleteIcon_18.svg" />
     <Content Include="src\template\icon\menuIcon_24.svg" />
@@ -304,7 +312,6 @@
     <Content Include="src\js\contentScript\beatport.js" />
     <Content Include="src\js\contentScript\youTubeIFrame.js" />
     <Content Include="src\js\contentScript\youTube.js" />
-    <Content Include="src\js\test\background\collection\playlistsTests.js" />
     <Content Include="src\js\test\background\model\clientErrorManagerTest.js" />
     <Content Include="src\js\test\background\model\signInManagerTests.js" />
     <Content Include="src\js\test\background\model\userTests.js" />

diff --git a/src/js/background/model/player.js b/src/js/background/model/player.js
@@ -22,7 +22,9 @@ define(function(require) {
                 currentLoadAttempt: 1,
                 //  TODO: maxLoadAttempts isn't DRY with YouTubePlayer.
                 maxLoadAttempts: 10,
+                previousState: PlayerState.Unstarted,
                 state: PlayerState.Unstarted,
+                seeking: false,
                 //  This will be set after the player is ready and can communicate its true value.
                 //  Default to 50 because having the music on and audible, but not blasting, seems like the best default if we fail for some reason.
                 volume: 50,
@@ -112,7 +114,8 @@ define(function(require) {
         },
 
         toggleState: function() {
-            var playing = this.get('state') === PlayerState.Playing;
+            var state = this.get('state');
+            var playing = state === PlayerState.Playing || state === PlayerState.Buffering;
 
             if (playing) {
                 this.pause();
@@ -162,7 +165,14 @@ define(function(require) {
 
         seekTo: function(timeInSeconds) {
             if (this.get('ready')) {
-                this.get('youTubePlayer').seekTo(timeInSeconds);
+                //  There's an issue in YouTube's API which makes this code necessary.
+                //  If the user calls seekTo to the end of the song then the state of the player gets put into 'ended'
+                //  That's OK, but then calling seekTo to the middle of the song will cause the song to move to the 'playing' state instead of 'paused'
+                if (timeInSeconds === this.get('loadedSong').get('duration')) {
+                    this.activateSong(this.get('loadedSong'), timeInSeconds);
+                } else {
+                    this.get('youTubePlayer').seekTo(timeInSeconds);
+                }
             } else {
                 this.set({
                     currentTime: timeInSeconds,
@@ -233,6 +243,7 @@ define(function(require) {
         },
 
         _onChangeState: function(model, state) {
+            console.log('new state:', state);
             if (state === PlayerState.Playing || state === PlayerState.Buffering) {
                 this._clearRefreshAlarm();
             } else {
@@ -286,20 +297,6 @@ define(function(require) {
                 });
             }
 
-            //  YouTube's API for seeking/buffering doesn't fire events reliably.
-            //  Listen directly to the element for more responsive results.
-            if (!_.isUndefined(message.seeking)) {
-                if (message.seeking) {
-                    if (this.get('state') === PlayerState.Playing) {
-                        this.set('state', PlayerState.Buffering);
-                    }
-                } else {
-                    if (this.get('state') === PlayerState.Buffering) {
-                        this.set('state', PlayerState.Playing);
-                    }
-                }
-            }
-
             if (!_.isUndefined(message.currentTimeHighPrecision)) {
                 //  Event listeners may need to know the absolute currentTime. They have no idea if it is current or not.
                 //  If it is current, still notify them.
@@ -310,6 +307,10 @@ define(function(require) {
                 var error = new Error(message.error);
                 Streamus.channels.error.commands.trigger('log:error', error);
             }
+
+            if (!_.isUndefined(message.seeking)) {
+                this.set('seeking', message.seeking);
+            }
         },
 
         _onChromeCommandsCommand: function(command) {
@@ -335,6 +336,7 @@ define(function(require) {
 
         _onYouTubePlayerChangeState: function(model, youTubePlayerState) {
             var playerState = this._getPlayerState(youTubePlayerState);
+            this.set('previousState', this.get('state'));
             this.set('state', playerState);
         },
 

diff --git a/src/js/background/model/stream.js b/src/js/background/model/stream.js
@@ -253,7 +253,11 @@
             //  Since the player's state is dependent on asynchronous actions it's important to ensure that
             //  the Stream is still in a valid state when an event comes in. The user could've removed songs after an event started to arrive.
             if (!this.get('items').isEmpty()) {
-                if (state === PlayerState.Ended) {
+                var previousState = this.get('player').get('previousState');
+                //  If the user seeks to the end of a song then YouTube will trigger an 'Ended' event, but skipping to the next song does not make sense.
+                var wasPlaying = previousState === PlayerState.Playing || previousState === PlayerState.Buffering;
+
+                if (state === PlayerState.Ended && wasPlaying) {
                     //  TODO: I need to be able to check whether there's an active item or not before calling activateNext.
                     model.set('playOnActivate', true);
                     var nextItem = this.activateNext();

diff --git a/src/js/foreground/view/appBar/playPauseButtonView.js b/src/js/foreground/view/appBar/playPauseButtonView.js
@@ -63,9 +63,25 @@
         _setState: function(enabled, playerState) {
             this.$el.toggleClass('is-disabled', !enabled);
 
-            //  TODO: There's a difference between buffering-->play and buffering-->paused. Don't want to change button when buffering-->paused. How to tell the difference?
-            this.ui.pauseIcon.toggleClass('is-hidden', playerState !== PlayerState.Playing && playerState !== PlayerState.Buffering);
-            this.ui.playIcon.toggleClass('is-hidden', playerState === PlayerState.Playing || playerState === PlayerState.Buffering);
+            //  If the player is playing then the pause icon clearly needs to be shown.
+            var showPauseIcon = playerState === PlayerState.Playing;
+
+            //  However, if the player is buffering, then it's not so simple. The player might be buffering and paused/unstarted.
+            if (playerState === PlayerState.Buffering) {
+                var previousState = this.player.get('previousState');
+
+                //  When seeking it's even more complicated. The seek might result in the player beginning playback, or remaining paused.
+                var wasPlaying = previousState === PlayerState.Playing || previousState === PlayerState.Buffering;
+
+                if (this.player.get('seeking') && !wasPlaying) {
+                    showPauseIcon = false;
+                } else {
+                    showPauseIcon = true;
+                }
+            }
+
+            this.ui.pauseIcon.toggleClass('is-hidden', !showPauseIcon);
+            this.ui.playIcon.toggleClass('is-hidden', showPauseIcon);
         }
     });