rinukkusu / spotify-dart

A dart library for interfacing with the Spotify API.
BSD 3-Clause "New" or "Revised" License
207 stars 93 forks source link

Add player methods for controlling playback (#134) #145

Closed rinukkusu closed 1 year ago

rinukkusu commented 1 year ago

This PR adds these player related endpoints:

@hayribakici in the shuffle endpoint you're making another call to playbackState and returning that response and I was wondering, if you think we should do this for these methods, too? 🤔

hayribakici commented 1 year ago

That is a good question. I returned the playbackState due to the fact, that it might help the library users with their (app) state handling. On the other hand, with making another request, the methods might take longer to return (with play/pause/next etc.). It would be interesting to know what others think of this. @roisnir @chances 🤔

hayribakici commented 1 year ago

@rinukkusu I gave this a little more thought and I think the methods should return the playbackState. The reason is, that the API for playbackState returns an optional action object (which we haven't serialized yet)

{
"actions": {
    "interrupting_playback": true,
    "pausing": true,
    "resuming": true,
    "seeking": true,
    "skipping_next": true,
    "skipping_prev": true,
    "toggling_repeat_context": true,
    "toggling_shuffle": true,
    "toggling_repeat_track": true,
    "transferring_playback": true
  }
}

The documentation of this is as follows:

Allows to update the user interface based on which playback actions are available within the current context.

Given this information, I think the the library user can adapt their UI accordingly (One scenario that comes to my mind is e.g. the (app) user wants to pause the the currently playing track from another device and showing a e.g. spinning wheel for the transfer)

So, imho in the long run, I think the playbackState can be returned after each player related request. What do you think?

rinukkusu commented 1 year ago

That's a very good point, didn't know about this actions object. As an app developer and consumer of this library I think this would help me to quickly update the user interface without having to think about making another api call.

Nevertheless I think I'd like to make it possible to opt out of this subsequent api call with a named parameter dontRetrievePlaybackState or maybe retrievePlaybackState and default is true and return a nullable PlaybackState.

Something like that:

Future<PlaybackState?> pause({String? deviceId, retrievePlaybackState = true}) async {
   // ...
}

Does that seem logical to you? 😆

hayribakici commented 1 year ago

That seems like a good compromise, yes 😄

cudajo commented 1 year ago

Calling spotify.player.startOrResume(options: StartOrResumeOptions(uris: [uri], positionMs: 41000,) to make Spotify start a specified track uri at 41000 ms, gives me an error stating that: At least one of "uri" or "position" should be specified

The only way to get rid of the error message and actually start playing the specified track seems to be to at least also include the offset parameter, that is: spotify.player.startOrResume(offset: PositionOffset(0), options: StartOrResumeOptions(uris: [uri], positionMs: 41000,) According to the docs though, the offset parameter is optional and "...only available when context_uri corresponds to an album or playlist object". Looking at the docs for the context_uri in turn, it says: "Optional. Spotify URI of the context to play. Valid contexts are albums, artists & playlists."

In my case I'm trying to start a specific track - which is not one of the mentioned "context_uri contexts". As I understand it, I should not add the track uri to the context_uri, and also not need to add the optional offset.

Something I've misunderstood, a simple error in the documentation, or something else?

Thanks!

hayribakici commented 1 year ago

@cudajo Yes, you are right. According to the documentation, the track uri does not belong into the context_uri, since only album,artist and playlist contexts are allowed. The only confusing part in the documentation is, that is says

Example: "offset": {"uri": "spotify:track:1301WleyT98MSxVHPZCA6M"}

I think this uri in the offset is bound to the album in the context_uri. Calling something like startOrResume(offset: UriOffset(uri: <some_track_uri_of_an_album_Foo>), options: StartOrResumeOptions(contextUri: <uri_of_album_Foo>, positionMs: 42000)

should play a track some_track_uri_of_an_album_Foo of uri_of_album_Foo starting at the positionMs: 42000 (Disclaimer: I haven't tried this!).

In your case, just playing a track would then be startOrResume(options: StartOfResumeOptions(uri: ["some_tack_uri"], positionMs: 123).

cudajo commented 11 months ago

Continuing on this topic as the returning of PlaybackState also was discussed earlier...

I have just recently run in to the issue that after calling startOrResume(...) to start playing a track, the returned PlaybackState sometimes comes with the isPlaying flag set to false instead of the expected value of true - causing some issues for me to adapt the UI to the current state...

This currently only seems to happen when the target device is a mobile phone. Starting a track with my laptop (Mac) as target device it seems like the isPlaying flag always comes back with the correct status (ie. "true").

I understand that this might be more related to how the Spotify API handles these things in the background and on specific devices, rather than this package, but thought it would be something to raise awareness about at least.

Making an additional call to PlayerEndpoint.playbackState() immediately after startOrResume(...) has returned seems to return the correct state even when targeting a mobile phone. It's not 100% though. I assume that when it's working, "enough time" has passed for the track to actually have been started - otherwise not.

rinukkusu commented 9 months ago

Yeah, there seems to be some delay on mobile devices then. I'd suggest to just check the state again, as you said.