-
Notifications
You must be signed in to change notification settings - Fork 3k
w32_common: Implement --screen-name #16098
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
base: master
Are you sure you want to change the base?
Conversation
Nice. Could you also extend |
Download the artifacts for this pull request: |
82b9487
to
f9cecec
Compare
@kasper93 Do you want to just change it from the GDI names to the friendly names completely? That seems easy enough. |
Not sure, there may be users who wants GDI names. Maybe we do |
Sounds reasonable. I'll poke at it tomorrow. |
374f2bd
to
f15318a
Compare
Added the function func(_, value)
local display_names = mp.get_property("display-names")
if display_names then
print("Display names:\n" .. display_names .. "\n")
end
if value then
print("Friendly display names:\n" .. value .. "\n")
end
end
mp.observe_property("display-names/friendly", "string", func) |
end: | ||
talloc_free(ctx); | ||
return monitor_friendly_device_name; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move common part to separate function, just to avoid duplication, even if it's not that much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yeah it seems like there should be a way to combine these, but since they use two different query structs, DISPLAYCONFIG_SOURCE_DEVICE_NAME
and DISPLAYCONFIG_TARGET_DEVICE_NAME
, there's not much overlap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could move lines 230-248 and 190-204 to separate functions and call in common function based on flag which one name you want to get.
f15318a
to
4d9b7eb
Compare
Previously the --screen-name and --fs-screen-name options did nothing on Windows. It now attempts to convert a friendly monitor name to the GDI device name using the DisplayConfig API first.
The `friendly` subkey return the same set of displays as `display-names` but it attempts to convert the names to the human-friendly name that DisplayConfig obtains from the EDID.
4d9b7eb
to
e740125
Compare
end: | ||
talloc_free(ctx); | ||
return monitor_friendly_device_name; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could move lines 230-248 and 190-204 to separate functions and call in common function based on flag which one name you want to get.
static BOOL CALLBACK get_monitor_by_name_proc(HMONITOR mon, HDC dc, LPRECT r, | ||
LPARAM p) | ||
{ | ||
struct get_monitor_by_name_data *const data = (struct get_monitor_by_name_data*)p; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
struct get_monitor_by_name_data *const data = (struct get_monitor_by_name_data*)p; | |
struct get_monitor_by_name_data *const data = (struct get_monitor_by_name_data *)p; |
{ | ||
struct get_monitor_by_name_data *const data = (struct get_monitor_by_name_data*)p; | ||
|
||
MONITORINFOEXW mi = { .cbSize = sizeof mi }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MONITORINFOEXW mi = { .cbSize = sizeof mi }; | |
MONITORINFOEXW mi = { .cbSize = sizeof(mi) }; |
struct get_monitor_by_name_data *const data = (struct get_monitor_by_name_data*)p; | ||
|
||
MONITORINFOEXW mi = { .cbSize = sizeof mi }; | ||
if (GetMonitorInfoW(mon, (MONITORINFO*)&mi)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (GetMonitorInfoW(mon, (MONITORINFO*)&mi)) { | |
if (GetMonitorInfoW(mon, (LPMONITORINFO)&mi)) { |
return TRUE; | ||
} | ||
|
||
static HMONITOR get_monitor_by_name(const wchar_t* name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static HMONITOR get_monitor_by_name(const wchar_t* name) | |
static HMONITOR get_monitor_by_name(const wchar_t *name) |
static void disp_names_append(HMONITOR mon, struct disp_names_data *data) | ||
{ | ||
MONITORINFOEXW mi = { .cbSize = sizeof mi }; | ||
if (GetMonitorInfoW(mon, (MONITORINFO*)&mi)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remarks as before.
``display-names/friendly`` | ||
These are the human-friendly names obtained from the monitor EDID | ||
(Dell AW3223DW, LC34G55T, etc.). These follow the same order as | ||
``display-names`` (Windows only) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might it be worth making the property available on all systems, but merely act as a duplicate of display-names
for non-windows systems? That way scripts, keybinds, and auto-profiles don't need to worry about the platform when grabbing useful display names, they could just grab display-names/friendly
and have it work on all systems.
Edit: or would this impact adding friendlier names for other platforms later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On linux, the display names are already friendly (i.e. like DP-1
). I think macOS is similar. As opposed to "friendly" names, this seems to be more like the model name of the display. It's certainly possible to add it to other platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can handle this new VOCTRL everywhere, for now with the same impl, and extended if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just as info, on macOS it's something like DELL U2723QE (12343224)
, name + serial number.
f7a32b5
just wanted to mention, when i changed the display names on macOS in f7a32b5 i had the problem that those 'friendly' names are not deterministic, see the commit. eg same model displays have the same name or were numerated 'randomly'. that's the reason why i added the serial number to those names, to make them unique. you might have the same problem here. |
Yes, exactly, that's why I suggested to keep GDI path that and friendly name too. |
Having multiple names exposed for the same display makes sense to me. How do we want to separate it? The existing |
So Though, I once was thinking about it long time ago and wanted to do And I agree, this is good opportunity to unify it little bit between macOS, Wayland and Windows. |
I think it would be better to append the serial number to the monitor name, while maintaining consistency with the behavior on macos.
I don't think it's a good idea to append the GDI path to the monitor name. As mentioned in #13743, the GDI path assigned to each monitor by the Windows system is not fixed, it changes for various reasons. This makes the property will remain unusable for persistent configuration. Glad to know it's an abandoned idea. |
for duplicate name also keep runtime in mind, eg you plug in the first display it would be ["Display"], plugging in a second one of the same model it would be ["Display (1)", "Display (2)"]. the first display would change names in that case. names would be unique but not deterministic. |
It would be If we want to have physical label, the serial number is an option, though it's not that friendly, because you wouldn't use it anywhere else except mpv. Connector number would be good too, but not sure if either of this information is easy to extract in Windows. |
then you would also have the case that both display are plugged in already. would it be i also want to mention that serial numbers aren't safe either in following cases:
the latter case could be valid, for example when using the display's PiP/PaP features. |
Previously the --screen-name and --fs-screen-name options did nothing on Windows.
It now attempts to convert a friendly monitor name to the GDI device name using the DisplayConfig API first. The friendly name is the shown in Windows settings like "Dell AW3423DW". Otherwise, assume it's already a GDI device name like "\.\DISPLAY1".
Edit: Added commit
w32_common: add display-names/friendly property
The
friendly
subkey return the same set of displays asdisplay-names
but it attempts to convert the names to the human-friendly name that
DisplayConfig obtains from the EDID.