-
Notifications
You must be signed in to change notification settings - Fork 83
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
Conversation
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 don't use the dbus features but the code looks solid.
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.
e87c5b6
to
8eebf5e
Compare
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.
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. |
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:
access to the dbus signal subscriber & filtering protocol.
This way we only need a total of two watchers at all times.
regex criteria.
still playing.
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 Pythonversion 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:
TypeError
s 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.