[Bug]: Using `translate_qobject()` Causes Resource Leak

[CyanVoxel]

Checklist

• I am using an up-to-date version.
• I have read the documentation.
• I have searched existing issues.

TagStudio Version

Alpha 9.5.0 PR1 (main branch)

Operating System & Version

macos 15.1.1

Description

As discovered while debugging #733 and explained in #735, there seems to be a resource leak stemming from the use of the translate_qobject() method. Under the circumstances of #733 the method was used to translate the context menu of TagWidgets that would frequently be deleted and recreated. The issue resulted in slower and slower application respond times and an increase in system memory usage.

Expected Behavior

Translating a QObject via translate_qobject() should not result in a permanent application slowdown and/or unfreeable memory allocation.

Steps to Reproduce

Follow steps in #733 under that TagStudio version

Logs

No response

[Tishj]

The differences in this path between translate_qobject and translate_formatted are:

1. Fetching the latest version of the translated string (i.e switching language support)

        if key in self._strings:
            self._strings[key].changed.connect(lambda text: setter(self.__format(text, **kwargs)))

2. Using a setter callback

setter(self.translate_formatted(key, **kwargs))

Rather than just returning the value, which is what translate_formatted does

My money is on the first point being the issue
I suspect the connect we're making is never being cleaned up, which makes every subsequent connect slower, probably because it's adding to some internal array of connections that it's ready to emit new values to

The SignalInstance has 3 methods:

class SignalInstance(builtins.object)
 |  Methods defined here:
 |
 |  __call__(self, /, *args, **kwargs)
 |      Call self as a function.
 |
 |  __getitem__(self, key, /)
 |      Return self[key].
 |
 |  __repr__(self, /)
 |      Return repr(self).
 |
 |  connect(...)
 |
 |  disconnect(...)
 |
 |  emit(...)
 |
 |  ----------------------------------------------------------------------
 |  Static methods defined here:
 |
 |  __new__(*args, **kwargs)
 |      Create and return a new object.  See help(type) for accurate signature.

[Tishj]

diff --git a/tagstudio/src/qt/translations.py b/tagstudio/src/qt/translations.py
index 5d2dcea..51c7237 100644
--- a/tagstudio/src/qt/translations.py
+++ b/tagstudio/src/qt/translations.py
@@ -1,5 +1,5 @@
 from pathlib import Path
-from typing import Callable
+from typing import Callable, Optional
 
 import structlog
 import ujson
@@ -56,23 +56,27 @@ class Translator:
         for k in self._strings:
             self._strings[k].value = translated.get(k, None)
 
-    def translate_qobject(self, widget: QObject, key: str, **kwargs):
+    def translate_qobject(self, widget: QObject, key: str, **kwargs) -> Optional[object]:
         """Translates the text of the QObject using :func:`translate_with_setter`."""
         if isinstance(widget, (QLabel, QAction, QPushButton, QMessageBox, QPushButtonWrapper)):
-            self.translate_with_setter(widget.setText, key, **kwargs)
+            return self.translate_with_setter(widget.setText, key, **kwargs)
         elif isinstance(widget, (QMenu)):
-            self.translate_with_setter(widget.setTitle, key, **kwargs)
+            return self.translate_with_setter(widget.setTitle, key, **kwargs)
         else:
             raise RuntimeError
 
-    def translate_with_setter(self, setter: Callable[[str], None], key: str, **kwargs):
-        """Calls `setter` everytime the language changes and passes the translated string for `key`.
-
+    def translate_with_setter(self, setter: Callable[[str], None], key: str, **kwargs) -> Optional[object]:
+        """
+        Calls `setter` everytime the language changes and passes the translated string for `key`.
+        Returns the slot created to respond to emitted translation signals. (used to disconnect from said signal)
         Also formats the translation with the given keyword arguments.
         """
-        if key in self._strings:
-            self._strings[key].changed.connect(lambda text: setter(self.__format(text, **kwargs)))
         setter(self.translate_formatted(key, **kwargs))
+        if key in self._strings:
+            # FIXME: 'key' not being in self._strings is an internal error, no?
+            # strings aren't added at runtime, they should always be present from the getgo
+            return self._strings[key].changed.connect(lambda text: setter(self.__format(text, **kwargs)))
+        return None
 
     def __format(self, text: str, **kwargs) -> str:
         try:
diff --git a/tagstudio/src/qt/widgets/tag.py b/tagstudio/src/qt/widgets/tag.py
index 22b3ed3..a878402 100644
--- a/tagstudio/src/qt/widgets/tag.py
+++ b/tagstudio/src/qt/widgets/tag.py
@@ -118,6 +118,8 @@ class TagWidget(QWidget):
         self.lib: Library | None = library
         self.has_edit = has_edit
         self.has_remove = has_remove
+        # Connection objects that are listening for emitted signals
+        self.connections: typing.List[typing.Tuple[object, typing.Callable]] = []
 
         # if on_click_callback:
         self.setCursor(Qt.CursorShape.PointingHandCursor)
@@ -133,7 +135,11 @@ class TagWidget(QWidget):
             self.bg_button.setText(escape_text(tag.name))
         if has_edit:
             edit_action = QAction(self)
-            edit_action.setText(Translations.translate_formatted("generic.edit"))
+            
+            translate_slot = Translations.translate_qobject(edit_action, 'generic.edit')
+            self.connections.append(
+                (translate_slot, Translations._strings['generic.edit'].changed.disconnect)
+            )
             edit_action.triggered.connect(on_edit_callback)
             edit_action.triggered.connect(self.on_edit.emit)
             self.bg_button.addAction(edit_action)
@@ -143,7 +149,10 @@ class TagWidget(QWidget):
         # TODO: This currently doesn't work in "Add Tag" menus. Either fix this or
         # disable it in that context.
         self.search_for_tag_action = QAction(self)
-        self.search_for_tag_action.setText(Translations.translate_formatted("tag.search_for_tag"))
+        translate_search_for_tag_slot = Translations.translate_qobject(self.search_for_tag_action, "tag.search_for_tag")
+        self.connections.append(
+            (translate_search_for_tag_slot, Translations._strings['tag.search_for_tag'].changed.disconnect)
+        )
         self.bg_button.addAction(self.search_for_tag_action)
         # add_to_search_action = QAction(self)
         # add_to_search_action.setText(Translations.translate_formatted("tag.add_to_search"))
@@ -212,7 +221,10 @@ class TagWidget(QWidget):
             )
             self.remove_button.setMinimumSize(18, 18)
             self.remove_button.setMaximumSize(18, 18)
-            self.remove_button.clicked.connect(self.on_remove.emit)
+            on_remove_slot = self.remove_button.clicked.connect(self.on_remove.emit)
+            self.connections.append(
+                (on_remove_slot, self.remove_button.clicked.disconnect)
+            )
 
         if has_remove:
             self.inner_layout.addWidget(self.remove_button)
@@ -221,7 +233,10 @@ class TagWidget(QWidget):
         # NOTE: Do this if you don't want the tag to stretch, like in a search.
         # self.bg_button.setMaximumWidth(self.bg_button.sizeHint().width())
 
-        self.bg_button.clicked.connect(self.on_click.emit)
+        on_click_slot = self.bg_button.clicked.connect(self.on_click.emit)
+        self.connections.append(
+            (on_click_slot, self.bg_button.clicked.disconnect)
+        )
 
     def enterEvent(self, event: QEnterEvent) -> None:  # noqa: N802
         if self.has_remove:
@@ -235,6 +250,10 @@ class TagWidget(QWidget):
         self.update()
         return super().leaveEvent(event)
 
+    def __del__(self):
+        for connection, disconnect in self.connections:
+            disconnect(connection)
+
 
 def get_primary_color(tag: Tag) -> QColor:
     primary_color = QColor(

Made a quick attempt at solving this, or at least enough to try to confirm that this is actually what the problem is
Basically connect returns a Connection, disconnect can be used without passing any object, but that will clear all connections, which is probably not what we want to do.

When it's given the Connection that was created through connect it will only disconnect that single Connection.
So the TagWidget class now collects these connections and the associated disconnect callbacks, and calls them upon destruction of the TagWidget

[CyanVoxel]

Made a quick attempt at solving this, or at least enough to try to confirm that this is actually what the problem is

Thank you, I'll take a look at this and see if I can get it replicates the same behavior

[Tishj]

Also this patch was made from https://github.com/TagStudioDev/TagStudio/tree/fix-tag-ux kind of accidentally, hope that means it still applies to main

We probably only want to do this disconnecting behavior for translations, since those are global objects, the others are much more ephemeral so the connections will probably die without help