Review of attachment 337255 [details] [review]:

Thanks for the patch. The code quality is good except for some minor nitpicks.

The only issue I'm having is that in some testing I can see the slider stutter slightly, like its waiting slightly longer every few ticks. And I had one crash tracing back to this patch (I didn't copy the trace -  my bad). I'll try and get some more substantial feedback.

A tick per second may not be smooth, but at least it is reliable and predictable, this patch needs to be as good or even better to be viable.

::: gnomemusic/player.py
@@ +876,3 @@
+        width = self.progressScale.get_allocated_width()
+        duration = self.get_current_media().get_duration()
+        self.timeout_period = min(1000//math.ceil(width/duration), 1000)

pep8 spacing please

@@ +976,3 @@
         if position > 0:
             self.progressScale.set_value(position * 60)
+            self.played_seconds += self.timeout_period/1000

spacing

Another issue I noticed is that when hovering over the slider dragpoint, it actually stops moving until you stop moving the mouse/touchpad resulting in heavy stutter.

I'm not sure of the reason, maybe b/c the patch disconnects the size-allocate signal at times?

Thanks for reviewing this.  I took a look at the problems you noted, and I must admit I'm puzzled by the last one.  It doesn't seem to be because of disconnecting size-allocate, as removing the code to disable it doesn't fix the problem.  I'll try removing other parts of the patch to isolate the problem.

The slight stuttering might be simply an artifact of the tick rate not being an exact multiple of the time of one pixel, making it occasionally not move after one tick.  It could probably be fixed by making the scale update independently from the playback time label, allowing the scale to update at the correct rate without making the label update wrong.

I haven't experienced any crashes from this myself, but if I get one I'll post the trace.

I found one crash, which may or may not be the one you mentioned.  When playing a song from the favorites playlist, if the current song is removed from the favorites, the patch caused the following crash:

Traceback (most recent call last):

+ Trace 236735

I'll submit another patch momentarily that, among other things, gets the duration of the current song from Player.player rather than Player.get_current_media(), fixing this crash.

Created attachment 337468 [details] [review]
player: Make progress scale move smoothly

The progress scale used to update once every second, which didn't look
very nice when shorter songs were playing.  This patch makes it update
once per second, or once per amount of time represented by one pixel of
the scale, whichever is shorter.  This makes the slider move smoothly
from left to right regardless the length of the song being played.  The
song position label updates once per second and every time the slider
moves, making it tick smoothly and update quickly when the user drags
the slider.

This fixes the problems pointed out by Marinus Schraal in the previous
patch: moving the cursor over the scale no longer freezes the scale,
and the slider moves more evenly as well.

Review of attachment 337468 [details] [review]:

Looks ok to me, some small issues left.

Regarding the git comment: keep it just about the issue (short 'n sweet), not the road to getting there (the bug is reference for that).

::: gnomemusic/player.py
@@ +80,3 @@
     nextTrack = None
     timeout = None
+    seconds_timeout = None

this is an internal state var, so prepend with _

See README, the last bit.

@@ +844,3 @@
+        return False
+
+    def update_timeout(self):

internal

@@ +859,3 @@
+        # starts moving.
+        if width == 1:
+            width = self._parent_window.get_allocated_width()

Is this really needed, when should I see this?

@@ +882,3 @@
+            self.seconds_timeout = None
+
+    def progress_scale_zero(self):

this should be marked as an internal function (prepend with _)

@@ +985,3 @@
+        self._on_progress_value_changed(None)
+
+        position = self.player.query_position(Gst.Format.TIME)[1] / 1000000000

Maybe use 10**9 here, a long line of zeros is unreadable.

Thanks for the review, again.  I'll add a new patch momentarily fixing the issues you pointed out.

@@ +859,3 @@
+        # starts moving.
+        if width == 1:
+            width = self._parent_window.get_allocated_width()

This really is needed, so I'll change the comment before it to explain a bit better.  When that function is first run (i.e. when playing the first song after starting Music), the scale doesn't have the correct area yet.  Without this block, the timeout_period calculation would cause the slider to stay still for one second even if it really shouldn't.  Using the window's width makes the timeout_period somewhat less than it should be, which keeps the slider moving smoothly and gets fixed the next time it moves.

Created attachment 339677 [details] [review]
player: Make progress scale move smoothly

The progress scale used to update once every second, which didn't look
very nice when shorter songs were playing.  This patch makes it update
once per second, or once per amount of time represented by one pixel of
the scale, whichever is shorter.  This makes the slider move smoothly
from left to right regardless the length of the song being played.  The
song position label updates once per second and every time the slider
moves, making it tick smoothly and update quickly when the user drags
the slider.

(In reply to Clayton G. Hobbs from comment #7)
> @@ +859,3 @@
> +        # starts moving.
> +        if width == 1:
> +            width = self._parent_window.get_allocated_width()
> 
> This really is needed, so I'll change the comment before it to explain a bit
<snip>

Ok, I can see what you mean now, but getting the parents width is not really correct either.

I'm not sure I like that much better than having a (near unnoticeable) stutter at the start of the first song.

I wonder where it comes from anyway, the widget is already realized, so the size should be available.

And this is not the actual size of the sliderbar itself (that one should be slightly smaller with some padding involved).

Created attachment 339776 [details] [review]
player: Make progress scale move smoothly

Thinking about what you said, I decided to look into this further and fix the `width == 1` hack.  It turns out the scale really hadn't been realized yet, which was causing the problem.  Some defensive coding to make sure _update_timeout isn't run when progressScale hasn't been realized, plus a signal handler to make sure _update_timeout is run the first time progressScale is drawn, fixed the problem entirely.

While I was at it, I also corrected the width calculation to subtract the scale's left and right padding.

Nothing much left to fault here, thanks for the work.

Attachment 339776 [details] pushed as d72c2d4 - player: Make progress scale move smoothly