From dca75b7d6a8e60a9c5c93f8c0ee84326aa9f5322 Mon Sep 17 00:00:00 2001 From: outfoxxed Date: Wed, 20 Nov 2024 00:52:47 -0800 Subject: [PATCH] service/mpris: clarify trackinfo emit order and use QBindings --- src/core/util.hpp | 10 +++++ src/services/mpris/player.cpp | 69 +++++++++++++++------------------- src/services/mpris/player.hpp | 71 +++++++++++++++++++++-------------- src/wayland/popupanchor.cpp | 2 +- src/window/popupwindow.cpp | 8 ++-- 5 files changed, 86 insertions(+), 74 deletions(-) diff --git a/src/core/util.hpp b/src/core/util.hpp index 1ff9b22b..5dae122f 100644 --- a/src/core/util.hpp +++ b/src/core/util.hpp @@ -4,6 +4,7 @@ #include #include +#include #include #include @@ -258,3 +259,12 @@ template bool setSimpleObjectHandle(auto* parent, auto* value) { return SimpleObjectHandleOps::setObject(parent, value); } + +// NOLINTBEGIN +#define QS_TRIVIAL_GETTER(Type, member, getter) \ + [[nodiscard]] Type getter() { return this->member; } + +#define QS_BINDABLE_GETTER(Type, member, getter, bindable) \ + [[nodiscard]] Type getter() { return this->member.value(); } \ + [[nodiscard]] QBindable bindable() { return &this->member; } +// NOLINTEND diff --git a/src/services/mpris/player.cpp b/src/services/mpris/player.cpp index 8629d592..a97020b0 100644 --- a/src/services/mpris/player.cpp +++ b/src/services/mpris/player.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -256,19 +257,13 @@ void MprisPlayer::setVolume(qreal volume) { this->pVolume.write(); } -const QVariantMap& MprisPlayer::metadata() const { return this->pMetadata.get(); } - void MprisPlayer::onMetadataChanged() { - emit this->metadataChanged(); - auto lengthVariant = this->pMetadata.get().value("mpris:length"); qlonglong length = -1; if (lengthVariant.isValid() && lengthVariant.canConvert()) { length = lengthVariant.value(); } - auto lengthChanged = this->setLength(length); - auto trackChanged = false; QString trackId; @@ -297,47 +292,43 @@ void MprisPlayer::onMetadataChanged() { } } - auto trackTitle = this->pMetadata.get().value("xesam:title").toString(); - auto trackTitleChanged = this->setTrackTitle(trackTitle.isNull() ? "Unknown Track" : trackTitle); - - auto trackArtists = this->pMetadata.get().value("xesam:artist").value>(); - auto trackArtistsChanged = this->setTrackArtists(trackArtists.join(", ")); - - auto trackAlbum = this->pMetadata.get().value("xesam:album").toString(); - auto trackAlbumChanged = this->setTrackAlbum(trackAlbum.isNull() ? "Unknown Album" : trackAlbum); - - auto trackAlbumArtist = this->pMetadata.get().value("xesam:albumArtist").toString(); - auto trackAlbumArtistChanged = this->setTrackAlbumArtist(trackAlbumArtist); - - auto trackArtUrl = this->pMetadata.get().value("mpris:artUrl").toString(); - auto trackArtUrlChanged = this->setTrackArtUrl(trackArtUrl); - if (trackChanged) { - this->mUniqueId++; - // Some players don't seem to send position updates or seeks on track change. - this->pPosition.update(); emit this->trackChanged(); } - DropEmitter::call( - trackTitleChanged, - trackArtistsChanged, - trackAlbumChanged, - trackAlbumArtistChanged, - trackArtUrlChanged, - lengthChanged - ); + Qt::beginPropertyUpdateGroup(); + + this->bMetadata = this->pMetadata.get(); + + auto trackTitle = this->pMetadata.get().value("xesam:title").toString(); + this->bTrackTitle = trackTitle.isNull() ? "Unknown Track" : trackTitle; + + auto trackArtist = this->pMetadata.get().value("xesam:artist").value>(); + this->bTrackArtist = trackArtist.join(", "); + + auto trackAlbum = this->pMetadata.get().value("xesam:album").toString(); + this->bTrackAlbum = trackAlbum.isNull() ? "Unknown Album" : trackAlbum; + + this->bTrackAlbumArtist = this->pMetadata.get().value("xesam:albumArtist").toString(); + this->bTrackArtUrl = this->pMetadata.get().value("mpris:artUrl").toString(); + + if (trackChanged) { + emit this->trackChanged(); + this->bUniqueId = this->bUniqueId + 1; + + // Some players don't seem to send position updates or seeks on track change. + this->pPosition.update(); + } + + Qt::endPropertyUpdateGroup(); + + this->setLength(length); + + emit this->postTrackChanged(); } -DEFINE_MEMBER_GET(MprisPlayer, uniqueId); DEFINE_MEMBER_SET(MprisPlayer, length, setLength); -DEFINE_MEMBER_GETSET(MprisPlayer, trackTitle, setTrackTitle); -DEFINE_MEMBER_GETSET(MprisPlayer, trackArtists, setTrackArtists); -DEFINE_MEMBER_GETSET(MprisPlayer, trackAlbum, setTrackAlbum); -DEFINE_MEMBER_GETSET(MprisPlayer, trackAlbumArtist, setTrackAlbumArtist); -DEFINE_MEMBER_GETSET(MprisPlayer, trackArtUrl, setTrackArtUrl); - MprisPlaybackState::Enum MprisPlayer::playbackState() const { return this->mPlaybackState; } void MprisPlayer::setPlaybackState(MprisPlaybackState::Enum playbackState) { diff --git a/src/services/mpris/player.hpp b/src/services/mpris/player.hpp index 9e633555..b509980b 100644 --- a/src/services/mpris/player.hpp +++ b/src/services/mpris/player.hpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -125,25 +126,27 @@ class MprisPlayer: public QObject { /// A map of common properties is available [here](https://www.freedesktop.org/wiki/Specifications/mpris-spec/metadata). /// Do not count on any of them actually being present. /// - /// Note that the @@trackTitle, @@trackAlbum, @@trackAlbumArtist, @@trackArtists and @@trackArtUrl + /// Note that the @@trackTitle, @@trackAlbum, @@trackAlbumArtist, @@trackArtist and @@trackArtUrl /// properties have extra logic to guard against bad players sending weird metadata, and should /// be used over grabbing the properties directly from the metadata. - Q_PROPERTY(QVariantMap metadata READ metadata NOTIFY metadataChanged); + Q_PROPERTY(QVariantMap metadata READ metadata NOTIFY metadataChanged BINDABLE bindableMetadata); /// An opaque identifier for the current track unique within the current player. /// /// > [!WARNING] This is NOT `mpris:trackid` as that is sometimes missing or nonunique /// > in some players. - Q_PROPERTY(quint32 uniqueId READ uniqueId NOTIFY trackChanged); + Q_PROPERTY(quint32 uniqueId READ uniqueId NOTIFY trackChanged BINDABLE bindableUniqueId); /// The title of the current track, or "Unknown Track" if none was provided. - Q_PROPERTY(QString trackTitle READ trackTitle NOTIFY trackTitleChanged); + Q_PROPERTY(QString trackTitle READ trackTitle NOTIFY trackTitleChanged BINDABLE bindableTrackTitle); + /// The current track's artist, or an empty string if none was provided. + Q_PROPERTY(QString trackArtist READ trackArtist NOTIFY trackArtistChanged BINDABLE bindableTrackArtist); + /// > [!ERROR] deprecated in favor of @@trackArtist. + Q_PROPERTY(QString trackArtists READ trackArtist NOTIFY trackArtistChanged BINDABLE bindableTrackArtist); /// The current track's album, or "Unknown Album" if none was provided. - Q_PROPERTY(QString trackAlbum READ trackAlbum NOTIFY trackAlbumChanged); + Q_PROPERTY(QString trackAlbum READ trackAlbum NOTIFY trackAlbumChanged BINDABLE bindableTrackAlbum); /// The current track's album artist, or "Unknown Artist" if none was provided. - Q_PROPERTY(QString trackAlbumArtist READ trackAlbumArtist NOTIFY trackAlbumArtistChanged); - /// The current track's artists, or an empty list if none were provided. - Q_PROPERTY(QString trackArtists READ trackArtists NOTIFY trackArtistsChanged); + Q_PROPERTY(QString trackAlbumArtist READ trackAlbumArtist NOTIFY trackAlbumArtistChanged BINDABLE bindableTrackAlbumArtist); /// The current track's art url, or `""` if none was provided. - Q_PROPERTY(QString trackArtUrl READ trackArtUrl NOTIFY trackArtUrlChanged); + Q_PROPERTY(QString trackArtUrl READ trackArtUrl NOTIFY trackArtUrlChanged BINDABLE bindableTrackArtUrl); /// The playback state of the media player. /// /// - If @@canPlay is false, you cannot assign the `Playing` state. @@ -254,7 +257,13 @@ public: [[nodiscard]] bool volumeSupported() const; void setVolume(qreal volume); - [[nodiscard]] const QVariantMap& metadata() const; + QS_BINDABLE_GETTER(quint32, bUniqueId, uniqueId, bindableUniqueId); + QS_BINDABLE_GETTER(QVariantMap, bMetadata, metadata, bindableMetadata); + QS_BINDABLE_GETTER(QString, bTrackTitle, trackTitle, bindableTrackTitle); + QS_BINDABLE_GETTER(QString, bTrackAlbum, trackAlbum, bindableTrackAlbum); + QS_BINDABLE_GETTER(QString, bTrackAlbumArtist, trackAlbumArtist, bindableTrackAlbumArtist); + QS_BINDABLE_GETTER(QString, bTrackArtist, trackArtist, bindableTrackArtist); + QS_BINDABLE_GETTER(QString, bTrackArtUrl, trackArtUrl, bindableTrackArtUrl); [[nodiscard]] MprisPlaybackState::Enum playbackState() const; void setPlaybackState(MprisPlaybackState::Enum playbackState); @@ -281,9 +290,22 @@ public: signals: /// The track has changed. /// - /// All track info change signalss will fire immediately after if applicable, - /// but their values will be updated before the signal fires. + /// All track information properties that were sent by the player + /// will be updated immediately following this signal. @@postTrackChanged + /// will be sent after they update. + /// + /// Track information properties: @@uniqueId, @@metadata, @@trackTitle, + /// @@trackArtist, @@trackAlbum, @@trackAlbumArtist, @@trackArtUrl + /// + /// > [!WARNING] Some particularly poorly behaved players will update metadata + /// > *before* indicating the track has changed. void trackChanged(); + /// Sent after track info related properties have been updated, following @@trackChanged. + /// + /// > [!WARNING] It is not safe to assume all track information is up to date after + /// > this signal is emitted. A large number of players will update track information, + /// > particularly @@trackArtUrl, slightly after this signal. + void postTrackChanged(); QSDOC_HIDE void ready(); void canControlChanged(); @@ -306,9 +328,9 @@ signals: void volumeSupportedChanged(); void metadataChanged(); void trackTitleChanged(); + void trackArtistChanged(); void trackAlbumChanged(); void trackAlbumArtistChanged(); - void trackArtistsChanged(); void trackArtUrlChanged(); void playbackStateChanged(); void loopStateChanged(); @@ -369,30 +391,21 @@ private: DBusMprisPlayerApp* app = nullptr; DBusMprisPlayer* player = nullptr; - quint32 mUniqueId = 0; QString mTrackId; QString mTrackUrl; - QString mTrackTitle; - QString mTrackArtists; - QString mTrackAlbum; - QString mTrackAlbumArtist; - QString mTrackArtUrl; - - DECLARE_MEMBER_NS(MprisPlayer, uniqueId, mUniqueId); DECLARE_MEMBER(MprisPlayer, length, mLength, lengthChanged); DECLARE_MEMBER_SET(length, setLength); // clang-format off - DECLARE_PRIVATE_MEMBER(MprisPlayer, trackTitle, setTrackTitle, mTrackTitle, trackTitleChanged); - DECLARE_PRIVATE_MEMBER(MprisPlayer, trackArtists, setTrackArtists, mTrackArtists, trackArtistsChanged); - DECLARE_PRIVATE_MEMBER(MprisPlayer, trackAlbum, setTrackAlbum, mTrackAlbum, trackAlbumChanged); - DECLARE_PRIVATE_MEMBER(MprisPlayer, trackAlbumArtist, setTrackAlbumArtist, mTrackAlbumArtist, trackAlbumArtistChanged); - DECLARE_PRIVATE_MEMBER(MprisPlayer, trackArtUrl, setTrackArtUrl, mTrackArtUrl, trackArtUrlChanged); + Q_OBJECT_BINDABLE_PROPERTY(MprisPlayer, quint32, bUniqueId); + Q_OBJECT_BINDABLE_PROPERTY(MprisPlayer, QVariantMap, bMetadata, &MprisPlayer::metadataChanged); + Q_OBJECT_BINDABLE_PROPERTY(MprisPlayer, QString, bTrackArtist, &MprisPlayer::trackArtistChanged); + Q_OBJECT_BINDABLE_PROPERTY(MprisPlayer, QString, bTrackTitle, &MprisPlayer::trackTitleChanged); + Q_OBJECT_BINDABLE_PROPERTY(MprisPlayer, QString, bTrackAlbum, &MprisPlayer::trackAlbumChanged); + Q_OBJECT_BINDABLE_PROPERTY(MprisPlayer, QString, bTrackAlbumArtist, &MprisPlayer::trackAlbumArtistChanged); + Q_OBJECT_BINDABLE_PROPERTY(MprisPlayer, QString, bTrackArtUrl, &MprisPlayer::trackArtUrlChanged); // clang-format on - -public: - DECLARE_MEMBER_GET(uniqueId); }; } // namespace qs::service::mpris diff --git a/src/wayland/popupanchor.cpp b/src/wayland/popupanchor.cpp index 81c1cb1d..c6a6412d 100644 --- a/src/wayland/popupanchor.cpp +++ b/src/wayland/popupanchor.cpp @@ -4,9 +4,9 @@ #include #include #include +#include #include #include -#include #include "../core/popupanchor.hpp" #include "../core/types.hpp" diff --git a/src/window/popupwindow.cpp b/src/window/popupwindow.cpp index df68474f..7454d363 100644 --- a/src/window/popupwindow.cpp +++ b/src/window/popupwindow.cpp @@ -1,6 +1,5 @@ #include "popupwindow.hpp" -#include #include #include #include @@ -42,9 +41,7 @@ void ProxyPopupWindow::setParentWindow(QObject* parent) { this->mAnchor.setWindow(parent); } -QObject* ProxyPopupWindow::parentWindow() const { - return this->mAnchor.window(); -} +QObject* ProxyPopupWindow::parentWindow() const { return this->mAnchor.window(); } void ProxyPopupWindow::updateTransientParent() { auto* bw = this->mAnchor.backingWindow(); @@ -70,7 +67,8 @@ void ProxyPopupWindow::updateTransientParent() { void ProxyPopupWindow::onParentUpdated() { this->updateTransientParent(); } void ProxyPopupWindow::setScreen(QuickshellScreenInfo* /*unused*/) { - qmlWarning(this) << "Cannot set screen of popup window, as that is controlled by the parent window"; + qmlWarning(this + ) << "Cannot set screen of popup window, as that is controlled by the parent window"; } void ProxyPopupWindow::setVisible(bool visible) {