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

mpris: Rewrite entirely, replacing pydbus with jeepney #683

Merged
merged 15 commits into from
May 1, 2024

Conversation

FichteFoll
Copy link
Collaborator

@FichteFoll FichteFoll commented Apr 16, 2023

pydbus had issues with properly registering, unregistering and filtering
dbus signals, resulting in one listener being created for each player
recognized and never cleaning them up. It is also unmaintained and the
Github repository hasn't seen activity (including responses to issues
or pull requests for over 4 years).

Additionally, there were several situations where opening another player
in the background with the same or a different file (that may or may
not be a recognizable episode) would trip up the tracker entirely,
resulting in missed updates that would cascade to any following episode
of the same show.

These situations are now resolved by:

  1. Switching to jeepney, a more low-level dbus library, providing full
    access to the dbus signal subscriber & filtering protocol.
    This way we only need a total of two watchers at all times.
  2. Properly tracking the full state of every player that fulfills the
    regex criteria.
  3. Ignoring any updates while the current player with a valid episode is
    still playing.
  4. Ignoring any updates for players whose filename cannot be recognized.

Because the parent Tracker class was not built with the idea of
potentially handling multiple players at the same time, this can still
result in confusions when the primary player is paused and another
valid episode is opened in the background, which will then replace the
tracking status of the previously paused player. However, this can be
recovered if the newly opened player is closed in time so that the
intended primary player has enough time to reach the episode update
threshold afterwards (and will probably occur rarely enough).

The code utilizes asyncio and dataclasses, bumping the required Python
version to 3.7, unless a dataclass backport is added. However, Python
3.7 is the oldest minor version that hasn't reached EOL yet, so this
should be of no concern.
(Edit: also uses asyncio.run, which was added in 3.7)

(Edit2: 3.7 was already mentioned as a requirement in the readme, so disregard)

Additionally, dependencies are re-locked due to the switch to jeepney.

Closes #440
Closes #420
Closes #636
Closes #471


I will be battle-testing this over the next few days but my initial tests seem to be rather promising.

Known issues/questions that should be addressed before merging:

  • When starting to play a recognized file and then loading a different file that is not recognized before the first file reached the update threshold, it will still be updated when the timer expires.
  • Assertions on the dbus signatures currently lead to TypeErrors that are not caught in order to make the process fail "violently". This kind of failure is hard to notice with the GUIs though and the tracker will be defunct after this happens.
  • When pausing and unpausing an episode that has already been updated, a "has already been updated" message is logged. It's only a minor invoncenience, but I'll see if I can find an easy fix for it.

Copy link
Contributor

@txtsd txtsd left a comment

Choose a reason for hiding this comment

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

I don't use the dbus features but the code looks solid.

@FichteFoll
Copy link
Collaborator Author

FichteFoll commented May 2, 2023

Thanks for taking a look. There are some issues/questions that should be addressed before merging that I've added to OP.

*pydbus* had issues with properly registering, unregistering and filtering
dbus signals, resulting in one listener being created for each player
recognized and never cleaning them up. It is also unmaintained and the
Github repository hasn't seen activity (including responses to issues
or pull requests for over 4 years).

Additionally, there were several situations where opening another player
in the background with the same or a different file (that may or may
not be a recognizable episode) would trip up the tracker entirely,
resulting in missed updates that would cascade to any following episode
of the same show.

These situations are now resolved by:

1. Switching to *jeepney*, a more low-level dbus library, providing full
   access to the dbus signal subscriber & filtering protocol.
   This way we only need a total of two watchers at all times.
2. Properly tracking the full state of every player that fulfills the
   regex criteria.
3. Ignoring any updates while the current player with a valid episode is
   still playing.
4. Ignoring any updates for players whose filename cannot be recognized.

Because the parent Tracker class was not built with the idea of
potentially handling multiple players at the same time, this can still
result in confusions when the primary player is paused and another
valid episode is opened in the background, which will then replace the
tracking status of the previously paused player. However, this can be
recovered if the newly opened player is closed in time so that the
intended primary player has enough time to reach the episode update
threshold afterwards (and will probably occur rarely enough).

The code utilizes asyncio and dataclasses, bumping the required Python
version to 3.7, unless a dataclass backport is added. However, Python
3.7 is the oldest minor version that hasn't reached EOL yet, so this
should be of no concern.

Additionally, dependencies are re-locked due to the switch to jeepney.

Closes #440
Closes #420
Closes #636
The event listener is quite reliable and we really don't need to log
each event from an inactive player when we can instead log their names
when we hear about them for the first time.

Spotify is known to send at least 5 metadata updates whenever a track
changes, for example.
Only reset `last_filename` if we're going through all currently playing
players to find a video candidate. Otherwise we'd get very frequent
(debug) messages for unrecognized videos whenever a video is paused and
re-played despite us already knowing that the video isn't recognizable.
That is, unless the list has been updated.
@FichteFoll FichteFoll force-pushed the bugfix/mpris-rewrite branch from e87c5b6 to 8eebf5e Compare April 28, 2024 10:08
FichteFoll added 7 commits May 1, 2024 14:40
Also don't re-raise the exception since we're already reporting on it
and just let the thread die afterwards.
While the timer *should* not be `None`, it certainly is possible
(usually under race conditions) and we can simply treat it as zero in
that case.

It doesn't seem like any other UI tracks the tracker state like this
besides curses and that can handle it already.
@FichteFoll
Copy link
Collaborator Author

So, I have been using this branch as my main and only branch for trackma basically since I created it, meaning for over a year now, and it just works really well. It addresses almost all of the concerns I had with the old pydbus code and beside very few remaining usability improvements that could be made, it's just a significant update over whatever we have currently. Other people, who are also using this, have urged me several times to merge it as well.

Thus, I am going to do this. Any further changes (including the three items I listed in OP) can be tackled at a later point and there is no reason to delay this PR any further. Besides, who knows when I (or someone else) will get around to looking into them.

I've rebased this banch onto master and also applied a few more low-hanging fixes that have either been reported to me (crash in the QT UI) or that I have noticed myself over time but never bothered to actually fix.

@FichteFoll FichteFoll merged commit c7f8e26 into master May 1, 2024
@FichteFoll FichteFoll deleted the bugfix/mpris-rewrite branch May 1, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants