GNOME Bugzilla – Bug 701461
Progress scale jumps back and forth when the next track is shorter
Last modified: 2013-06-16 09:09:25 UTC
Created attachment 245849 [details] [review] Patch When the next track is shorter, the progress scale jumps from end, to start, to end then to start again. This is because the track metadata is loaded when the state changes to NULL, but when that happens, we are not sure if the stream is indeed loaded already. I attached a patch that changes the loading during stream-start signal. I also removed setting the player state to PLAYING because we are now sure that we are indeed already in this state. Removed the changing of position to start since we are already in the start inside the signal (this may also cause the jumping if not removed). Then renamed loadNextTrack since it no longer loads the next track, just moves it to the next one.
Created attachment 245855 [details] [review] Patch
Review of attachment 245855 [details] [review]: moveNextTrack should not be called in 'about-to-finish' as this event is called 2-3 seconds before the actual song change. I do agree with the bug itself, but the patch should handle progress bar correctly
Should I move it to stream-changed then? But I don't have a way to know if the stream was started by about-to-finish, by selecting a new song in the views, or clicking previous or next. It only moves the iter to the next item though, so I think it doesn't affect the progress bar.
Created attachment 245926 [details] [review] Updated patch (don't commit!) The message is not yet updated, so don't commit yet.
Review of attachment 245926 [details] [review]: This patch also doesn't play the next song after the current one ends ::: src/player.js @@ +79,3 @@ + GLib.idle_add(GLib.PRIORITY_HIGH, Lang.bind(this, + function() { + log("State changed to null"); Please remove debug output from the patch @@ -119,3 @@ - loadNextTrack: function(){ - if (this.timeout) { - GLib.source_remove(this.timeout); If this code is removed, we might possibly have problems with leaking timeouts during pause. Is this handled? @@ +199,3 @@ this.load( this.playlist.get_value( this.currentTrack, this.playlistField)); + let media = this.playlist.get_value( this.currentTrack, this.playlistField); Code duplication: 'this.playlist.get_value( this.currentTrack, this.playlistField)' Please put this before 'this.load'
Created attachment 246095 [details] [review] Updated patch Updated function names, and fixes errors found in the review. (In reply to comment #5) > Review of attachment 245926 [details] [review]: > > This patch also doesn't play the next song after the current one ends > Haven't recheck the old patch attached as 245926, but this new patch works for me. > ::: src/player.js > @@ +79,3 @@ > + GLib.idle_add(GLib.PRIORITY_HIGH, Lang.bind(this, > + function() { > + log("State changed to null"); > > Please remove debug output from the patch > Oops, forgot that one. Removed. > @@ -119,3 @@ > - loadNextTrack: function(){ > - if (this.timeout) { > - GLib.source_remove(this.timeout); > > If this code is removed, we might possibly have problems with leaking timeouts > during pause. Is this handled? > I moved it to message:stream-start, and I think moveNextTrack is no longer (or not) called when the song is paused. > @@ +199,3 @@ > this.load( this.playlist.get_value( this.currentTrack, > this.playlistField)); > > + let media = this.playlist.get_value( this.currentTrack, > this.playlistField); > > Code duplication: 'this.playlist.get_value( this.currentTrack, > this.playlistField)' > Please put this before 'this.load' Fixed.
Created attachment 246096 [details] [review] Updated commit message
Looks good to me. But can you rename moveNextTrack back to loadNextTrack?
Created attachment 246373 [details] [review] Updated patch Thanks Vadim and Seif for reviewing the patches. I updated it again. (In reply to comment #8) > Looks good to me. But can you rename moveNextTrack back to loadNextTrack? Fixed.
Origin report was mostly fixed, the only case is now described in bug 702352