search

sphh / mopidy-autoplay

Apache License 2.0
6 stars 2 forks source link

Use LibraryController lookup, and seek only if not stopped #7

Closed fmauNeko closed 3 years ago

fmauNeko commented 3 years ago

Hello !

I made 2 small changes to the restore_state function

The first is to use library lookups instead of relying on playlist schemes to detect playlists from normal tracks, because it breaks with spotify: URIs (the scheme is registered in the playlist schemes, which means track URIs are processed as playlist URIs, preventing them to be added to the tracklist).

The second is to return before seeking, because if seek is called when the playback state is stopped, it will switch to playing, thus defeating the purpose of a stopped state.

I also took the liberty to create an Arch AUR package at https://aur.archlinux.org/packages/mopidy-autoplay/ .

Have a good day !

sphh commented 3 years ago

Thanks @fmauNeko for both changes!

May I ask you for two changes?

  1. Could you please format the list comprehension for track_uris in a similar way to that one of the (deleted) uris.extend(), so that it conforms to pep8 and the mopdiy formatting guidelines?
  2. Could you please remove the return and add an additional and playback == 'stopped' to the if time_position is not None: clause? I know, that the time_position = might become redundant, but in my opinion that change would improve the readability of the code, since this is be the only position, where restore_state() would returning prematurely. Alternatively the part time_position = …; if time_position … could be put in a if playback != 'stopped': time_position …; if time_position … clause.

Would that be possible?

sphh commented 3 years ago

If you want, I can do these changes myself. Just tell me what you prefer!

fmauNeko commented 3 years ago

Changes submitted, don't hesitate to tell me if there's still anything wrong !

sphh commented 3 years ago

Thanks! Highly appreciated!

Could you please change your nested list comprehension to something like:

    ... = [
        x.y
        for sublist in ...
        for x in sublist]

which I believe is the common way to write long list comprehensions. Than it's ready to be merged!

fmauNeko commented 3 years ago

Done !

sphh commented 3 years ago

I just merged you PR. Thanks for it!

I still have a questions (I haven't looked into it more closely): Isn't that a bug in mopidy's spotify implementation? (and your PR ‘just’ a workaround?)

sphh commented 3 years ago

@fmauNeko: In the new release, I would like to shed some light onto the reason, why this PR was needed. For this I would love to have the background information from above. May I ask you to have a look at my question? Thanks.

sphh commented 3 years ago

@fmauNeko: Since I have to received an explanation and background information, I have to remove this PR, because I do not really know, what it is doing. It's actually a pity, but I cannot support code, which I do not know or understand.

As soon as I get an explanation, I can integrate it again …

sphh commented 3 years ago

This repository has been moved to https://codeberg.org/sph/mopidy-autoplay/ and this one set to read-only.

Please continue at https://codeberg.org/sph/mopidy-autoplay/pulls/7