Make fade_brightness() thread stoppable for concurrent requests

[BingoWon]

PR Summary

This PR introduces enhancements to the fade_brightness() method in the Display class, aimed at improving how multiple brightness fade requests are handled. Previously, executing several fade_brightness() calls in rapid succession risked initiating overlapping non-blocking threads, potentially causing display flickering. The updates ensure that only the most recent fade request is executed, while all earlier requests are efficiently terminated.

Key Changes

• One Physical Display -> One Display Instance -> One Fade Thread

• Singleton Pattern for Display Instances:

  • The PR enforces a singleton pattern for Display instances, ensuring that each physical display is associated with a unique Display instance. These instances are managed within a dictionary, _instances, centralizing the control of fade operations and ensuring coordinated execution across displays.

• Thread Management with _fade_thread:

  • An _fade_thread attribute has been introduced within each Display instance, allowing for the tracking of the thread currently handling the fade operation. This mechanism is crucial for identifying and halting any preceding fade operations when a new fade request is made, ensuring that only the latest request remains active.

• Method Refactoring:

  • The original fade_brightness() method has been renamed to fade_brightness_thread(), which now solely carries out the fade logic within a dedicated thread.

• Unified Fade Adjustment Invocation:

  • There are two ways to invoke the fade brightness adjustment: sbc.fade_brightness() and sbc.Display.fade_brightness(). Regardless of the method used, both share the same exclusive thread for a given display.

Example Program Demonstrating the PR Benefits

import tkinter as tk
import screen_brightness_control as sbc
import threading
import time

sbc.config.ALLOW_DUPLICATES = True
first_two_monitors = sbc.list_monitors_info()[:2]
display_0 = sbc.Display.from_dict(first_two_monitors[0])
display_1 = sbc.Display.from_dict(first_two_monitors[1])


def make_slider(label, command=None, extra_args=()):
    slider = tk.Scale(root, from_=0, to=100, length=200, orient=tk.HORIZONTAL, command=lambda val: command(val, *extra_args) if command else None)
    slider.pack()
    tk.Label(root, text=label).pack()
    return slider


def safe_set(slider, value):
    slider.config(state=tk.NORMAL)
    slider.set(value)
    slider.config(state=tk.DISABLED)


def adjust_brightness(val, display):
    # Adjust the brightness of a single monitor, using display.fade_brightness()
    display.fade_brightness(int(val), blocking=False)


def adjust_brightness_all(val):
    # Adjust the brightness of all monitors, using sbc.fade_brightness()
    sbc.fade_brightness(int(val), blocking=False, haystack=first_two_monitors)


def update_current_brightness():
    while True:
        current_brightness_0 = display_0.get_brightness()
        current_brightness_1 = display_1.get_brightness()
        thread_count = threading.active_count()
        try:
            safe_set(slider_current_0, current_brightness_0)
            safe_set(slider_current_1, current_brightness_1)
            thread_count_label.config(text=f"Thread Count: {thread_count}")
        except Exception as e:
            print(e)
        time.sleep(0.1)


root = tk.Tk()
root.title("Brightness Control")
root.attributes('-topmost', 1)

# Label for thread count
thread_count_label = tk.Label(root, text="Thread Count: 0", fg="white", bg="blue")
thread_count_label.pack()

# Slider for current_brightness
slider_current_0 = make_slider("Display 0 Current Brightness")
slider_current_1 = make_slider("Display 1 Current Brightness")

# Slider for user inputs
slider_user_all = make_slider("Display 0&1 Use sbc.fade_brightness()", adjust_brightness_all)
slider_user_0 = make_slider("Display 0 Use Display.fade_brightness()", adjust_brightness, extra_args=(display_0,))
slider_user_1 = make_slider("Display 1 Use Display.fade_brightness()", adjust_brightness, extra_args=(display_1,))

# Update the current brightness and thread count in a separate thread
threading.Thread(target=update_current_brightness, daemon=True).start()

root.mainloop()

[Crozzers]

Thanks for this. I left some comments and if you could add some test coverage as well, that would be great

[Crozzers]

Not sure there's a use case for users invoking this function directly so it might make more sense as an internal function (eg: __fade_brightness). Also means we wouldn't have to maintain a near identical docstring compared to fade_brightness

[BingoWon]

Double underscores in Python cause name mangling, which changes the attribute's name internally. This made pytest fail because it couldn't find the attribute. So, I will use a single underscore instead. It allows pytest to access the attribute while still suggesting it's not for direct access.

[Crozzers]

Both fade_brightness and fade_brightness_thread return the new brightness, resulting in two calls to get_brightness.

Since returning the new brightness is deprecated for fade_brightness anyway, this one should probably return None

[Crozzers]

If a display can be accessed via multiple methods (eg: via linux.I2C and linux.DDCUtil), the args/kwargs would largely be the same except from the method and maybe index. In that case you could initialise two Display instances for the same physical monitor and run into the same flickering issue, especially if you're using allow_duplicates.

Maybe you could use the result from get_identifier as the dict keys?

[BingoWon]

Sorry that I didn't know index is strictly associated with method.
If a display can be accessed via multiple methods, filter_monitor(allow_duplicates=True) will return duplicate indices, and the actual duplicate displays info have different method and maybe index. Am I right?
If yes, there must be duplicate indices, and my PR about filter_monitor() can't handle this properly, and I think there is a discrepancy in the definition of "duplicates".
Is there any preference when choosing a display from two different method?

[Crozzers]

If a display can be accessed via multiple methods, filter_monitor(allow_duplicates=True) will return duplicate indices, and the actual duplicate displays info have different method and maybe index. Am I right?

Yep that's right. They should have most of the same other info, which is how we try to filter them out in filter_monitors

my PR about filter_monitor() can't handle this properly, and I think there is a discrepancy in the definition of "duplicates"

I'm not sure what you mean about the definition of "duplicate". To me, "duplicate" is when we detect the same display twice (or more), usually via different methods.
In the case of your two monitors, there's no way to tell them apart based on their reported information. As far as the library knows, they are the same display but it's detected it twice.

Is there any preference when choosing a display from two different method?

The _OS_METHODS tuple has them in order. Doesn't matter as much on Windows but does on Linux

[BingoWon]

When duplicates are allowed, current Display.get_identifier() cannot provide a valid identifier, which presents a significant systematic issue and will break the code. However, the potential flickering issues that may arise from current implementation can be manually avoided by the user. Here's a more detailed explanation:

Duplicate displays can occur due to:

1. the same display can be accessed via multiple methods;
2. The same monitor is connected through different interfaces at the same time;
3. Different monitors share identical identifiers, including EDID, due to careless manufacturer.

The first scenario leads to duplicate index properties and the last two lead to duplicate edid. This means when duplicates are allowed, none of existing identifiers can guarantee uniqueness. method+index might be a potential solution, which requires further discussion, but Display.get_identifier() seems to be less necessary than it used to be.

[Crozzers]

When duplicates are allowed, current Display.get_identifier() cannot provide a valid identifier

Duplicates being allowed is very much an edge case and is not fully supported. In my mind it's like saying rm -rf instead of rm. At that point, the user has said they know best and they will handle the consequences.

Looking at the scenarios mentioned:

1. Solved by disallowing duplicates and allowing filter_monitors to do its thing
2. Not considered a valid use case
   • Not sure if there's a good reason to connect a single display to a machine via 2 different cables?
3. These displays cannot be told apart. As far as the software knows, they are the same display

The library cannot enforce a singleton pattern if it can't uniquely identify the displays, and it can't do that when allow_duplicates is enabled. This is not a scenario I want to support and at that point, it's down to the user to sort that out manually.

[BingoWon]

Thank you for your feedback.
The only usage of singleton pattern is for _fade_thread, the instance variable within sbc.Display. It doesn't modify any existing user behaviors or introduce any new potential issues.
In terms of uniqueness, neither the existing code nor this PR can guarantee it. However, I believe that ensuring uniqueness should be a goal for future development.

3. These displays cannot be told apart. As far as the software knows, they are the same display

Regarding the third point, it's important to note that Windows does not distinguish displays based on their hardware information, but rather by the interface they are connected to. Therefore, in the last two scenarios, these are neither duplicates nor same display from a Windows perspective.

Singleton pattern can surely be replaced with other implementations. I chose it for the potential benefits it could provide in the future.

Duplicates being allowed is very much an edge case

I understand that allowing duplicates is considered an edge case. However, I believe there are users, like me, who find this feature useful. I appreciate your patience with my previous PR. Besides, I've come to realize that the current implementations around duplicates are not sufficient and I agree that a more incremental approach is a better choice for now.
For this PR, could you please specify what changes you would like to see in the next commit?

[Crozzers]

these are neither duplicates nor same display from a Windows perspective

If it's possible to get a useful unique identifier from windows, that's great. We can use that in get_identifier instead. Last time I looked at this I couldn't find anything that fit the bill, but that was a while ago and this should be re-evaluated. I'll have another read of the MS docs.
Ideally Linux would have something similar, but there's no guarantees.

I believe there are users, like me, who find this feature useful

I agree. The stoppable threads bit is really nice. The singleton bit would be nice to have, but it requires us to be able to reliably uniquely identify monitors.

the current implementations around duplicates are not sufficient

Current implementations for detecting and de-duplicating monitors? Alright but not great.
Current implementation for allowing duplicates? Not supported and not planned to be supported. My approach instead would be to better identify monitors so that we can tell them apart (maybe by using some windows GUID in the future).

For this PR, could you please specify what changes you would like to see in the next commit?

Keeping track of the thread within the instance is a good safeguard, but the user will have to manage the class instance on their own without a singleton.

The issue is that we can't enforce this pattern reliably if we can't uniquely identify monitors reliably.
If ALLOW_DUPLICATES==False and get_identifier falls back to the index, that doesn't work.
If ALLOW_DUPLICATES==False and one method can fetch the display's serial, whereas another can only fetch the EDID, that doesn't work.
If ALLOW_DUPLICATES==True and one display can be addressed via multiple methods, that doesn't work either.

I think implementing a safe guard that sometimes works and sometimes doesn't will cause issues, and if I were a user I would have a hard time trusting it.
I like the singleton idea and would love to revisit it in the future, but, at the moment the foundations for it aren't there.

[BingoWon]

Thank you for your feedback.
The latest commit removes singleton pattern （and its test code）.
While using method + index can't guarantee uniqueness, it is the best solution I can think of to identify the same display.

[Crozzers]

Thanks for this, and thanks for your patience during the review process