search

haimgel / display-switch

Turn a $30 USB switch into a full-featured multi-monitor KVM switch
https://haim.dev/posts/2020-07-28-dual-monitor-kvm/
MIT License
2.83k stars 110 forks source link

display_control: Make display_name include monitor name, serial number, etc #56

Closed pm215 closed 3 years ago

pm215 commented 3 years ago

Hi; this pull request adds information to the display_name of each monitor, as I suggested in https://github.com/haimgel/display-switch/issues/54 .

Currently the display_name() function uses only the 'id' field of the ddc_hi::DisplayInfo struct, plus a possible numeric suffix to disambiguate otherwise-identical display names. At least on Linux, this leaves a lot of useful information out of the display name, because for the i2c-dev backend to ddc_hi the 'id' string is just the decimal representation of the (major,minor) of the i2c device node (e.g., '22789' == 0x5905 == device (59,05) == /dev/i2c-5).

There is usually more usefully identifying information in the DisplayInfo structure, including the 3-character manufacturer ID, the model name, and the monitor serial number. The pullreq adds this information to 'id' if it is present. The result is that instead of logging like

  Display '22790' is currently set to Custom(0x1111)

you get

  Display '22790 DEL DELL U2719D DT86VS2' is currently set to Custom(0x1111)

and you can also write monitor_id config file settings that match against the monitor model or serial number.

I guess in theory if users have config files with very short strings in their monitor_id settings then adding the extra info might cause them to match on monitors that previously didn't match; but this seems fairly unlikely in practice to me.

I'm not sure what Windows and OSX monitor ID strings look like, but adding the extra info doesn't seem like it could hurt.

NB: There are a lot of ways you could write the string-assembly in this function (eg heavy use of iterators); as somebody fairly new to rust I went for something that seemed to me fairly straightforward and easy to understand, and more-or-less in line with how the previous version of the function did things. Let me know if you'd like me to have a go at writing this in a different style.

haimgel commented 3 years ago

Thanks for the PR! Yeah, Linux support is sub-par, comparing to MacOS and Windows, I personally use just these too and have no easy way to test on Linux.

So, on macOS and Windows display.info.id is already "good enough", for example for my displays it shows "LEN P27u-10 S/N 1144206897" -- as you see, this includes manufacturer, model and the serial number already. If we are going to append these again, it's going to be a bit too much... I also think this "string building" is a bit un-rusty.

I don't think this should be merged as-is, but this is a good start to make the display_name behave reasonably on all platforms.