-
Notifications
You must be signed in to change notification settings - Fork 12
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 missing property changes #35
base: main
Are you sure you want to change the base?
Conversation
It has been a long time since I looked at this project, but I think these changes make sense. Can you update the three failing tests to match your changes:
Please also add a new test for |
This version fixes the existing tests and adds new unit and integration tests. I hope you don’t mind the two commits doing minor cleanups and adding Python 3.10 to tox. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks very good. Just a couple of control questions!
except Exception as e: | ||
logger.warning("MPRIS frontend setup failed (%s)", e) | ||
except Exception: | ||
logger.exception("MPRIS frontend setup failed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to note that this will change the output from something short like e.g. "MPRIS frontend setup failed (unable to bind to port)" to a full stack trace.
I don't remember the typical errors here, so I guess this is OK.
else: | ||
self.core.playback.play().get() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I don't remember how this works. Will .play()
correctly resume from the middle of the track where you paused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems so, https://docs.mopidy.com/stable/_modules/mopidy/core/playback/#PlaybackController.play
elif tl_track is None and self.get_state() == PlaybackState.PAUSED:
self.resume()
return
@@ -3,7 +3,7 @@ requires = ["setuptools >= 30.3.0", "wheel"] | |||
|
|||
|
|||
[tool.black] | |||
target-version = ["py37", "py38"] | |||
target-version = ["py37", "py38", "py39", "py310"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cherry-picked the Python 3.10 commit to main
, so this will disappear if you rebase.
def player_call(can_play, can_go_next, can_go_previous): | ||
vals = DictFields( | ||
{ | ||
"CanPlay": can_play, | ||
"CanGoNext": can_go_next, | ||
"CanGoPrevious": can_go_previous, | ||
} | ||
) | ||
return call("org.mpris.MediaPlayer2.Player", vals, []) | ||
|
||
|
||
def assert_called(can_play, can_go_next, can_go_previous): | ||
# Beware the multithreading! Wait for the signal to get emitted. | ||
sleep(0.1) | ||
mopidy_mpris.interface.Interface.PropertiesChanged.assert_has_calls( | ||
[player_call(can_play, can_go_next, can_go_previous)] | ||
) | ||
mopidy_mpris.interface.Interface.PropertiesChanged.reset_mock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would read better with player_call
inlined and by requiring the arguments to assert_called
to be kwargs:
def player_call(can_play, can_go_next, can_go_previous): | |
vals = DictFields( | |
{ | |
"CanPlay": can_play, | |
"CanGoNext": can_go_next, | |
"CanGoPrevious": can_go_previous, | |
} | |
) | |
return call("org.mpris.MediaPlayer2.Player", vals, []) | |
def assert_called(can_play, can_go_next, can_go_previous): | |
# Beware the multithreading! Wait for the signal to get emitted. | |
sleep(0.1) | |
mopidy_mpris.interface.Interface.PropertiesChanged.assert_has_calls( | |
[player_call(can_play, can_go_next, can_go_previous)] | |
) | |
mopidy_mpris.interface.Interface.PropertiesChanged.reset_mock() | |
def assert_called(*, can_play, can_go_next, can_go_previous): | |
# Beware the multithreading! Wait for the signal to get emitted. | |
sleep(0.1) | |
mopidy_mpris.interface.Interface.PropertiesChanged.assert_has_calls( | |
[ | |
call( | |
"org.mpris.MediaPlayer2.Player", | |
DictFields({ | |
"CanPlay": can_play, | |
"CanGoNext": can_go_next, | |
"CanGoPrevious": can_go_previous, | |
}), | |
[], | |
), | |
] | |
) | |
mopidy_mpris.interface.Interface.PropertiesChanged.reset_mock() |
Even though the README says
If you're interested in this, just replace my name with your own when completing the last step here. |
Currently not all affected MPRIS properties are updated when mopidy’s state changes. This PR adds handling for