TagStudioDev / TagStudio

A User-Focused Photo & File Management System
GNU General Public License v3.0
3.21k stars 277 forks source link

Use of lambda functions when connecting to Qt signals #131

Open gabrieljreed opened 2 months ago

gabrieljreed commented 2 months ago

I've noticed that across the codebase, signals are connected using lambda functions. While they certainly work, and can seem like a good solution for properly connecting signals to functions (especially with arguments), they do have some downsides.

Memory safety

Lambda functions can cause QWidgets to not be properly garbage collected. (See this forum post and this StackOverflow thread). This can lead to some unexpected behavior and/or the application consuming more resources than expected.

Code cleanliness/readability

While it's true that

self.my_button.clicked.connect(lambda: self.my_button_pressed())

does work,

self.my_button.clicked.connect(self.my_button_pressed)

is functionally equivalent, along with being cleaner and easier to read. Additionally, while lambda functions can be extremely useful in the right situations, they can over-complicate what would otherwise be a simple operation.

Fix

Simple cases can just drop the lambda and actual function call in favor of a reference to the function. This is actually the intended way to use signals (see this example from the Qt for Python docs).

Example:

# Original
open_library_action.triggered.connect(lambda: self.open_library_from_dialog())

# Updated
open_library_action.triggered.connect(self.open_library_from_dialog)

More complicated cases (ex: a function needs to be called with specific arguments) can be replaced with functools.partial, which is part of the Python standard library. partial creates a "partial" function call, with pre-filled arguments.

Example:

# Original
self.edit_modal.saved.connect(lambda: self.edit_tag_callback(btp))

# Updated
from functools import partial

self.edit_modal.saved.connect(partial(self.edit_tag_callback, btp))
gabrieljreed commented 2 months ago

I can start updating this, I just wanted to do my due diligence and submit an issue first.

Loran425 commented 2 months ago

This seems like a solid improvement. Great write up to explain the issue and the fix.