Created attachment 245760 [details] [review]
Initial patch, don't commit

Oops, wrong one...

Created attachment 245787 [details] [review]
Initial patch, don't commit

Rebased against bgo# 701376.

Review of attachment 245787 [details] [review]:

Thanks for your patch, but current default behaviour is correct - repeat is off be deault. Could you update your patch - this should check 'repeat' variable?

(In reply to comment #3)
> Review of attachment 245787 [details] [review]:
> 
> Thanks for your patch, but current default behaviour is correct - repeat is off
> be deault. Could you update your patch - this should check 'repeat' variable?

Okay.

By the way, is there a plan already on how to implement shuffle? Replay Song and Replay All are quite easy to implement, but shuffle is quite problematic - I think ways of doing it are:

- Create a model (GtkTreeModelSort?) with order randomized when a playlist is
  set. This way prev and next is sure to work properly, though I don't know if
  it will have performance issues. (I think the behavior is same with Rhythmbox
  here, don't sure how it is implemented)
- Randomly select a song on next (unless cached), cache previous songs. Maybe an
  array or list for the cache.

I want to know because I think it would be better if shuffle and replay will be
implemented at the same time.

Created attachment 245851 [details] [review]
Updated patch

I implemented all repeat mode except shuffle (the code assumes that if it is not repeat all or repeat none, then it's repeat song). Note that this is still not accessible in the user interface - you have to modify the default repeat mode in the source code to test.

The patch depends on the patch of bgo #701461;

Created attachment 245856 [details] [review]
Patch rebased

(In reply to comment #4)
> (In reply to comment #3)
> > Review of attachment 245787 [details] [review] [details]:
> By the way, is there a plan already on how to implement shuffle? 
> - Create a model (GtkTreeModelSort?) with order randomized when a playlist is
>   set. This way prev and next is sure to work properly, though I don't know if
>   it will have performance issues. (I think the behavior is same with Rhythmbox
>   here, don't sure how it is implemented)
> - Randomly select a song on next (unless cached), cache previous songs. Maybe
> an
>   array or list for the cache.
> 
> I want to know because I think it would be better if shuffle and replay will be
> implemented at the same time.

I think we should randomly select the file for shuffle, but a) this needs a separate bug, b) needs to be discussed with designers/developers

By the way, is the expected behavior when you click next and the repeat mode is repeat song, is to replay the current song?

Also, I haven't worked on the previous buttons yet.

I agree that shuffle needs some discussions with the designers and developers so that we would know the expected behavior. For example, what happens when we click previous then next, should we play again the track before the previous button was clicked or select a new one, something like that.

Created attachment 246203 [details] [review]
Connects the Repeat buttons with the respective behavior

Connects the menubuttons for Repeat Song and Repeat All to the respective behaviors in context to the current playlist.

Review of attachment 246203 [details] [review]:

A couple of bugs found in this implementation:
* after state is changed, playback jumps to the first song
* setting Repeat All should enable Next track button if the last song is reached
* Repeat Song doesn't seem to work

(In reply to comment #10)
> Review of attachment 246203 [details] [review]:
> 
> A couple of bugs found in this implementation:
> * after state is changed, playback jumps to the first song
> * setting Repeat All should enable Next track button if the last song is
> reached
> * Repeat Song doesn't seem to work

Okay, fixing these.

Created attachment 246363 [details] [review]
Probable code for repeat logic (Not ready for commit yet)

To fix the issues with the previous patch.
There might be an issue with gtk+ wherein the callbacks for the repeat button might not be called for some builds. (Yet to see about that)

Created attachment 246466 [details] [review]
Combined commits patch for RepeatALL

Implementation of the RepeatALL functionality.

Created attachment 246471 [details] [review]
Corrected Patch 

Attached the wrong patch previously.

Created attachment 246476 [details] [review]
Rebased patch to implement Repeat ALL feature

Rebasing multiple commits, implements repeat ALL

Review of attachment 246476 [details] [review]:

Coding style review. I also noticed that the previous button is not handled.

::: src/player.js
@@ +89,2 @@
                 this.player.set_state(Gst.State.NULL);
+                this.load( this.playlist.get_value( this.currentTrack, this.playlistField));

Remove spaces after the parentheses.

@@ +95,3 @@
+                }
+                else {
+                    this.progressScale.set_value(0);

Code duplication. Move similar code outside if..else block.

@@ +101,3 @@
+                    this.playBtn.set_image(this._pauseImage);
+		}
+                 

Whitespace error in this empty line.

@@ +135,3 @@
+            if (RepeatType.ALL == this.repeat) {
+                this.currentTrack = this.playlist.get_iter_first()[1];
+                this.load( this.playlist.get_value( this.currentTrack, this.playlistField));

Remove spaces after parentheses.

@@ +141,3 @@
+                this.timeout = GLib.timeout_add(GLib.PRIORITY_DEFAULT, 1000, Lang.bind(this, this._updatePositionCallback));
+            }
+            else 

Whitespace error in this line.

@@ +142,3 @@
+            }
+            else 
+                this.currentTrack=null;

Space before and after equal.

Review of attachment 246476 [details] [review]:

Repeat All works correctly, yet Arnel's style nitpicks should be fixed. Also, an exception on startup:
(gnome-music:24748): Gjs-WARNING **: JS ERROR: TypeError: this.currentTrack is null
Player<._onShuffleRepeatOffActivated@/opt/gnome/share/gnome-music/player.js:441
wrapper@/opt/gnome/share/gjs-1.0/lang.js:213
Player<._setupView@/opt/gnome/share/gnome-music/player.js:341
wrapper@/opt/gnome/share/gjs-1.0/lang.js:213
Player<._init@/opt/gnome/share/gnome-music/player.js:115
wrapper@/opt/gnome/share/gjs-1.0/lang.js:213
_Base._construct@/opt/gnome/share/gjs-1.0/lang.js:154
Class._construct/newClass@/opt/gnome/share/gjs-1.0/lang.js:248
MainWindow<._setupView@/opt/gnome/share/gnome-music/window.js:58
wrapper@/opt/gnome/share/gjs-1.0/lang.js:213
MainWindow<._init@/opt/gnome/share/gnome-music/window.js:49
wrapper@/opt/gnome/share/gjs-1.0/lang.js:213
Application<.vfunc_activate@/opt/gnome/share/gnome-music/application.js:112
wrapper@/opt/gnome/share/gjs-1.0/lang.js:213
main@/opt/gnome/share/gnome-music/main.js:28
start@/opt/gnome/share/gnome-music/package.js:154
@/opt/gnome/bin/gnome-music:3

Created attachment 246568 [details] [review]
Rebased patch to implement Repeat ALL feature

Also tackles the previous button unlike the previous patch.

Created attachment 246569 [details] [review]
Handles the startup error for last patch and implements Repeat All

Review of attachment 246569 [details] [review]:

Good job :D

::: src/player.js
@@ +91,2 @@
                 this.player.set_state(Gst.State.NULL);
+                this.load(this.playlist.get_value(this.currentTrack,this.playlistField));

You forgot a space here after comma :D

@@ +436,3 @@
         this.repeatBtnImage.set_from_icon_name('media-playlist-consecutive-symbolic', 1);
         this.repeat = RepeatType.NONE;
+        if(this.currentTrack) {

Contents of this code block should be indented.

Created attachment 246573 [details] [review]
Fixing code style errors

Implements repeat ALL (fixed code style errors and in sync with previous reviews)

Created attachment 246574 [details] [review]
Implements RepeatALL and fixes some coding style errors

Review of attachment 246574 [details] [review]:

Thanks, functionality works fine for me, here's some nitpicks on the code:

::: src/player.js
@@ +91,2 @@
                 this.player.set_state(Gst.State.NULL);
+                this.load(this.playlist.get_value(this.currentTrack, this.playlistField));

Use 'GLib.idle_add(GLib.PRIORITY_HIGH, Lang.bind(this,this.load,media));' here (see removed part of code)

@@ +97,3 @@
+                else {
+                    this.player.set_state(Gst.State.PLAYING);
+                    //this.onProgressScaleChangeValue(this.progressScale);

Please remove comments from the code

@@ +419,3 @@
         this.repeatBtnImage.set_from_icon_name('media-playlist-repeat-symbolic', 1);
         this.repeat = RepeatType.ALL;
+        let tmp = this.currentTrack.copy();

I don't think this is needed - next and prev buttons should always be enabled

Created attachment 246622 [details] [review]
Implements RepeatALL 

In sync with the latest reviews for the patch

Comment on attachment 246622 [details] [review]
Implements RepeatALL 

Thanks, looks good, pushed.

Keeping the bug open, as 'Repeat Song' and 'Shuffle' is not yet implemented

The following fix has been pushed:
59df315 Implement Repeat All functionality

Created attachment 246623 [details] [review]
Implement Repeat All functionality

Implemented all repeats, pushed as 7cb273e in master