From 63d4e096826dc2b9846b7c427c2ae2b8b8ab6051 Mon Sep 17 00:00:00 2001 From: Hector Tellez Date: Mon, 2 Sep 2024 01:38:52 -0700 Subject: [PATCH 1/3] Small suggestions: * 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 --- i3blocks_mpris.py | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/i3blocks_mpris.py b/i3blocks_mpris.py index 869c638..0546fa6 100644 --- a/i3blocks_mpris.py +++ b/i3blocks_mpris.py @@ -132,7 +132,7 @@ class MPRISBlocklet: _loop = None _stdin_stream = None - _bus = None + _bus: dbus.SessionBus | None = None _properties_changed_signal_match = None _specific_name_owner_changed_signal_match = None _any_name_owner_changed_signal_match = None @@ -171,9 +171,8 @@ def __init__(self, bus_name, config=None): self._last_info = None self._last_status = None self._last_metadata = None - # a dict used as an ordered set, keys — well-known names with unique - # instance suffixes, values — True - self._instances = {} + # a dict set of well-known names with unique instance suffixes + self._instances = set() @classmethod def create_loop(cls): @@ -184,9 +183,7 @@ def create_loop(cls): DBusGMainLoop(set_as_default=True) return loop - def bus_name_has_owner(self, bus_name=None): - if not bus_name: - bus_name = self._bus_name + def bus_name_has_owner(self, bus_name: str): return self._bus.name_has_owner(bus_name) def init_bus(self): @@ -215,7 +212,7 @@ def run(self, *, loop=None, read_stdin=True, nowait=False): # initially, we don't know which match mode to use match_mode = MatchMode.UNKNOWN player_found = False - if self.bus_name_has_owner(): + if self.bus_name_has_owner(self._bus_name): # either the player don't allow multiple instance, e.g., # `org.mpris.MediaPlayer2.spotify`, or the user specified the # exact instance, e.g., `org.mpris.MediaPlayer2.chromium.instance2` @@ -242,11 +239,10 @@ def run(self, *, loop=None, read_stdin=True, nowait=False): self.start_stdin_read_loop() try: self._loop.run() - except KeyboardInterrupt: - pass + except KeyboardInterrupt as e: + raise e finally: - if read_stdin: - self.stop_stdin_read_loop() + self.stop_stdin_read_loop() def _find_instances(self) -> None: for name in self._bus.list_names(): @@ -259,22 +255,22 @@ def _maybe_add_instance(self, name: str) -> bool: maybe_prefix, _, _ = name.rpartition('.') if maybe_prefix != name_prefix: return False - self._instances[name] = True + self._instances.add(name) return True def _maybe_remove_instance(self, name: str) -> None: name_prefix = self._bus_name_prefix if not name.startswith(name_prefix): return - maybe_prefix, _, _ = name.rpartition('.') - if maybe_prefix == name_prefix: - self._instances.pop(name, None) + maybe_prefix, _, _ = name.rpartition('.' ) + if maybe_prefix == name_prefix and name in self._instances: + self._instances.remove(name) def _pick_instance(self) -> str | None: - for bus_name in reversed(tuple(self._instances)): + for bus_name in sorted(self._instances, reverse=True): if self.bus_name_has_owner(bus_name): return bus_name - del self._instances[bus_name] + self._instances.remove(bus_name) return None def start_stdin_read_loop(self): @@ -284,11 +280,12 @@ def start_stdin_read_loop(self): self._read_stdin_once() def stop_stdin_read_loop(self): + if not self._stdin_stream: + return self._stdin_stream.close_async( io_priority=GLib.PRIORITY_DEFAULT, callback=lambda *args: self._loop.quit(), ) - self._loop.run() self._stdin_stream = None def _read_stdin_once(self): @@ -351,7 +348,7 @@ def _connect_to_specific_name_owner_changed_signal(self): ) self._specific_name_owner_changed_signal_match = signal_match - def _on_specific_name_owner_changed(self, name, old_owner, new_owner): + def _on_specific_name_owner_changed(self, unused_name, old_owner, new_owner): if not old_owner and new_owner: if not self._player_connected: self._connect_to_player() From 95eb79e7cb6942a4af83edfbf6fba91164cbf2d4 Mon Sep 17 00:00:00 2001 From: Dmitry Meyer Date: Sun, 8 Sep 2024 08:06:00 +0000 Subject: [PATCH 2/3] Revert some changes --- i3blocks_mpris.py | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/i3blocks_mpris.py b/i3blocks_mpris.py index 0546fa6..f6ec81c 100644 --- a/i3blocks_mpris.py +++ b/i3blocks_mpris.py @@ -171,8 +171,9 @@ def __init__(self, bus_name, config=None): self._last_info = None self._last_status = None self._last_metadata = None - # a dict set of well-known names with unique instance suffixes - self._instances = set() + # a dict used as an ordered set, keys — well-known names with unique + # instance suffixes, values — True + self._instances = {} @classmethod def create_loop(cls): @@ -255,22 +256,22 @@ def _maybe_add_instance(self, name: str) -> bool: maybe_prefix, _, _ = name.rpartition('.') if maybe_prefix != name_prefix: return False - self._instances.add(name) + self._instances[name] = True return True def _maybe_remove_instance(self, name: str) -> None: name_prefix = self._bus_name_prefix if not name.startswith(name_prefix): return - maybe_prefix, _, _ = name.rpartition('.' ) - if maybe_prefix == name_prefix and name in self._instances: - self._instances.remove(name) + maybe_prefix, _, _ = name.rpartition('.') + if maybe_prefix == name_prefix: + self._instances.pop(name, None) def _pick_instance(self) -> str | None: - for bus_name in sorted(self._instances, reverse=True): + for bus_name in reversed(tuple(self._instances)): if self.bus_name_has_owner(bus_name): return bus_name - self._instances.remove(bus_name) + del self._instances[bus_name] return None def start_stdin_read_loop(self): @@ -286,6 +287,7 @@ def stop_stdin_read_loop(self): io_priority=GLib.PRIORITY_DEFAULT, callback=lambda *args: self._loop.quit(), ) + self._loop.run() self._stdin_stream = None def _read_stdin_once(self): @@ -348,7 +350,7 @@ def _connect_to_specific_name_owner_changed_signal(self): ) self._specific_name_owner_changed_signal_match = signal_match - def _on_specific_name_owner_changed(self, unused_name, old_owner, new_owner): + def _on_specific_name_owner_changed(self, _name, old_owner, new_owner): if not old_owner and new_owner: if not self._player_connected: self._connect_to_player() From 28ed4db0f58845ff051650969a6d4c3c68ed8364 Mon Sep 17 00:00:00 2001 From: Dmitry Meyer Date: Sun, 8 Sep 2024 08:20:20 +0000 Subject: [PATCH 3/3] Remove redundant try except --- i3blocks_mpris.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/i3blocks_mpris.py b/i3blocks_mpris.py index f6ec81c..0d8699c 100644 --- a/i3blocks_mpris.py +++ b/i3blocks_mpris.py @@ -240,8 +240,6 @@ def run(self, *, loop=None, read_stdin=True, nowait=False): self.start_stdin_read_loop() try: self._loop.run() - except KeyboardInterrupt as e: - raise e finally: self.stop_stdin_read_loop()