first step of optimization after the introduction of the dedicated proxy model.
already cleans up a lot of the interface and makes the Qml code easier. Performance does not seem to change much.
no functional change intended. all tests pass
Details
Details
- Reviewers
mgallien - Group Reviewers
Elisa - Commits
- R255:4d6d83218faf: move MediaPlaylist to C++
Diff Detail
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.
Comment Actions
Next step is to move ManageAudioPlayer and maybe the persistent settings to c++. One question inline.
| src/qml/ElisaMainWindow.qml | ||
|---|---|---|
| 99 | Is this a leftover or actually needed somewhere? | |
Comment Actions
This is a quite big review. I would be able to review faster if they were smaller.
I am not quite convinced by the way we go from replaceAndPlay to asking the audio player to play. As said in my comment, we should strive to avoid crossing the c++/qml frontier as much as possible. This is more efficient and better for the performance and reactivity.
| src/mediaplaylist.cpp | ||
|---|---|---|
| 521–537 | You changed the position of the methods. This makes it very hard to review in Phabricator. Could you move it to their old positions ? | |
| src/qml/ElisaMainWindow.qml | ||
| 99 | This was needed by the UPnP support. It is probably safe to remove it. | |
| src/qml/MediaAllTracksView.qml | ||
| 83–87 | I am not very happy of this change. It may be a good idea to move some event handling code to c++ only but this modification goes from qml to c++ and back to qml and back to c++. This is not very efficient. It would probably be a good idea to keep this away from this diff and do it in a next review. | |
Comment Actions
I'll try to reduce the scope of changes for next reviews, but it's really hard if moving complete class from Qml to C++ (with the exception of the code reordering).
| src/mediaplaylist.cpp | ||
|---|---|---|
| 521–537 | Yeah, sorry about that. But I will do the opposite. Reorder it in master and then update this diff, since the methods should really be grouped like this. | |
| src/qml/MediaAllTracksView.qml | ||
| 83–87 | My aim with this change is actually to avoid exactly that. Before, the proxy models were in C++, and the media playlist constructed in Qml. Note that I would like to move the audio control manager also to C++ as stated in the first post. Then this would be in pure C++. | |
Comment Actions
I have compiled it with clazy and it reported one problem:
../src/models/abstractmediaproxymodel.cpp:85:5: warning: Missing emit keyword on signal call MediaPlayList::ensurePlay [-Wclazy-incorrect-emit]
this->mediaPlayList()->ensurePlay();
Can you fix it ?
Comment Actions
If you're fine with the approach, sure. I think it is somewhat incompatible with the changes ahead but it probably just makes a difference to who does the adaption :)