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