Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hasShuffle and hasLoopStatus properties #20

Merged
merged 1 commit into from Mar 13, 2023
Merged

Add hasShuffle and hasLoopStatus properties #20

merged 1 commit into from Mar 13, 2023

Conversation

llewelld
Copy link
Member

Adds canShuffle and canLoop properties to indicate whether or not the Shuffle and LoopStatus properties are supported.

These work by exposing/hiding the DBus property, so work within the MPRIS specification.

@llewelld
Copy link
Member Author

I was in two minds about the naming. Maybe canControlShuffle and canControlLoopStatus are better than canShuffle and canLoop?

@Tomin1
Copy link
Member

Tomin1 commented Jan 30, 2023

I was in two minds about the naming. Maybe canControlShuffle and canControlLoopStatus are better than canShuffle and canLoop?

I think they could be better names for this.

@abranson
Copy link
Member

I'm not sure. canControl would imply that those properties are set to something that's read only if it's false, while the implementation presents them more as null. I'd say 'hasShuffle' is more accurate than 'canControlShuffle', but 'canShuffle' is closer and more consistent with the other methods.

Copy link
Member

@Tomin1 Tomin1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Andrew's comments about the naming. That hasShuffle() should be considered as an alternative. I suppose mpris specification won't get any canShuffle() method, but if it did, these might collide a bit.

declarative/declarativemprisplayer.cpp Outdated Show resolved Hide resolved
declarative/declarativemprisplayer.cpp Outdated Show resolved Hide resolved
declarative/declarativemprisplayer.cpp Outdated Show resolved Hide resolved
src/mprispropertiesadaptor.cpp Show resolved Hide resolved
@Tomin1
Copy link
Member

Tomin1 commented Mar 1, 2023

I changed these to my liking but of course someone else should review the changes. I can also squash them if the new changes are fine.

@Tomin1
Copy link
Member

Tomin1 commented Mar 1, 2023

I'm not sure. canControl would imply that those properties are set to something that's read only if it's false, while the implementation presents them more as null. I'd say 'hasShuffle' is more accurate than 'canControlShuffle', but 'canShuffle' is closer and more consistent with the other methods.

I noticed that there is also HasTrackList property in the specification, so these 'has' prefixed names are not that alien to mpris after all. In some ways that is a bit analogous to 'hasShuffle' and 'hasLoopStatus' as it's exactly that whether something is exposed through D-Bus or not ('HasTrackList' determines whether there is a track list API or not).

I wasn't yet sure which we should prefer so I didn't change the naming. I need to consider it myself still but I might change it still.

…B#59545

Adds hasShuffle and hasLoopStatus properties to indicate whether or not
the Shuffle and LoopStatus properties are supported. They can be set
until the properties are read the first time with GetAll. If library
user wants to change them still after that point, they should unregister
the service first.

These work by exposing/hiding the DBus property, so work within the
MPRIS specification. However this only affects GetAll, Get and Set
methods. Introspection will still show that these properties exist.

Co-authored-by: Tomi Leppänen <tomi.leppanen@jolla.com>
Signed-off-by: Tomi Leppänen <tomi.leppanen@jolla.com>
@Tomin1
Copy link
Member

Tomin1 commented Mar 3, 2023

I updated the naming and adjusted the docstrings a little more still. I didn't touch introspection but I think that while it would be good to do it properly, we could also leave to another PR, if we want to get this merged sooner.

@Tomin1 Tomin1 requested review from pvuorela and abranson March 3, 2023 14:14
@Tomin1 Tomin1 changed the title [mpris] Add canShuffle and canLoop properties. Contributes to JB#59545 Add hasShuffle and hasLoopStatus properties Mar 3, 2023
@Tomin1 Tomin1 merged commit 88b0953 into master Mar 13, 2023
@Tomin1 Tomin1 deleted the jb59545 branch March 13, 2023 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants