This diff adds the title, artist and album to the MPRIS network packets. That's useful when you need more detail than just "artist - title", for example in the future media control notification. It also fixes weird song descriptions for empty artist strings (e.g. Spotify uses an empty (but present) artist when playing ads)
Details
Details
- Reviewers
apol - Group Reviewers
KDE Connect - Commits
- R224:71d8eb07c3d5: Add title, artist and album to MPRIS network packets
Diff Detail
Diff Detail
- Repository
- R224 KDE Connect
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| plugins/mpriscontrol/mpriscontrolplugin.cpp | ||
|---|---|---|
| 140 | That doesn't work: I get the following compile error, probably because the argument is templated and not a QString. error: no matching function for call to ‘NetworkPackage::set(QString, <brace-enclosed initializer list>)’ | |
| 240 | Sorry, that shouldn't have been included. I was experimenting with it earlier, but figured that I didn't really need it. Mpris allows three states: "Stopped", "Paused" and "Playing", although a lot of players never report "Stopped". Using the current code you cannot distinguish between "Stopped" and "Paused". Since I don't need it, I removed it in the diff. | |
| plugins/mpriscontrol/mpriscontrolplugin.cpp | ||
|---|---|---|
| 133 | I would rather extract all metadata like: if (nowPlayingMap.contains(QStringLiteral("xesam:foo"))) {
QString foo = nowPlayingMap[QStringLiteral("xesam:foo")].toString();
}
....And compose the nowPlaying at the end. This way all the ifs have the same structure and the artist isn't checked twice. | |
| 135 | Please check whether it's really necessary to set it to an empty string or it's the default case. I'm not sure about that right now :) | |
Comment Actions
Incorporated suggested improvements. The code really is a lot cleaner this way! I think I also fixed a (latent) bug with inconsistent sending of the length of the playing track.
| plugins/mpriscontrol/mpriscontrolplugin.cpp | ||
|---|---|---|
| 135 | It's necessary, since the code is written to send only changes in the network packets. If we don't send an empty artist etc. the Android part will think it's unchanged. With the current code it implicitely sends empty values, which is better. | |
Comment Actions
Looks much better! Those two blocks of code appear the same to me, have you tried factoring them out into a method?
Comment Actions
Not yet, but that shouldn't be too difficult.
Do you want only the part relating to nowPlayingMap? That's functionality-wise identical.
I could also remove nearly the whole MprisControlPlugin::propertiesChanged method, but then the code will make additional DBus calls to fetch all information even if only some properties change.
| plugins/mpriscontrol/mpriscontrolplugin.cpp | ||
|---|---|---|
| 269 | i would prefer something like insertPlayingInformation(NetworkPackage& target, const QVariantMap& source) to make it more clear which one is the source/target. Or make the method name more clear | |