search

Crozzers / screen_brightness_control

A Python tool for controlling the brightness of your monitor
https://crozzers.github.io/screen_brightness_control
MIT License
50 stars 9 forks source link

add `uid` field to monitor info dict #41

Closed BingoWon closed 1 month ago

BingoWon commented 1 month ago

This PR adds only uid field to monitor info dict. This PR is derived from #40

Furthermore, all the possible duplicate monitors problem will be gone without needing ALLOW_DUPLICATES in my case after this PR.

BingoWon commented 1 month ago

To fully resolve the pytest issues, I had to modify some code that is unrelated to this PR.

BingoWon commented 1 month ago

@Crozzers Yes, I agree to move uid to the very front within filter_monitors() so that the latter displays with the same edid won't be filtered out (without requiring allow_duplicates=True).

However, the term uid is misleading because it’s specific to how a display is connected on Windows, not a true display identifier. So, I think in the Display class, edid should still be prioritized as the main display identifier.

To avoid confusion, I suggest renaming uid to connection_uid to better reflect its purpose.

Aspect connection_uid edid
Definition A Windows-specific ID that tracks how a display is connected (port, connection type). Standardized metadata embedded in the display hardware
Structure Extracted from DeviceID (via win32api) or InstanceName (via WMI) on Windows. Combines connection path and port info. A 128 or 256-byte binary block with information like manufacturer ID, model, and serial number.
Consistency Changes if the display is connected to a different port or system. Remains constant for the same monitor model and hardware.

Prioritizing edid in get_identifier() within Display can lead to confusing logs due to duplicate edid in cases like mine. I think that's acceptable for now. And users can easily debug because now they are guaranteed to have access to all displays even if they encounter the same "duplicate edid" issue", thanks to the addition of connection_uid.

I'm also absolutely happy to make modifications if you prefer the term uid or prioritizing uid within Display.

Crozzers commented 1 month ago

To me, UID just means something we can use to uniquely identify a display. I'm not too fussed with how it's derived as long as it fufills that purpose. Plus on Linux it's likely to be derived in much the same way, by how it's connected to the system (i2c path, or location within /sys/).

The main reason for Display.get_identifier is to get something that can uniquely identify the display so that if that value is passed to get/set_brightness, it knows exactly which display to use. Since the Windows uid is currently the most unique thing we have, I think it makes sense to prioritise it everywhere.

As for uid vs connection_uid, I think I prefer uid as it's shorter, easier to type out in code and abstracts away how that UID is inferred.

Crozzers commented 1 month ago

LGTM. Thanks for this. I'll do some testing on Linux and see if we can get a UID equivalent before doing a release