Code review comment for lp:~music-app-dev/music-app/media-hub-bg-playlists-rework

Revision history for this message
Andrew Hayzen (ahayzen) wrote :

1) Is there a reason this length of time was chosen?

48ms is the maximum possible time we can wait without being noticeable to the user (less than 50ms) that is still divisible by 16ms. (note that this worked when set to 16ms on mako to me, I just wanted to give extra time to slower devices)

2) Hasn't this bug been fixed and released?

Nope, using print_tree I could not see the MediaPlayer or the property var :-/ (Would be nice if it was though :-) )

3) Consider changing to a switch statement. Since there are only 3 conditions it isn't too bad though.

I'm not sure that can be a switch statement? What would be the control ? if you did switch(settings.shuffle) you couldn't have a case for repeat? I think it reads fine as an if, else if, else

4) Could you change "bgplaylists" to something more specific? MediaPlayer's playlist/queue or something?

Fixed.

I also noticed that the positionAt currentIndex was broken when switching to the queue view, currently investigating could be due to the Layouts change as then the height isn't 'fixed'.

« Back to merge proposal