Code review comment for lp:~samuel-buffet/entertainer/reactive_videoOSD

>> * 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.

Okay, that's fine with me. I'd rather see this branch land in a mostly working state with a couple of minor issues than to hold it back until it's absolutely perfect.

>> * 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!)

Thanks for applying this fix anyway. It may not seem like much, but since the level of effort to fix it was minimal, I think it was worth it in this case.

>> * 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.

Okay, that helped a lot. I agree that it would be bad if users were able to pause/seek on other screens (outside of keyboard methods, of course).

>> * 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.

Ah, sorry, you're right. For some reason, I thought the position checking stuff was just a few lines above this section of code. I still don't know what happens in the seek backwards case (it seems to me like you'd want to decrement instead of increment), but I'm fine with this.

I understand about not being able to get tests in place. I wonder if there is any way we could simulate/mock events in a future branch.