Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make index Unique and Ordered by win32api.EnumDisplayMonitors() #40

Closed
wants to merge 1 commit into from

Conversation

BingoWon
Copy link
Contributor

What’s this about:

I made a small change so that the index for each monitor is unique and matches the order from win32api.EnumDisplayMonitors(). The order actually matters in some cases, and I think most people expect index to be unique already.

What’s changed:

  • Monitors found in EnumDisplayMonitors() will get their index based on that order.
  • If a monitor isn’t found (like when a laptop display is asleep), it gets the next available index, to keep things unique.

Quick question:

Do you think this is a good idea for handling index? I’m not sure how this will work on Linux yet – I’ve looked at the code in linux.py, and it seems a bit overwhelming, so I’m not sure if this approach will be possible there. If you think it’s worth pursuing, I’ll dive into that too. Let me know what you think!

@Crozzers
Copy link
Owner

Having a unique index is an idea I can get behind. However, currently the monitors are ordered such that the primary display is first. This lets users assume that index 0 refers to the main display, 1 as the secondary and so on. If we order the displays according to EnumDisplayMonitors we lose this.
Is it possible to keep the old ordering or is there a particular reason to order by EnumDisplayMonitors?

Regarding Linux, it lacks anything like win32api so it doesn't really order the displays. I think XRandr and DDCUtil make an effort to report the primary display first, so that will get reflected in the output, but if you're using the SysFiles or I2C methods, they don't do any sorting/ordering.

@BingoWon
Copy link
Contributor Author

Yes, the primary display is listed first by default. However, after testing by unplugging and replugging the primary display, it comes back as the last in all three lists: win32api, wmi, and our sbc. This shows that the order is hard to guarantee consistently, so I think the behavior of SysFiles and I2C is fine, and their indices are already unique.

Since asleep monitors might be skipped, the order from wmi can't guarantee continuous indices, which could lead to confusion. If we can avoid omitting asleep monitors or if discontinuous indices are acceptable, I'm happy to use the indices directly from wmi.

The main reason I started looking into this indexing issue is because I wanted to use the monitor coordinates from win32api, but I couldn't link the monitors in our monitor_info with those from win32api since they don’t share any common attributes. EDID isn’t directly accessible from win32api and can also be duplicated in my case. I initially thought the index might solve this issue, but I just discovered the existence of UID in both win32api and wmi.

The UID is always unique because it’s tied to the connection interface, but it's not perfect—it doesn’t account for different displays using the same port. However, if we include the UID, or better yet, incorporate win32api or wmi objects directly into the info dictionary, that would definitely help.

import win32api, wmi, re
import screen_brightness_control as sbc


def extract_uid(device_id):
    """Extract UID from  string."""
    match = re.search(r"UID(\d+)", device_id)
    return match.group(1) if match else None


for monitor_enum in win32api.EnumDisplayMonitors():
    print(monitor_enum)
    pyhandle = monitor_enum[0]
    monitor_info = win32api.GetMonitorInfo(pyhandle)
    device = win32api.EnumDisplayDevices(monitor_info["Device"], 0, 1)
    device_id = device.DeviceID
    UID = extract_uid(device_id)
    print(f"{UID=}\t{device_id=}")

print("-" * 100)
wmi = wmi.WMI(namespace="wmi")
for monitor in wmi.WmiMonitorDescriptorMethods():
    instance_name = monitor.InstanceName
    UID = extract_uid(instance_name)
    print(f"{UID=}\t{instance_name=}")


print("-" * 100)
for monitor in sbc.list_monitors_info(allow_duplicates=True):
    print(monitor)
OUTPUTS
(<PyHANDLE:66393361>, <PyHANDLE:0>, (0, 0, 1920, 1080))
UID='4355'	device_id='\\\\?\\DISPLAY#IPS2800#5&21344eb6&0&UID4355#{e6f07b5f-ee97-4a90-b076-33f57bf4eaa7}'
(<PyHANDLE:1971683>, <PyHANDLE:0>, (0, -1080, 1920, 0))
UID='4353'	device_id='\\\\?\\DISPLAY#IPS2800#5&21344eb6&0&UID4353#{e6f07b5f-ee97-4a90-b076-33f57bf4eaa7}'
(<PyHANDLE:503714509>, <PyHANDLE:0>, (-1080, -840, 0, 1080))
UID='4357'	device_id='\\\\?\\DISPLAY#RTK0000#5&21344eb6&0&UID4357#{e6f07b5f-ee97-4a90-b076-33f57bf4eaa7}'
----------------------------------------------------------------------------------------------------
UID='4353'	instance_name='DISPLAY\\IPS2800\\5&21344eb6&0&UID4353_0'
UID='4355'	instance_name='DISPLAY\\IPS2800\\5&21344eb6&0&UID4355_0'
UID='4357'	instance_name='DISPLAY\\RTK0000\\5&21344eb6&0&UID4357_0'
----------------------------------------------------------------------------------------------------
{'name': 'None Generic Monitor', 'model': 'Generic Monitor', 'serial': '', 'manufacturer': None, 'manufacturer_id': 'IPS', 'edid': '00ffffffffffff00261300280000000020210104c53e23782987e5a456509e260d5054210800d1c0010181c0a9c0010101010101010150d000a0f0703e80302035006b5c2100001e000000fc005532383047410a202020202020000000fd003090ffff8c010a202020202020000000ff000a20202020202020202020202002bc', 'method': <class 'screen_brightness_control.windows.VCP'>, 'index': 1}
{'name': 'None Generic Monitor', 'model': 'Generic Monitor', 'serial': '', 'manufacturer': None, 'manufacturer_id': 'IPS', 'edid': '00ffffffffffff00261300280000000020210104c53e23782987e5a456509e260d5054210800d1c0010181c0a9c0010101010101010150d000a0f0703e80302035006b5c2100001e000000fc005532383047410a202020202020000000fd003090ffff8c010a202020202020000000ff000a20202020202020202020202002bc', 'method': <class 'screen_brightness_control.windows.VCP'>, 'index': 0}
{'name': 'None Generic Monitor', 'model': 'Generic Monitor', 'serial': 'demoset-1\n 0', 'manufacturer': None, 'manufacturer_id': 'RTK', 'edid': '00ffffffffffff004a8b00000000000028210104b53c21782bea4fad5141ac260e5357210800d1c001010101950090408180814081c040d000a0f0703e803020350055502100001a000000fc005532373041430a202020202020000000ff0064656d6f7365742d310a203020000000fd00304b919146010a2020202020200127', 'method': <class 'screen_brightness_control.windows.VCP'>, 'index': 2}

@Crozzers
Copy link
Owner

This shows that the order is hard to guarantee consistently, so I think the behavior of SysFiles and I2C is fine, and their indices are already unique

Just checked this on my machine and it looks like the current behaviour of the library lists my secondary monitor first anyway, which I don't think it used to do.

So yeah, the order apparently can't be guarenteed on Windows or Linux so I'm happy to get rid of this expectation.
I'll have to update a few pages of documentation where this is assumed but that's fine.

the order from wmi can't guarantee continuous indices, which could lead to confusion. If we can avoid omitting asleep monitors or if discontinuous indices are acceptable

I'm happy to allow discontinuous indices in this case. I think it makes some intuitive sense rather than one monitor going to sleep and all the numbers shuffling around and getting reassigned to awake monitors.

The UID is always unique because it’s tied to the connection interface, but it's not perfect—it doesn’t account for different displays using the same port.

What kind of scenario would have monitors connected to the same port? Would that be through a splitter/adapter? Might be worth testing that scenario and seeing what WMI/win32api spits out. I'll see if I have any splitters lying around I can mess with


The UID idea seems promising, and could solve alot of the hassle of uniquely identifying monitors if Windows just handles it for us.

On Linux, SysFiles could use the /sys/... path and I2C could use the /dev/i2c- path as an ID. DDCUtil I think also reports the /dev/i2c- path so that works together. Light is deprecated and XRandr will probably also go away with the transition to Wayland, so that all works out quite nicely in theory.

I'll play around with it as well, but for now the proposed changes on this branch make sense to me. I'll do some testing locally and let you know if there are any issues. Otherwise, LGTM

@Crozzers
Copy link
Owner

So I've done some playing around with this and found a couple of things.

First, I've taken a look at the current code and how displays are ordered, and they're ordered according to the return of windows.enum_display_devices, which enumerates over EnumDisplayMonitors internally.

Second is an issue with how index is scoped.

Currently, index is scoped per method. So a WMI display and a VCP display can both have an index of 0. This is intentional and is used internally to choose which display to alter. For example, in WMI.set_brightness:

def set_brightness(cls, value: IntPercentage, display: Optional[int] = None):
brightness_method = _wmi_init().WmiMonitorBrightnessMethods()
if display is not None:
brightness_method = [brightness_method[display]]
for method in brightness_method:
method.WmiSetBrightness(value, 0)

In VCP it's a similar story except the index is used as a counter when iterating over monitor handles.

The problem is that this PR makes the index "global" scoped across all methods, and also reliant on the order that EnumDisplayMonitors returns them in, which can change.

Consider a scenario where we have 1 laptop screen and 1 external monitor and the external monitor is enumerated first. The laptop screen will get an index of 1, but WMI can only see one addressable display. When it tries to access displays[1] it will get an index error.

Unfortunately, because of this I don't think this PR is feasible. On Windows and Linux the index is method scoped and changing this would require extensive re-writes in terms of how each method resolves an index which is no longer directly connected to the displays it can see.

In terms of including a unique identifer, I'm working on getting an HDMI splitter to see how Windows assigns the UID property in that case, and I'm also playing around with display tiling. If Windows handles it correctly, we may be able to use the UID that is shown in the display's InstanceName

@BingoWon
Copy link
Contributor Author

@Crozzers

You are absolutely right on index. I was surprised and felt like:
a_trap__

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants