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

[John Found]

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)?

-- 
John Found <johnfound at asm32.info>