Fix LoopStatus errors #16

llewelld · 2023-01-17T22:16:38Z

Return DBus errors for incorrect Set parameters: Fixes a SIGSEGV when incorrect parameters were passed when setting LoopStatus and PlaybackStatus via DBus. Provides a separate DBus interface for org.freedesktop.DBus.Properties rather than using the automatic interface, so that errors can be returned when these invalid values are Set.
Send correct LoopStatus values. Send "None", "Track" and "Playlist" instead of "LoopNone", "LoopTrack" and "LoopPlaylist" for the LoopStatus values.
Use enums internally for LoopStatus and playbackStatus. Rather than storing the loopStatus and playbackStatus as QStrings, this uses the enums internally to ensure valid values are always maintained.
Update canControl at initialisation. Ensures that canControl emits a change signal in case it switches from false (the default) to true during initialisation. Otherwise clients won't notice that it's changed.

src/mpris.cpp

src/mprispropertiesadaptor.cpp

src/mpris.cpp

src/mprispropertiesadaptor.cpp

inzanity · 2023-01-20T14:58:53Z

src/mprispropertiesadaptor.cpp

+{
+    // The property was not found
+    m_playerPrivate->sendErrorReply(QDBusError::UnknownProperty,
+                                    QString::fromLatin1("Property %1%2%3 was not found in object %4")


I guess QStringLiteral would be the best choice here.

Although, this is the error path, so the binary size vs performance might lean towards binary size.

Yes, I guess you're right: the choice here is between an extra run-time Latin1 -> UTF-16 conversion, and doubling the string size in the binary.

I think this approach here is okay; it's used quite widely elsewhere, e.g. in Qt itself

inzanity · 2023-01-20T15:04:01Z

src/mprispropertiesadaptor.cpp

+    }
+
+    const QString &getter = getMap.value(property_name);
+    if (!getter.isEmpty()) {


I think .isNull() would be the most correct check, but this works just as well.

Not that it should happen, but this is intended to capture "" entries in the map as well.

src/mprispropertiesadaptor_p.h

Tomin1

Some comments. I need to come back to this still though.

src/mpris.cpp

src/mprisplayer_p.h

src/mprispropertiesadaptor.cpp

src/mpris.cpp

src/mprispropertiesadaptor_p.h

Tomin1

I had some style related comments to check but otherwise looks good to me and AFAICT it also works. 👍

src/mprispropertiesadaptor.cpp

qtdbusextended/dbusextendedabstractinterface.h

Tomin1 · 2023-01-25T15:56:44Z

One more thing, one of the commit messages has title:

[mpris] Send correct LoopStatus values. Fixes JB59854

That is missing # character.

Fixes a SIGSEGV when incorrect parameters were passed when setting LoopStatus and PlaybackStatus via DBus. Provides a separate DBus interface for org.freedesktop.DBus.Properties rather than using the automatic interface, so that errors can be returned when these invalid values are Set.

Send "None", "Track" and "Playlist" instead of "LoopNone", "LoopTrack" and "LoopPlaylist" for the LoopStatus values.

… JB#59562 Rather than storing the loopStatus and playbackStatus as QStrings, this uses the enums internally to ensure valid values are always maintained. Note that this will change the behaviour a little as now setting an invalid value does not result in change at all. Previously it would have set the value as is and made the library effectively use a default value. Internal functions access the values directly through internal getters and setters. Co-authored-by: Tomi Leppänen <tomi.leppanen@jolla.com>

Ensures that canControl emits a change signal in case it switches from false (the default) to true during initialisation. Otherwise clients won't notice that it's changed.

llewelld · 2023-01-25T17:15:17Z

That is missing # character.

Thanks for your careful review Tomi. I've fixed the commit message too.

llewelld marked this pull request as draft January 17, 2023 22:16

inzanity reviewed Jan 17, 2023

View reviewed changes

src/mpris.cpp Outdated Show resolved Hide resolved

inzanity reviewed Jan 17, 2023

View reviewed changes

src/mprispropertiesadaptor.cpp Outdated Show resolved Hide resolved

llewelld force-pushed the jb59853 branch from eac759b to 70da624 Compare January 18, 2023 10:27

inzanity reviewed Jan 18, 2023

View reviewed changes

src/mpris.cpp Outdated Show resolved Hide resolved

inzanity reviewed Jan 18, 2023

View reviewed changes

src/mprispropertiesadaptor.cpp Outdated Show resolved Hide resolved

inzanity reviewed Jan 18, 2023

View reviewed changes

src/mprispropertiesadaptor.cpp Outdated Show resolved Hide resolved

llewelld force-pushed the jb59853 branch 4 times, most recently from f1639b0 to a88a16e Compare January 20, 2023 07:58

llewelld marked this pull request as ready for review January 20, 2023 08:06

llewelld requested review from inzanity, Tomin1 and abranson and removed request for inzanity January 20, 2023 08:06

inzanity reviewed Jan 20, 2023

View reviewed changes

src/mprispropertiesadaptor_p.h Outdated Show resolved Hide resolved

Tomin1 reviewed Jan 23, 2023

View reviewed changes

src/mpris.cpp Outdated Show resolved Hide resolved

src/mprisplayer_p.h Outdated Show resolved Hide resolved

src/mprispropertiesadaptor.cpp Show resolved Hide resolved

src/mprispropertiesadaptor.cpp Outdated Show resolved Hide resolved

pvuorela reviewed Jan 23, 2023

View reviewed changes

src/mpris.cpp Outdated Show resolved Hide resolved

llewelld force-pushed the jb59853 branch from a88a16e to 2bbcd6b Compare January 24, 2023 17:55

llewelld requested review from pvuorela, inzanity and Tomin1 and removed request for inzanity January 24, 2023 18:02

pvuorela reviewed Jan 25, 2023

View reviewed changes

src/mprispropertiesadaptor_p.h Outdated Show resolved Hide resolved

llewelld force-pushed the jb59853 branch from 2bbcd6b to 267c7b3 Compare January 25, 2023 08:53

Tomin1 approved these changes Jan 25, 2023

View reviewed changes

llewelld force-pushed the jb59853 branch from 267c7b3 to 520b47a Compare January 25, 2023 16:57

llewelld and others added 4 commits January 25, 2023 17:00

[mpris] Send correct LoopStatus values. Fixes JB#59854

598917a

Send "None", "Track" and "Playlist" instead of "LoopNone", "LoopTrack" and "LoopPlaylist" for the LoopStatus values.

[mpris] Update canControl at initialisation. Contributes to JB#51154

4ba6c5a

Ensures that canControl emits a change signal in case it switches from false (the default) to true during initialisation. Otherwise clients won't notice that it's changed.

llewelld force-pushed the jb59853 branch from 520b47a to 4ba6c5a Compare January 25, 2023 17:01

llewelld merged commit 588ee25 into master Jan 28, 2023

Tomin1 deleted the jb59853 branch March 31, 2023 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix LoopStatus errors #16

Fix LoopStatus errors #16

llewelld commented Jan 17, 2023 •

edited

inzanity Jan 20, 2023 •

edited

llewelld Jan 24, 2023

inzanity Jan 20, 2023

llewelld Jan 24, 2023

Tomin1 left a comment

Tomin1 left a comment

Tomin1 commented Jan 25, 2023

llewelld commented Jan 25, 2023

Fix LoopStatus errors #16

Fix LoopStatus errors #16

Conversation

llewelld commented Jan 17, 2023 • edited

inzanity Jan 20, 2023 • edited

Choose a reason for hiding this comment

llewelld Jan 24, 2023

Choose a reason for hiding this comment

inzanity Jan 20, 2023

Choose a reason for hiding this comment

llewelld Jan 24, 2023

Choose a reason for hiding this comment

Tomin1 left a comment

Choose a reason for hiding this comment

Tomin1 left a comment

Choose a reason for hiding this comment

Tomin1 commented Jan 25, 2023

llewelld commented Jan 25, 2023

llewelld commented Jan 17, 2023 •

edited

inzanity Jan 20, 2023 •

edited