Make MediaPlayList responsible for remaining tracks
ClosedPublic

Authored by astippich on Jul 27 2019, 8:08 AM.

Details

Summary

Make MediaPlayList responsible for remaining tracks.
Allows simplification by only reacting to currentTrack
changes inside manageheaderbar.
Add tests for remainingTracks and cleanup manageheaderbar.
Fixes the remaining tracks showing when in playlist repeat
or shuffle mode.

Test Plan

tests pass
Remaining tracks are only showed for when not in
shuffle or repeat mode

Diff Detail

Repository
R255 Elisa
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
astippich requested review of this revision.Jul 27 2019, 8:08 AM
astippich created this revision.

Could you please add a general overview of what is the goal of this change ?

As is, I am under the impression that you want to remove ManageHeaderBar without explaining why.

Please see my inline comments.

src/manageheaderbar.cpp
365

Unrelated change

src/mediaplaylist.cpp
1024

What is the purpose of this ?

I do not understand why this change is needed and fear that this will introduce a loop.

1307

Code style

src/qml/ElisaMainWindow.qml
186

As is, your patch is somehow pushing for a removal of ManageHeaderBar class that would serve no purpose.

I am not sure that is desirable though.

Could you please explain in the commit message what is the reason for such changes ?

src/qml/HeaderBar.qml
326

This will display "0 tracks remaining". That is not good.

Please put back the previous behavior or fix the i18n call to have a correct way to say "no remaining tracks".

329

Not sure we really want this change but can you make it ">= 0" as this is easier to understand.

I forgot to say that the very big reduction in line count is really good. Thanks for that

I don't want to remove ManageHeaderBar, on the contrary. This is a simplification and cleanup in order to easily extend it in the future after D22771, which is the goal of this whole series.
While working on it, I found that the ManageHeaderBar is needlessly entangled to the MediaPlayList by connecting to all of those signals, but which are mostly ignored. This is just because of the remaining tracks, for which the MediaPlayList is the correct place to retrieve the information.
By only connecting to the currentTrackChanged signal, the rest can be implemented much more easily. D22771 is working towards T7674

src/mediaplaylist.cpp
1024

The current index needs to be re-emitted after current track has changed. The index will stay the same, but the data has changed. So we need to signal the manageHeaderBar to reload.

astippich updated this revision to Diff 63675.Aug 13 2019, 6:47 PM
astippich marked 3 inline comments as done.
  • fix code style and unrelated changes
  • fix remaining tracks label
astippich updated this revision to Diff 63678.Aug 13 2019, 6:59 PM
  • fix code style again
astippich updated this revision to Diff 63836.Aug 15 2019, 6:43 PM
  • rebase on master
mgallien accepted this revision.Aug 18 2019, 7:46 PM

OK, thanks

This revision is now accepted and ready to land.Aug 18 2019, 7:46 PM
This revision was automatically updated to reflect the committed changes.