Merge pull request slgobinath#807 from deltragon/fix-disable-trayicon-plugin

trayicon: fix removing trayicon when plugin is disabled

Author: deltragon

safeeyes/plugin_manager.py

@@ -68,9 +68,17 @@
 import logging
 import os
 import sys
+import typing
 
 from safeeyes import utility
-from safeeyes.model import Break, PluginDependency, RequiredPluginException, TrayAction
+from safeeyes.context import Context
+from safeeyes.model import (
+    Config,
+    Break,
+    PluginDependency,
+    RequiredPluginException,
+    TrayAction,
+)
 
 sys.path.append(os.path.abspath(utility.SYSTEM_PLUGINS_DIR))
 sys.path.append(os.path.abspath(utility.USER_PLUGINS_DIR))
@@ -81,13 +89,16 @@
 class PluginManager:
     """Imports the Safe Eyes plugins and calls the methods defined in those plugins."""
 
-    def __init__(self):
+    __plugins: dict[str, "LoadedPlugin"]
+    last_break: typing.Optional[Break]
+
+    def __init__(self) -> None:
         logging.info("Load all the plugins")
         self.__plugins = {}
         self.last_break = None
         self.horizontal_line = "─" * HORIZONTAL_LINE_LENGTH
 
-    def init(self, context, config):
+    def init(self, context: Context, config: Config) -> None:
         """Initialize all the plugins with init(context, safe_eyes_config,
         plugin_config) function.
         """
@@ -111,12 +122,43 @@ def init(self, context, config):
         # Initialize the plugins
         for plugin in self.__plugins.values():
             plugin.init_plugin(context, config)
-        return True
 
-    def needs_retry(self):
+    def reload(self, context: Context, config: Config) -> None:
+        """Reinitialize all the plugins with updated config."""
+        plugin_ids: set[str] = set()
+        # Load the plugins
+        for plugin in config.get("plugins"):
+            plugin_id = plugin["id"]
+            plugin_ids.add(plugin_id)
+            if plugin_id in self.__plugins:
+                self.__plugins[plugin_id].reload_config(plugin)
+            else:
+                try:
+                    loaded_plugin = LoadedPlugin(plugin)
+                    self.__plugins[loaded_plugin.id] = loaded_plugin
+                except BaseException as e:
+                    traceback_wanted = (
+                        logging.getLogger().getEffectiveLevel() == logging.DEBUG
+                    )
+                    if traceback_wanted:
+                        import traceback
+
+                        traceback.print_exc()
+                    logging.error("Error in loading the plugin %s: %s", plugin["id"], e)
+                    continue
+
+        removed_plugins = set(self.__plugins.keys()).difference(plugin_ids)
+        for plugin_id in removed_plugins:
+            self.__plugins[plugin_id].disable()
+
+        # Initialize the plugins
+        for plugin in self.__plugins.values():
+            plugin.init_plugin(context, config)
+
+    def needs_retry(self) -> bool:
         return self.get_retryable_error() is not None
 
-    def get_retryable_error(self):
+    def get_retryable_error(self) -> typing.Optional[RequiredPluginException]:
         for plugin in self.__plugins.values():
             if plugin.required_plugin and plugin.errored and plugin.enabled:
                 if (
@@ -129,7 +171,7 @@ def get_retryable_error(self):
 
         return None
 
-    def retry_errored_plugins(self):
+    def retry_errored_plugins(self) -> None:
         for plugin in self.__plugins.values():
             if plugin.required_plugin and plugin.errored and plugin.enabled:
                 if (
@@ -138,32 +180,29 @@ def retry_errored_plugins(self):
                 ):
                     plugin.reload_errored()
 
-    def start(self):
+    def start(self) -> None:
         """Execute the on_start() function of plugins."""
         for plugin in self.__plugins.values():
             plugin.call_plugin_method("on_start")
-        return True
 
-    def stop(self):
+    def stop(self) -> None:
         """Execute the on_stop() function of plugins."""
         for plugin in self.__plugins.values():
             plugin.call_plugin_method("on_stop")
-        return True
 
-    def exit(self):
+    def exit(self) -> None:
         """Execute the on_exit() function of plugins."""
         for plugin in self.__plugins.values():
             plugin.call_plugin_method("on_exit")
-        return True
 
-    def pre_break(self, break_obj):
+    def pre_break(self, break_obj) -> bool:
         """Execute the on_pre_break(break_obj) function of plugins."""
         for plugin in self.__plugins.values():
             if plugin.call_plugin_method_break_obj("on_pre_break", 1, break_obj):
                 return False
         return True
 
-    def start_break(self, break_obj):
+    def start_break(self, break_obj) -> bool:
         """Execute the start_break(break_obj) function of plugins."""
         self.last_break = break_obj
         for plugin in self.__plugins.values():
@@ -172,25 +211,24 @@ def start_break(self, break_obj):
 
         return True
 
-    def stop_break(self):
+    def stop_break(self) -> None:
         """Execute the stop_break() function of plugins."""
         for plugin in self.__plugins.values():
             plugin.call_plugin_method("on_stop_break")
 
-    def countdown(self, countdown, seconds):
+    def countdown(self, countdown, seconds) -> None:
         """Execute the on_countdown(countdown, seconds) function of plugins."""
         for plugin in self.__plugins.values():
             plugin.call_plugin_method("on_countdown", 2, countdown, seconds)
 
-    def update_next_break(self, break_obj, break_time):
+    def update_next_break(self, break_obj, break_time) -> None:
         """Execute the update_next_break(break_time) function of plugins."""
         for plugin in self.__plugins.values():
             plugin.call_plugin_method_break_obj(
                 "update_next_break", 2, break_obj, break_time
             )
-        return True
 
-    def get_break_screen_widgets(self, break_obj):
+    def get_break_screen_widgets(self, break_obj) -> str:
         """Return the HTML widget generated by the plugins.
 
         The widget is generated by calling the get_widget_title and
@@ -246,14 +284,14 @@ class LoadedPlugin:
     # misc data
     # FIXME: rename to plugin_config to plugin_json? plugin_config and config are easy
     # to confuse
-    config = None
-    plugin_config = None
-    plugin_dir = None
-    module = None
-    last_error = None
-    id = None
-
-    def __init__(self, plugin):
+    config: dict
+    plugin_config: dict
+    plugin_dir: str
+    module: typing.Optional[typing.Any] = None
+    last_error: typing.Optional[typing.Union[str, PluginDependency]] = None
+    id: str
+
+    def __init__(self, plugin: dict) -> None:
         (plugin_config, plugin_dir) = self._load_config_json(plugin["id"])
 
         self.id = plugin["id"]
@@ -285,11 +323,10 @@ def __init__(self, plugin):
 
             self._import_plugin()
 
-    def reload_config(self, plugin):
-        if self.enabled and not plugin["enabled"]:
-            self.enabled = False
-            if not self.errored and utility.has_method(self.module, "disable"):
-                self.module.disable()
+    def reload_config(self, plugin: dict) -> None:
+        if not plugin["enabled"]:
+            self.disable()
+            return
 
         if not self.enabled and plugin["enabled"]:
             self.enabled = True
@@ -315,7 +352,18 @@ def reload_config(self, plugin):
                 # No longer errored, import the module now
                 self._import_plugin()
 
-    def reload_errored(self):
+    def disable(self) -> None:
+        if self.enabled:
+            self.enabled = False
+            if (
+                not self.errored
+                and self.module is not None
+                and utility.has_method(self.module, "disable")
+            ):
+                self.module.disable()
+            logging.info("Successfully unloaded the plugin '%s'", self.id)
+
+    def reload_errored(self) -> None:
         if not self.errored:
             return
 
@@ -336,10 +384,10 @@ def reload_errored(self):
                 # No longer errored, import the module now
                 self._import_plugin()
 
-    def get_name(self):
+    def get_name(self) -> str:
         return self.plugin_config["meta"]["name"]
 
-    def _import_plugin(self):
+    def _import_plugin(self) -> None:
         if self.errored:
             # do not try to import errored plugin
             return
@@ -350,7 +398,7 @@ def _import_plugin(self):
         if utility.has_method(self.module, "enable"):
             self.module.enable()
 
-    def _load_config_json(self, plugin_id):
+    def _load_config_json(self, plugin_id: str) -> typing.Tuple[dict, str]:
         # Look for plugin.py
         if os.path.isfile(
             os.path.join(utility.SYSTEM_PLUGINS_DIR, plugin_id, "plugin.py")
@@ -373,16 +421,16 @@ def _load_config_json(self, plugin_id):
 
         return (plugin_config, plugin_dir)
 
-    def init_plugin(self, context, safeeyes_config):
+    def init_plugin(self, context: Context, safeeyes_config: Config) -> None:
         if self.errored:
             return
         if self.break_override_allowed or self.enabled:
-            if utility.has_method(self.module, "init", 3):
+            if self.module is not None and utility.has_method(self.module, "init", 3):
                 self.module.init(context, safeeyes_config, self.config)
 
     def call_plugin_method_break_obj(
         self, method_name: str, num_args, break_obj, *args, **kwargs
-    ):
+    ) -> typing.Any:
         if self.errored:
             return None
 
@@ -399,7 +447,9 @@ def call_plugin_method_break_obj(
 
         return None
 
-    def call_plugin_method(self, method_name: str, num_args=0, *args, **kwargs):
+    def call_plugin_method(
+        self, method_name: str, num_args=0, *args, **kwargs
+    ) -> typing.Any:
         if self.errored:
             return None
 
@@ -412,7 +462,7 @@ def call_plugin_method(self, method_name: str, num_args=0, *args, **kwargs):
 
     def _call_plugin_method_internal(
         self, method_name: str, num_args=0, *args, **kwargs
-    ):
+    ) -> typing.Any:
         # FIXME: cache if method exists
         if utility.has_method(self.module, method_name, num_args):
             return getattr(self.module, method_name)(*args, **kwargs)

safeeyes/plugins/trayicon/plugin.py

@@ -32,7 +32,7 @@
 Safe Eyes tray icon plugin
 """
 
-tray_icon = None
+tray_icon: typing.Optional["TrayIcon"] = None
 safeeyes_config = None
 
 SNI_NODE_INFO = Gio.DBusNodeInfo.new_for_xml(
@@ -405,6 +405,14 @@ def register(self):
             cancellable=None,
         )
 
+        # Note that according to the (freedesktop) spec, we should own the name
+        # org.freedesktop.StatusNotifierItem-PID-ID and pass that to the watcher
+        # instead
+        # with the path being hardcoded at /StatusNotifierItem
+        # The spec behaviour is worse for flatpak, however, as it requires owning a
+        # pretty generic name.
+        # Note that libappindicator/ayatana also used this non-standard behaviour -
+        # this must be pretty well supported then.
         watcher.RegisterStatusNotifierItem("(s)", self.DBUS_SERVICE_PATH)
 
     def unregister(self):
@@ -441,6 +449,8 @@ class TrayIcon:
 
     _resume_timeout_id: typing.Optional[int] = None
 
+    _session_bus: Gio.DBusConnection
+
     def __init__(self, context: Context, plugin_config):
         self.context = context
         self.on_show_settings = context.api.show_settings
@@ -458,10 +468,19 @@ def __init__(self, context: Context, plugin_config):
         self.allow_disabling = plugin_config["allow_disabling"]
         self.menu_locked = False
 
-        session_bus = Gio.bus_get_sync(Gio.BusType.SESSION)
+        # This is using a separate dbus connection on purpose
+        # StatusNotifierWatcher does not have an unregister method - the spec instead
+        # says that the watcher should detect the item "going away from the bus"
+        # in practice, this means that the connection closing is detected by the watcher
+        # which can only happen if we use our own connection, and close it manually
+        self._session_bus = Gio.DBusConnection.new_for_address_sync(
+            Gio.dbus_address_get_for_bus_sync(Gio.BusType.SESSION),
+            Gio.DBusConnectionFlags.AUTHENTICATION_CLIENT
+            | Gio.DBusConnectionFlags.MESSAGE_BUS_CONNECTION,
+        )
 
         self.sni_service = StatusNotifierItemService(
-            session_bus, menu_items=self.get_items()
+            self._session_bus, menu_items=self.get_items()
         )
         self.sni_service.register()
 
@@ -475,6 +494,10 @@ def initialize(self, plugin_config):
         self.update_menu()
         self.update_tooltip()
 
+    def unregister(self) -> None:
+        self.sni_service.unregister()
+        self._session_bus.close_sync()
+
     def get_items(self):
         breaks_found = self.has_breaks()
 
@@ -864,3 +887,12 @@ def on_start():
 def on_stop():
     """Disable the tray icon."""
     tray_icon.disable_ui()
+
+
+def disable() -> None:
+    """Disable the tray icon plugin."""
+    global tray_icon
+
+    if tray_icon:
+        tray_icon.unregister()
+        tray_icon = None

safeeyes/safeeyes.py

@@ -471,7 +471,7 @@ def restart(self, config, set_active=False):
         self.break_screen.initialize(config)
 
         try:
-            self.plugins_manager.init(self.context, self.config)
+            self.plugins_manager.reload(self.context, self.config)
         except RequiredPluginException as e:
             self.show_required_plugin_dialog(e)
             return