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

[Zebediah Figura]

On 4/19/19 3:58 PM, John Found wrote:
> On Fri, 19 Apr 2019 15:43:46 -0500
> Zebediah Figura <z.figura12 at gmail.com> wrote:
> 
>> 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.
>>
>>
> 
> Thanks for the hints. My C skills are really not the best of my skills.
> Will fix it and submit as a v2.
> 
> BTW, what you think about the notice of Gabriel Ivăncescu in his post above, about the difference between GetParent (returns the owner as well) and GetAncestor(...,GA_PARENT) (returns only the parent)?
> 

Yes, that seems correct to me; I wouldn't imagine the owner would be 
take into consideration. Though, of course, it always helps to test :-)