Created attachment 247217 [details] [review]
player: don't connect to GstPlaybin::about-to-finish

The signal is emitted from a GStreamer worker thread, but gjs
is not thread safe and cannot run on different threads.

Created attachment 247218 [details] [review]
player: clean up and restructure playlist handling

We won't have get a GstMessage for state NULL, because that state
change is immediate, and the pipeline on its own goes PAUSED, not
NULL, at eos.
We can therefore take advantage of that to make all track changes
explicit in the message::eos, playNext or playPrevious().
There should be no functional changes.

Created attachment 247219 [details] [review]
player: consolidate and cleanup the code handling the repeat button

We can get a lot less code with a GSettingsAction and GMenuModel
watching it. And we get persistency for free.

Review of attachment 247219 [details] [review]:

Thanks, these change make player model much more logical and clear, except the patches introduce two minor bugs:

1) tests (make --check or jhbuild buildone --check) break:
Gjs-Message: JS LOG: Call to assertFalse(boolean) with true

Stack trace follows:
JsUnitException@/opt/gnome/share/gjs-1.0/jsUnit.js:272
_assert@/opt/gnome/share/gjs-1.0/jsUnit.js:110
assertFalse@/opt/gnome/share/gjs-1.0/jsUnit.js:140
testAlbumViewPlayback@tests_albumPlayback.js:136
gjstestRun@/opt/gnome/share/gjs-1.0/jsUnit.js:438
gjstestRun@tests_albumPlayback.js:4
@tests_albumPlayback.js:167

Gjs-Message: JS LOG: 1 tests failed in this file
Gjs-Message: JS LOG: Failures were: testAlbumViewPlayback

Tests assume that repeat is set to off, so checks that previous button is disabled. tests_albumPlayback.js:136 should be removed

2) When last song has finished and Repeat is Off, set selected song to the first in the playlist
* Set repeat to off
* Play last song
* Fast-forward to the end of the song
Result: playback stops, last song is selected
Expected: playback stops, first song in the playlist is selected

Review of attachment 247218 [details] [review]:

Setting repeat mode to all, playing the first song then clicking previous button doesn't play last song.

::: src/player.js
@@ +139,3 @@
+
+        do {
+            last = iter;

This line should be "last = iter.copy()". TreeModel.iter_next invalidates iter if there's no next row.

@@ +148,3 @@
+    _getPreviousTrack: function() {
+        let currentTrack = this.currentTrack;
+        let previousTrack;

Should be "var" since it has no value.

@@ +158,3 @@
+            previousTrack = currentTrack.copy();
+            if (!this.playlist.iter_previous(previousTrack))
+                nextTrack = this._getIterLast();

Typo. nextTrack should be previousTrack.

Review of attachment 247218 [details] [review]:

With repeat mode set to off, first song is not loaded after the last song is played.

::: src/player.js
@@ +94,3 @@
+                    this.currentTrack = nextTrack;
+                    this.play();
+                }));

Adding:
            } else if (this.repeat == RepeatType.NONE) {
                this.stop();
                this.playBtn.set_image(this._playImage);
                this.progressScale.set_value(0);
                this.progressScale.sensitive = false;
                this.currentTrack = this.playlist.get_iter_first()[1];
                this.load(this.playlist.get_value(this.currentTrack, this.playlistField));

is a possible way of loading first track when last song is played on repeat off.

Thanks, applied all patches, fixed 'Repeat Off on the last track' behaviour and updated tests