Matt,

> main.py:
>  * The connect to 'refresh' could now be one line.

Modified.



> progress_bar.py:
>  * I found the two lines `self._progess = None; self.progress = 0` to be a bit
> confusing. I realize now that the second line is actually calling the property
> setter function, but it wasn't very clear to me that that was happening until
> I hunted around. Could you please add a comment there to help remove that
> confusion? Something like "# Call the property setter." between the two lines.

I have added a comment there to explain that I set the progress property to initialize the widget.



>  * _reset_auto_display_timeout docstring has odd spacing and is missing a
> period.

Yes, Fixed.



>  * The progress bar does not update its position after a seek while the media
> was paused. I mentioned this to you on IRC.

Yes you're right. Unfortunatly, fix is not as trivial as expected at the widget level. I think that's because we do to much gstreamer queries. I plan to make another branch for that if you're okay.



> media_player.py:
>  * _internal_timer_callback docstring doesn't really make sense anymore
> (because it's not just emitting a position-changed event).

Right, I've modified that docstring.



>  * The amount of calls to _internal_timer_callback and the frequency at which
> they occur kind of bothers me. A media player object is created when
> Entertainer starts and the way this init is written means that we *never* stop
> calling that timer (which fires every 1/5 of a second). That seems pretty
> resource intensive to me. Any ideas of how to make this timer only run when
> media is *actually* playing (because I think that is the only time when it is
> really needed)?

Well, I don't really see the point here because I think we don't have to be afraid of timers like that. The tasks done here are very*100 limited for a CPU. But the fix is trivial so I've done the modification (but it's an added complexity!)



>  * Is the is_reactive property logically backwards? I must confess that I am
> confused by it. In the init, self.is_reactive is set to False, then a few
> lines later, self.video_texture.set_reactive(True). I don't get it.

Yeah, I've renamed it to is_reactive_allowed to limit confusion. This variable is used to neutralize the reactive commands when the video texture of the player is on the background on other screen but video_osd screen.

Indeed, it would be a very bad user experience imho if we were able pause/seek on other screens.



>  * The video_texture connects mix " and ' quote marks. Could you pick one or
> the other?

Yep, it's fixed.



>  * position += self._seek_step -> What if you're seeking backwards? Should it
> decrement instead? Also, what is position was 1 and then you just added the
> seek_step to it. Would gstreamer complain?

No in fact because I've protected it against bad values. Check the set_media_position method, I've added test on the position parameter.


> General:
>  * Can you think of any worthwhile tests for some of this?

Yes, that is really something I have in mind. The main issue is that we need to start a clutter main loop during our tests. Otherwise events won't work. Even timeouts are not working in our tests at the moment (you may have noticed). I've started to work on that but that work is not ready yet and goes far beyond the goal of that branch.

Also, I've introduced a bunch of test (regular ones) in 
>> lp:~samuel-buffet/entertainer/volume_management


Samuel-