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 missing property changes #35

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

V02460
Copy link
Contributor

@V02460 V02460 commented Jan 19, 2022

Currently not all affected MPRIS properties are updated when mopidy’s state changes. This PR adds handling for

  • the tracklist transitioning between empty/non-empty
  • next/prev/play state on song change

@jodal
Copy link
Member

jodal commented Jan 20, 2022

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:

FAILED tests/test_events.py::test_track_playback_started_changes_playback_status_and_metadata
FAILED tests/test_events.py::test_track_playback_ended_changes_playback_status_and_metadata
FAILED tests/test_events.py::test_playback_state_changed_changes_playback_status_and_metadata

Please also add a new test for tracklist_changed.

@V02460
Copy link
Contributor Author

V02460 commented Feb 7, 2022

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.

Copy link
Member

@jodal jodal left a 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")
Copy link
Member

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.

Comment on lines +94 to 95
else:
self.core.playback.play().get()
Copy link
Member

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?

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"]
Copy link
Member

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.

Comment on lines +12 to +29
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()
Copy link
Member

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:

Suggested change
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()

@jodal
Copy link
Member

jodal commented Feb 9, 2022

Even though the README says Current maintainer: Stein Magnus Jodal, this extension is just as much up for adoption as the other extensions where that is explicitly stated. Quoting Mopidy-MPD:

If you want to be the maintainer of Mopidy-MPD, please:

  • Make 2-3 good pull requests improving any part of the project.
  • Read and get familiar with all of the project's open issues.
  • Send a pull request removing this section and adding yourself as the "Current maintainer" in the "Credits" section below. In the pull request description, please refer to the previous pull requests and state that you've familiarized yourself with the open issues.

As a maintainer, you'll be given push access to the repo and the authority to make releases to PyPI when you see fit.

If you're interested in this, just replace my name with your own when completing the last step here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants