[PATCH] winex11: The correct way to search the target window for drag&drop operations.

[Zebediah Figura]

On 4/19/19 3:28 AM, John Found wrote:
> On Thu, 18 Apr 2019 12:53:35 -0500
> Zebediah Figura <z.figura12 at gmail.com> wrote:
> 
>> On 04/18/2019 12:35 PM, John Found wrote:
>>> On Thu, 18 Apr 2019 12:09:02 -0500
>>> Zebediah Figura <z.figura12 at gmail.com> wrote:
>>>
>>>>
>>>> Really? I'm looking at this test right here:
>>>>
>>>> <https://source.winehq.org/git/wine.git/blob/refs/heads/master:/dlls/user32/tests/win.c#l9398>
>>>>
>>>
>>> Well, yes, I am wrong about the WindowFromPoint, but still only the first level of the children is checked.
>>>
>>> The mentioned test checks exactly this in 3 cases:
>>>
>>> 1. Main window without child created - main window returned.
>>> 2. Main window with single STATIC child created - main window returned. (correct, according to MSDN)
>>> 3. Main window with single BUTTON child created - the child returned.
>>>
>>> There is no child-in-child cases (and IMHO, should not be).
>>>
>>
>> Really? Why not? Our implementation is recursive, so it should return
>> the deepest child. If it's wrong, we should add tests to prove it.
>>
>>
> 
> Well, I finished my extended tests and they are contradictory.
> 
> 1. Yes, WindowFromPoint searches recursively on random depth, both in Windows and WINE.
> I was totally wrong about this. Excuse my ignorance.
> 
> 2. But WindowFromPoint still does not work properly in my drag&drop patch. The problem is that WindowFromPoint
> fully ignores the STATIC controls (documented and correct behavior), but in Windows the STATIC controls are
> valid drop target if they have WS_EX_ACCEPTFILES set.
> 
> This way, the implemented in the patch "window_from_point_dnd" function correctly returns the STATIC windows drop targets.
> Comparing with the original Windows behavior, I can't see any differences.
> 
> (I will attach the improved test application I used, now it has STATIC as a drop targets and WindowFromPoint test tool as well.)
> 
> So, the question is what to do now?
> 
> Regards.
> 
> 
> 

Thanks for testing, that makes sense. In that case it seems your 
original approach is necessary. I would recommend adding a comment to 
clarify this, so that someone doesn't make the mistake of trying to 
replace it with WindowFromPoint() in the future.

I also have some stylistic comments on your original patch:

> +/* the recursive worker for window_from_point_dnd */
> +HWND do_window_from_point_dnd(HWND hwnd, POINT* point)
> +{
> +    HWND w;
> +    w = ChildWindowFromPointEx(hwnd, *point, CWP_SKIPDISABLED | CWP_SKIPINVISIBLE);
> +    if (w && (w != hwnd))
> +    {
> +        MapWindowPoints(hwnd, w, point, 1);
> +        w = do_window_from_point_dnd(w, point);
> +    }
> +    return w;
> +}
> +
> +
> +/* Recursively search for the window on given coordinates in a drag&drop specific manner. */
> +HWND window_from_point_dnd(HWND hwnd, POINT point)
> +{
> +    POINT p;
> +    p.x = point.x;
> +    p.y = point.y;
> +    ScreenToClient(hwnd, &p);
> +    return do_window_from_point_dnd(hwnd, &p);
> +}
> +

These functions should be static, since they're not used elsewhere.

I also feel like this would more readable formatted as a loop instead of 
a recursive function. Something like:

/* Find the deepest child window at the given point. We can't use 
WindowFromPoint() for this, because it skips HTTRANSPARENT windows, and 
those should still receive drop notifications. */
static HWND window_from_point( HWND hwnd, POINT point )
{
     HWND child;

     ScreenToClient( hwnd, &point );

     while ((child = ChildWindowFromPointEx( hwnd, point, 
CWP_SKIPDISABLED | CWP_SKIPINVISIBLE )) && child != hwnd)
     {
         MapWindowPoints( hwnd, child, &point, 1 );
         hwnd = child;
     }
     return hwnd;
}

> +        while (targetWindow && !(GetWindowLongW(targetWindow, GWL_EXSTYLE) & WS_EX_ACCEPTFILES))
> +            targetWindow = GetParent(targetWindow);

...

> +        while (hwnd_drop && !(GetWindowLongW(hwnd_drop, GWL_EXSTYLE) & WS_EX_ACCEPTFILES))
> +            hwnd_drop = GetParent(hwnd_drop);

Since you do this in both places, you could also factor it into the 
window_from_point helper, in which case it might be better to call it 
something like get_drop_target().

> +        POINT pt;
> +        HWND hwnd_drop;
> +
> +        pt.x = XDNDxy.x;
> +        pt.y = XDNDxy.y;
> +
> +        hwnd_drop = window_from_point_dnd(hWnd, pt);

You're passing pt by value, so you don't need to copy it into a local 
variable.