Fix #111295: Add Missing Win32 Platform-Specific Window functions #111359

Harley Acheson · 2023-08-21T20:24:51+02:00

Harley Acheson commented

2023-08-21 20:24:51 +02:00

Add platform-specific (Windows) versions of getOSWindow() and
getWindowUnderCursor().

This does not add any new ghost functions, just Windows-specific implementations of existing ones.

The base default behavior of getOSWindow always returns nullptr. This PR makes it return the Windows OS Handle as expected. This is needed for getWindowAssociatedWithOSWindow to work correctly because it uses this as a test while iterating through ghost windows. So it currently always returns nullptr.

The base default behavior of getWindowUnderCursor does a forward search through the ghost windows checking bounds. This does not consider z-depth. And because the search is forward it will always return the main window if a child is overlapping in front. This specific implementation gets the cursor location from the OS and then the Window handle from that using WindowFromPoint. Converting from window handle to ghost window requires getWindowAssociatedWithOSWindow, which needs a working getOSWindow.

This PR does not use the passed cursor location because that will often be wrong. The best example is when we have a blender window on one monitor but the mouse cursor is on a different monitor that uses a different scale. In this case the cursor location will not be a nice offset from our blender window. Best to just ask the OS where the cursor actually is.

For testing and how this relates to the referenced bug report. Opening the "Preferences" window and having it above the main window, using the color picker in the Themes area will select through to the main window. This is because the base getWindowUnderCursor behavior returns the first window with bounds that match the cursor position.

Add platform-specific (Windows) versions of getOSWindow() and getWindowUnderCursor(). --- This does not add any new ghost functions, just Windows-specific implementations of existing ones. The base default behavior of `getOSWindow` always returns nullptr. This PR makes it return the Windows OS Handle as expected. This is needed for `getWindowAssociatedWithOSWindow` to work correctly because it uses this as a test while iterating through ghost windows. So it currently always returns nullptr. The base default behavior of `getWindowUnderCursor` does a forward search through the ghost windows checking bounds. This does not consider z-depth. And because the search is forward it will always return the main window if a child is overlapping in front. This specific implementation gets the cursor location from the OS and then the Window handle from that using `WindowFromPoint`. Converting from window handle to ghost window requires `getWindowAssociatedWithOSWindow`, which needs a working `getOSWindow`. This PR does not use the passed cursor location because that will often be wrong. The best example is when we have a blender window on one monitor but the mouse cursor is on a different monitor that uses a different scale. In this case the cursor location will not be a nice offset from our blender window. Best to just ask the OS where the cursor actually is. For testing and how this relates to the referenced bug report. Opening the "Preferences" window and having it above the main window, using the color picker in the Themes area will select through to the main window. This is because the base getWindowUnderCursor behavior returns the first window with bounds that match the cursor position.

Harley Acheson added 1 commit 2023-08-21 20:25:01 +02:00

6bf593b3ad Fix #111295 : Add Missing Win32 Platform-Specific Window functions

Add platform-specific (Windows) versions of getOSWindow() and
getWindowUnderCursor().

---

The base default behavior of `getOSWindow` always returns nullptr. This PR makes it return the Windows OS Handle as expected. This is needed for `getWindowAssociatedWithOSWindow` to work correctly, otherwise that always returns nullptr.

The base default behavior of `getWindowUnderCursor` does a forward search through the ghost windows checking bounds. This does not consider z-depth. And because the search is forward it will always return the main window if a child is overlapping in front. This specific implementation gets the cursor location from the OS and then the Window from that.

This PR does not use the passed cursor location because that will often be wrong. The best example is when we have a blender window on one monitor but the mouse cursor is on a different monitor that uses a different scale. In this case the cursor location will not be a nice offset from our blender window. Best to just ask the OS where the cursor actually is.

For testing and how this relates to the referenced bug report. Opening the "Preferences" window and having it above the main window, using the color picker in the Themes area will select through to the main window. This is because the base getWindowUnderCursor returns the first window with bounds that match the cursor position.

Harley Acheson added this to the Platforms, Builds Tests & Devices project 2023-08-21 20:25:11 +02:00

Harley Acheson requested review from Ray molenkamp 2023-08-21 20:25:20 +02:00

Ray molenkamp requested changes 2023-08-21 21:44:58 +02:00

Ray molenkamp left a comment

I gotta say, i'm not loving the x,y coordinates being ignored, then again i'm also not loving the initial design, where a function called getWindowUnderCursor needs x,y coordinates fed to it, feels like something it should be able to figure out on its own. Or perhaps the function should have been called getWindowForScreenCoordinates in the first place if the location of the cursor has no influence on the results what so ever.

from talking with you on chat, we seem a bit stuck with this mess, without time to fix it, as some UI theming work needs this to function, to move forward i see 3 options :

this function needs a better comment in both GHOST_ISystem.hh and GHOST_C-api.h outlining that the x and y parameters are hints at best, and could be ignored on some backends.
Change the signature not to take x,y coordinates. I'm unsure why they are needed, @mano-wii may have some historic knowledge here as he implemented this in the past.
Make it actually work with the x,y coordinates, I understand per monitor DPI scaling makes this difficult/impossible

1 is the bare minimum we need to land this since it's a blocker, 2 or 3 would be my preference though

I gotta say, i'm not loving the x,y coordinates being ignored, then again i'm also not loving the initial design, where a function called `getWindowUnderCursor` needs x,y coordinates fed to it, feels like something it should be able to figure out on its own. Or perhaps the function should have been called `getWindowForScreenCoordinates` in the first place if the location of the cursor has no influence on the results what so ever. from talking with you on chat, we seem a bit stuck with this mess, without time to fix it, as some UI theming work needs this to function, to move forward i see 3 options : 1) this function needs a better comment in both `GHOST_ISystem.hh` and `GHOST_C-api.h` outlining that the x and y parameters are hints at best, and could be ignored on some backends. 2) Change the signature not to take x,y coordinates. I'm unsure why they are needed, @mano-wii may have some historic knowledge here as he implemented this in the past. 3) Make it actually work with the x,y coordinates, I understand per monitor DPI scaling makes this difficult/impossible 1 is the bare minimum we need to land this since it's a blocker, 2 or 3 would be my preference though

Germano Cavalcante commented

2023-08-21 22:34:40 +02:00

To bring a little historical context.
Prior to 9bd586a01e all OS's had the same solution for finding a window under the cursor.

But the existing solution there didn't work for MacOS when the cursor was on another monitor (bringing bugs).

It was not possible to use the same generic solution for Mac. So it was necessary to create a specific solution for this OS.

Thus, the algorithm that was previously in WM_window_find_under_cursor was then moved to GHOST_IWindow *GHOST_SystemWin32::getWindowUnderCursor;

And on Mac another algorithm using OS API overwrote the existing one.

To bring a little historical context. Prior to 9bd586a01e all OS's had the same solution for finding a window under the cursor. But the existing solution there didn't work for MacOS when the cursor was on another monitor (bringing bugs). It was not possible to use the same generic solution for Mac. So it was necessary to create a specific solution for this OS. Thus, the algorithm that was previously in `WM_window_find_under_cursor` was then moved to `GHOST_IWindow *GHOST_SystemWin32::getWindowUnderCursor`; And on Mac another algorithm using OS API overwrote the existing one.

Harley Acheson added 1 commit 2023-08-21 22:43:57 +02:00

16e664d14b Comment improvements

Harley Acheson commented

2023-08-21 22:54:40 +02:00

But the existing solution there didn't work for MacOS when the cursor was on another monitor (bringing bugs).

Not on Mac here, but to my eye I think it might also benefit from asking the OS where the mouse is, instead of using the coordinates we supply. Something like:

diff --git a/intern/ghost/intern/GHOST_SystemCocoa.mm b/intern/ghost/intern/GHOST_SystemCocoa.mm
index b2fa4de47b9..bb419cbbd62 100644
--- a/intern/ghost/intern/GHOST_SystemCocoa.mm
+++ b/intern/ghost/intern/GHOST_SystemCocoa.mm
@@ -809,7 +809,7 @@ GHOST_TSuccess GHOST_SystemCocoa::disposeContext(GHOST_IContext *context)
 
 GHOST_IWindow *GHOST_SystemCocoa::getWindowUnderCursor(int32_t x, int32_t y)
 {
-  NSPoint scr_co = NSMakePoint(x, y);
+  NSPoint scr_co = CGEventGetLocation(CGEventCreate(NULL));
 
   int windowNumberAtPoint = [NSWindow windowNumberAtPoint:scr_co belowWindowWithWindowNumber:0];
   NSWindow *nswindow = [NSApp windowWithWindowNumber:windowNumberAtPoint];

> But the existing solution there didn't work for MacOS when the cursor was on another monitor (bringing bugs). Not on Mac here, but to my eye I _think_ it might also benefit from asking the OS where the mouse is, instead of using the coordinates we supply. Something like: ```Diff diff --git a/intern/ghost/intern/GHOST_SystemCocoa.mm b/intern/ghost/intern/GHOST_SystemCocoa.mm index b2fa4de47b9..bb419cbbd62 100644 --- a/intern/ghost/intern/GHOST_SystemCocoa.mm +++ b/intern/ghost/intern/GHOST_SystemCocoa.mm @@ -809,7 +809,7 @@ GHOST_TSuccess GHOST_SystemCocoa::disposeContext(GHOST_IContext *context) GHOST_IWindow *GHOST_SystemCocoa::getWindowUnderCursor(int32_t x, int32_t y) { - NSPoint scr_co = NSMakePoint(x, y); + NSPoint scr_co = CGEventGetLocation(CGEventCreate(NULL)); int windowNumberAtPoint = [NSWindow windowNumberAtPoint:scr_co belowWindowWithWindowNumber:0]; NSWindow *nswindow = [NSApp windowWithWindowNumber:windowNumberAtPoint]; ```

Germano Cavalcante commented

2023-08-21 23:05:02 +02:00

I remember having tested it on monitors with different pixel resolution on the Mac. I don't really understand this limitation.

If that's really the case, it looks like the function wm_cursor_position_to_ghost(win, &x, &y); is incorrect or limited.

Since the cursor isn't used, calling wm_cursor_position_to_ghost seems to be redundant, (which doesn't look good on something that gets called so often).

Also since this function is working incorrectly, the other associated function wm_cursor_position_from_ghost(r_win, &x, &y); must also be incorrect.

So even if the window is the correct one, the relative coordinates returned by WM_window_find_under_cursor are incorrect.

So it would be ideal to fix wm_cursor_position_to_ghost and wm_cursor_position_from_ghost (or remove the need for them for all OS's).

I remember having tested it on monitors with different pixel resolution on the Mac. I don't really understand this limitation. If that's really the case, it looks like the function `wm_cursor_position_to_ghost(win, &x, &y);` is incorrect or limited. Since the cursor isn't used, calling `wm_cursor_position_to_ghost` seems to be redundant, (which doesn't look good on something that gets called so often). Also since this function is working incorrectly, the other associated function `wm_cursor_position_from_ghost(r_win, &x, &y);` must also be incorrect. So even if the window is the correct one, the relative coordinates returned by `WM_window_find_under_cursor` are incorrect. So it would be ideal to fix `wm_cursor_position_to_ghost` and `wm_cursor_position_from_ghost` (or remove the need for them for all OS's).

YimingWu commented

2023-08-22 04:27:08 +02:00

FYI I'm not sure this patch handles that, but #111295 is also reproducible on linux, so it's probably also broken on at least X11. (My wayland is broken can't test it)

Harley Acheson commented

2023-08-22 05:04:25 +02:00

@ChengduLittleA - FYI I'm not sure this patch handles that, but #111295 is also reproducible on linux, so it's probably also broken on at least X11. (My wayland is broken can't test it)

Assuming you mean that when Properties is on top of the main blender window the picker will select from the main window. That is probably a strong argument for having the loop reversed in GHOST_System.cc, GHOST_System::getWindowUnderCursor. That way the default behavior would test children before their parents. That would be better in almost all cases.

The loop there now is a "foreach" style (range-based) for loop, which AFAIK does not offer some easy "reverse" option. So just needs to be rewritten in regular for style.

> @ChengduLittleA - FYI I'm not sure this patch handles that, but #111295 is also reproducible on linux, so it's probably also broken on at least X11. (My wayland is broken can't test it) Assuming you mean that when Properties is on top of the main blender window the picker will select from the main window. That is probably a strong argument for having the loop reversed in GHOST_System.cc, GHOST_System::getWindowUnderCursor. That way the default behavior would test children before their parents. That would be better in almost all cases. The loop there now is a "foreach" style (range-based) for loop, which AFAIK does not offer some easy "reverse" option. So just needs to be rewritten in regular for style.

👍 2

Harley Acheson referenced this pull request

2023-08-24 21:34:31 +02:00

UI: Improved Default GetWindowUnderCursor Behavior #111489

Harley Acheson added 2 commits 2023-08-25 19:55:55 +02:00

c79e951954 Merge branch 'main' of projects.blender.org:blender/blender into Win32WinFunctions

buildbot/vexp-code-patch-coordinator Build done. Details

87fae133fa Comment Changes. Remove x,y arguments from Win32 getWindowUnderCursor

Harley Acheson commented

2023-08-25 19:59:04 +02:00

@blender-bot build

Harley Acheson added 2 commits 2023-08-25 21:50:52 +02:00