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

Small suggestions after Add multi-instance player support in commit 80862c1 #14

Closed
wants to merge 1 commit into from

Conversation

tellezhector
Copy link
Contributor

@tellezhector tellezhector commented Sep 2, 2024

Small suggestions after commit 80862c1

  • If the player is disconnected, let the user know (i.e. print a message that says '(no player)').
  • Propagate KeyInterrupt exceptions properly (important for launcher's to properly handle interruptions and redirect interruptions; i.e. if the launcher is interrupted, the launcher can in turn interrupt the sub-process).
  • MPRISBlocket._instances can be a set instead of a dictionary. Dictionaries where values are always True can always be translated to sets (and sets also have O(1) time look-ups).
  • bus_name_has_owner(bus_name=None) was always equivalent to bus_name_has_owner(bus_name=self._bus_name), removed the redudancy by removing the default value and always passing an explicit value
  • moved null check guard within stop_stdin_read_loop, reducing cyclomatic complexity of MPRISBlocket.run

* If the player is disconnected, let the user know (i.e. print a
  message that says `'(no player)'`).
* Propagate KeyInterrupt exceptions properly (important for launcher's
  to properly handle interruptions and redirect interruptions; i.e. if the
  launcher is interrupted, the launcher can in turn interrupt the sub-process).
* `MPRISBlocket._instances` can be a `set` instead of a dictionary.
  Dictionaries where values are always True can always be translated to
  sets (and sets also have O(1) time look-ups).
* `bus_name_has_owner(bus_name=None)` was always equivalent to
  `bus_name_has_owner(bus_name=self._bus_name)`, removed the redudancy
  by removing the default value and always passing an explicit value
* moved null check guard within stop_stdin_read_loop, reducing
  cyclomatic complexity of MPRISBlocket.run
@tellezhector tellezhector changed the title Small suggestions after Add multi-instance player support 80862c1 Small suggestions after Add multi-instance player support in commit 80862c1 Sep 3, 2024
@un-def un-def mentioned this pull request Sep 8, 2024
un-def added a commit that referenced this pull request Sep 8, 2024
See: #14

* Propagate KeyboardInterrupt exceptions (important for launcher's
  to properly handle interruptions and redirect interruptions; i.e. if the
  launcher is interrupted, the launcher can in turn interrupt the sub-process).
* `bus_name_has_owner(bus_name=None)` was always equivalent to
  `bus_name_has_owner(bus_name=self._bus_name)`, removed the redundancy
  by removing the default value and always passing an explicit value
* moved null check guard within stop_stdin_read_loop, reducing
  cyclomatic complexity of MPRISBlocket.run

Co-authored-by: Hector Tellez <tellezhector@users.noreply.github.com>
@un-def
Copy link
Owner

un-def commented Sep 8, 2024

The one thing that seemed odd is that when the current player is disconnected, the blocket goes blank, a little message could be nice to let the user know what's going on.

This is exactly the behavior I expect. I have multiple instances of the blocklet for multiple players, and it's a feature, not quirk, that unused blocklets “collapse”.

If the player is disconnected, let the user know (i.e. print a message that says '(no player)').

I've made this configurable, add -n (no-player) to the command-line arguments or "placeholder": "(no player)" to the JSON config to get the same result. The default is an empty string, that is, it keeps the previous behavior.

Propagate KeyInterrupt exceptions properly

I've removed except KeyboardInterrupt completely as redundant.

MPRISBlocket._instances can be a set instead of a dictionary

A dict is used as an ordered set (insertion-order). When a player disappears, we pick the latest one as a new player. With regular set, there is no way to prioritize one instance over the other (except lexicographically, which makes little sense).

bus_name_has_owner(bus_name=None) was always equivalent to bus_name_has_owner(bus_name=self._bus_name), removed the redudancy by removing the default value and always passing an explicit value

moved null check guard within stop_stdin_read_loop, reducing cyclomatic complexity of MPRISBlocket.run

I've applied these changes. Merged to master as a new PR: #15

Thanks for your contributions!

@un-def
Copy link
Owner

un-def commented Sep 8, 2024

Closed in favor of #15

@un-def un-def closed this Sep 8, 2024
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.

2 participants