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

[John Found]

-- 
John Found <johnfound(a)asm32.info>

[Zebediah Figura]

Hello John, thanks for the patch! Note that all patches require a 
Signed-off-by header, as a way of taking responsibility for the patch 
and acknowledging that you believe it's acceptable for Wine.

On 4/17/19 4:20 PM, John Found wrote:
> +/* 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);
> +}
> +

Maybe I'm missing something here, but don't these two functions 
effectively do the equivalent of WindowFromPoint(point)? Is there any 
reason we can't just use that instead?

[John Found]

On Wed, 17 Apr 2019 18:30:11 -0500
Zebediah Figura <z.figura12(a)gmail.com> wrote:
> Hello John, thanks for the patch! Note that all patches require a 
> Signed-off-by header, as a way of taking responsibility for the patch 
> and acknowledging that you believe it's acceptable for Wine.

Hello Zebediah. I though it is needed only if I am posting a patch written by someone else.
But yes, I beleave, that this patch is acceptable and Wine. That is why I am sending it here.

Should I resend the patch again now, with this header?
> On 4/17/19 4:20 PM, John Found wrote:
> > +/* 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);
> > +}
> > +
>
> Maybe I'm missing something here, but don't these two functions 
> effectively do the equivalent of WindowFromPoint(point)? Is there any 
> reason we can't just use that instead?

WindowFromPoint, ChildWindowFromPointEx and other similar functions return only the top level windows 
or the immediate children of them. While the drop target can be somewhere deeper in the window tree.
This is exactly why the behavior of drag&drop operations in Wine for Linux is different from the original Windows.

The function "do_window_from_point_dnd" searches recursively down the windows tree and returns the most deeper child that
contains the point. Then it searches back up, until a window with WS_EX_ACCEPTFILES is found. (or not, of course).


-- 
John Found <johnfound(a)asm32.info>

[Zebediah Figura]

On 4/18/19 12:30 AM, John Found wrote:
> On Wed, 17 Apr 2019 18:30:11 -0500
> Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > Hello John, thanks for the patch! Note that all patches require a
> > Signed-off-by header, as a way of taking responsibility for the patch
> > and acknowledging that you believe it's acceptable for Wine.
>
> Hello Zebediah. I though it is needed only if I am posting a patch written by someone else.
> But yes, I beleave, that this patch is acceptable and Wine. That is why I am sending it here.
>
> Should I resend the patch again now, with this header?

Yes, please do so.
> > On 4/17/19 4:20 PM, John Found wrote:
> > > +/* 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);
> > > +}
> > > +
> >
> > Maybe I'm missing something here, but don't these two functions
> > effectively do the equivalent of WindowFromPoint(point)? Is there any
> > reason we can't just use that instead?
>
> WindowFromPoint, ChildWindowFromPointEx and other similar functions return only the top level windows
> or the immediate children of them. While the drop target can be somewhere deeper in the window tree.
> This is exactly why the behavior of drag&drop operations in Wine for Linux is different from the original Windows.

I don't think that's correct; the documentation states that 
WindowFromPoint() returns the deepest child, and we have tests to 
support this.
> The function "do_window_from_point_dnd" searches recursively down the windows tree and returns the most deeper child that
> contains the point. Then it searches back up, until a window with WS_EX_ACCEPTFILES is found. (or not, of course).

[John Found]

On Thu, 18 Apr 2019 00:34:16 -0500
Zebediah Figura <z.figura12(a)gmail.com> wrote:
> On 4/18/19 12:30 AM, John Found wrote:
> > On Wed, 17 Apr 2019 18:30:11 -0500
> > Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > > Hello John, thanks for the patch! Note that all patches require a
> > > Signed-off-by header, as a way of taking responsibility for the patch
> > > and acknowledging that you believe it's acceptable for Wine.
> >
> > Hello Zebediah. I though it is needed only if I am posting a patch written by someone else.
> > But yes, I beleave, that this patch is acceptable and Wine. That is why I am sending it here.
> >
> > Should I resend the patch again now, with this header?
>
> Yes, please do so.

Do I need to label the patch as v2 or it is enough to email it the same way, 
only with "Signed-off-by" header added?
> > > On 4/17/19 4:20 PM, John Found wrote:
> > > > +/* 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);
> > > > +}
> > > > +
> > >
> > > Maybe I'm missing something here, but don't these two functions
> > > effectively do the equivalent of WindowFromPoint(point)? Is there any
> > > reason we can't just use that instead?
> >
> > WindowFromPoint, ChildWindowFromPointEx and other similar functions return only the top level windows
> > or the immediate children of them. While the drop target can be somewhere deeper in the window tree.
> > This is exactly why the behavior of drag&drop operations in Wine for Linux is different from the original Windows.

> I don't think that's correct; the documentation states that 
> WindowFromPoint() returns the deepest child, and we have tests to 
> support this.

What documentation? MS documentation is pretty vague: 

https://docs.microsoft.com/en-us/windows/desktop/api/winuser/nf-winuser-wind... 

It states only that: "Retrieves a handle to the window that contains the specified point."

In addition, the current implementation of drop target detection code uses exactly WindowFromPoint 
(see the patch - I have replaced this line) and it definitely does not detects the children windows at all.

I will attach a small test application that creates a parent window and 3 nested children, all of them has WS_EX_ACCEPTFILES set.

When you drop a file at some of the windows (the parent or some of the children) the static control displays what is
the drop operation target. 

So, in Linux, only the "Parent" is returned, regardless of the real drop target. In Windows and in Linux with the 
applied patch, the drop target is always the right child window.
> > The function "do_window_from_point_dnd" searches recursively down the windows tree and returns the most deeper child that
> > contains the point. Then it searches back up, until a window with WS_EX_ACCEPTFILES is found. (or not, of course).

-- 
John Found <johnfound(a)asm32.info>

[Zebediah Figura]

On 4/18/19 6:37 AM, John Found wrote:
> > I don't think that's correct; the documentation states that
> > WindowFromPoint() returns the deepest child, and we have tests to
> > support this.
>
> What documentation? MS documentation is pretty vague:
>
> https://docs.microsoft.com/en-us/windows/desktop/api/winuser/nf-winuser-wind...
>
> It states only that: "Retrieves a handle to the window that contains the specified point." >
> In addition, the current implementation of drop target detection code uses exactly WindowFromPoint
> (see the patch - I have replaced this line) and it definitely does not detects the children windows at all.
>
> I will attach a small test application that creates a parent window and 3 nested children, all of them has WS_EX_ACCEPTFILES set.
>
> When you drop a file at some of the windows (the parent or some of the children) the static control displays what is
> the drop operation target.
>
> So, in Linux, only the "Parent" is returned, regardless of the real drop target. In Windows and in Linux with the
> applied patch, the drop target is always the right child window.

Er, yes, that was my mistake, sorry. MSDN is not clear at all. However, 
the point remains, we have tests for this behaviour, and as far as I'm 
reading the server code it should be implemented correctly. If it's not 
working, then I would guess there's a bug elsewhere (or possibly there's 
some quirk here that the tests don't cover, but then we should find and 
identify that first.)

[John Found]

On Thu, 18 Apr 2019 09:47:17 -0500
Zebediah Figura <z.figura12(a)gmail.com> wrote:
> On 4/18/19 6:37 AM, John Found wrote:
> > > I don't think that's correct; the documentation states that
> > > WindowFromPoint() returns the deepest child, and we have tests to
> > > support this.
> >
> > What documentation? MS documentation is pretty vague:
> >
> > https://docs.microsoft.com/en-us/windows/desktop/api/winuser/nf-winuser-wind...
> >
> > It states only that: "Retrieves a handle to the window that contains the specified point." >
> > In addition, the current implementation of drop target detection code uses exactly WindowFromPoint
> > (see the patch - I have replaced this line) and it definitely does not detects the children windows at all.
> >
> > I will attach a small test application that creates a parent window and 3 nested children, all of them has WS_EX_ACCEPTFILES set.
> >
> > When you drop a file at some of the windows (the parent or some of the children) the static control displays what is
> > the drop operation target.
> >
> > So, in Linux, only the "Parent" is returned, regardless of the real drop target. In Windows and in Linux with the
> > applied patch, the drop target is always the right child window.
>
> Er, yes, that was my mistake, sorry. MSDN is not clear at all. However, 
> the point remains, we have tests for this behaviour, and as far as I'm 
> reading the server code it should be implemented correctly. If it's not 
> working, then I would guess there's a bug elsewhere (or possibly there's 
> some quirk here that the tests don't cover, but then we should find and 
> identify that first.)

Well, I don't know. I always expected that WindowFromPoint will return the top level window. And GetChildWindowFromPointEx will return only the children from the first level. Both in Windows and in WINE.  

BTW, MSDN for ChildWindowFromPointEx says: "Determines which, if any, of the child windows belonging to the specified parent window contains the specified point. The function can ignore invisible, disabled, and transparent child windows. The search is restricted to immediate child windows. **Grandchildren and deeper descendants are not searched.**"

So, IMHO you are wrong and the implementation in WINE is correct. And the tests that pass are probably not for the
children from the upper levels.

-- 
John Found <johnfound(a)asm32.info>

[Zebediah Figura]

On 04/18/2019 11:39 AM, John Found wrote:
> On Thu, 18 Apr 2019 09:47:17 -0500
> Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > On 4/18/19 6:37 AM, John Found wrote:
> > > > I don't think that's correct; the documentation states that
> > > > WindowFromPoint() returns the deepest child, and we have tests to
> > > > support this.
> > >
> > > What documentation? MS documentation is pretty vague:
> > >
> > > https://docs.microsoft.com/en-us/windows/desktop/api/winuser/nf-winuser-wind...
> > >
> > > It states only that: "Retrieves a handle to the window that contains the specified point." >
> > > In addition, the current implementation of drop target detection code uses exactly WindowFromPoint
> > > (see the patch - I have replaced this line) and it definitely does not detects the children windows at all.
> > >
> > > I will attach a small test application that creates a parent window and 3 nested children, all of them has WS_EX_ACCEPTFILES set.
> > >
> > > When you drop a file at some of the windows (the parent or some of the children) the static control displays what is
> > > the drop operation target.
> > >
> > > So, in Linux, only the "Parent" is returned, regardless of the real drop target. In Windows and in Linux with the
> > > applied patch, the drop target is always the right child window.
> >
> > Er, yes, that was my mistake, sorry. MSDN is not clear at all. However, 
> > the point remains, we have tests for this behaviour, and as far as I'm 
> > reading the server code it should be implemented correctly. If it's not 
> > working, then I would guess there's a bug elsewhere (or possibly there's 
> > some quirk here that the tests don't cover, but then we should find and 
> > identify that first.)
>
> Well, I don't know. I always expected that WindowFromPoint will return the top level window. And GetChildWindowFromPointEx will return only the children from the first level. Both in Windows and in WINE.
>
> BTW, MSDN for ChildWindowFromPointEx says: "Determines which, if any, of the child windows belonging to the specified parent window contains the specified point. The function can ignore invisible, disabled, and transparent child windows. The search is restricted to immediate child windows. **Grandchildren and deeper descendants are not searched.**"
>
> So, IMHO you are wrong and the implementation in WINE is correct. And the tests that pass are probably not for the
> children from the upper levels.

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>

And the implementation here:

<https://source.winehq.org/git/wine.git/blob/refs/heads/master:/dlls/user32/winpos.c#l289>

and

<https://source.winehq.org/git/wine.git/blob/refs/heads/master:/server/window.c#l2223>

That seems to pretty clearly show that WindowFromPoint() will return a
child window at the given point. Both in Windows and in Wine. Not a
matter of opinion, really.

Maybe the tests or the implementation are incomplete, but then we should
fix that, since it seems otherwise clear that WindowFromPoint() should
be returning the deepest child.

[John Found]

On Thu, 18 Apr 2019 12:09:02 -0500
Zebediah Figura <z.figura12(a)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).
> And the implementation here:
>
> <https://source.winehq.org/git/wine.git/blob/refs/heads/master:/dlls/user32/winpos.c#l289>
>
> and
>
> <https://source.winehq.org/git/wine.git/blob/refs/heads/master:/server/window.c#l2223>
>
> That seems to pretty clearly show that WindowFromPoint() will return a
> child window at the given point. Both in Windows and in Wine. Not a
> matter of opinion, really.
>
> Maybe the tests or the implementation are incomplete, but then we should
> fix that, since it seems otherwise clear that WindowFromPoint() should
> be returning the deepest child.

IMO, the tests and implementations are OK. Windows works this way. 

-- 
John Found <johnfound(a)asm32.info>

[Zebediah Figura]

On 04/18/2019 12:35 PM, John Found wrote:
> On Thu, 18 Apr 2019 12:09:02 -0500
> Zebediah Figura <z.figura12(a)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.

[John Found]

On Thu, 18 Apr 2019 12:53:35 -0500
Zebediah Figura <z.figura12(a)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(a)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, if I understand correctly, WindowFromPoint in WINE should work the same way as in Windows. I don't have Windows machine right now, but will test tomorrow how exactly WindowFromPoint works. This test will show whether the discussed
behavior is bug or not...

Right now, I can only say, that drop targets in WINE are searched different than in Windows (the test from my previous email) and this is what my patch fixes...

-- 
John Found <johnfound(a)asm32.info>

[Gabriel Ivăncescu]

On 4/18/19 9:19 PM, John Found wrote:
> On Thu, 18 Apr 2019 12:53:35 -0500
> Zebediah Figura <z.figura12(a)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(a)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, if I understand correctly, WindowFromPoint in WINE should work the same way as in Windows. I don't have Windows machine right now, but will test tomorrow how exactly WindowFromPoint works. This test will show whether the discussed
> behavior is bug or not...
>
> Right now, I can only say, that drop targets in WINE are searched different than in Windows (the test from my previous email) and this is what my patch fixes...

This might be an interesting read: 
https://devblogs.microsoft.com/oldnewthing/20101230-00/?p=11873

Do you test with disabled child windows? Because that's one case where 
WindowFromPoint fails, it won't check deeper child windows even if they 
are active in this case. Not sure if Windows treats it differently here.

BTW I'm not sure about this but I think GetAncestor(hwnd, GA_PARENT) is 
more appropriate since GetParent can also retrieve the owner window, not 
sure if that's a factor here, but seems safer to me.

[John Found]

> This might be an interesting read: 
> https://devblogs.microsoft.com/oldnewthing/20101230-00/?p=11873
>
> Do you test with disabled child windows? Because that's one case where 
> WindowFromPoint fails, it won't check deeper child windows even if they 
> are active in this case. Not sure if Windows treats it differently here.
>
> BTW I'm not sure about this but I think GetAncestor(hwnd, GA_PARENT) is 
> more appropriate since GetParent can also retrieve the owner window, not 
> sure if that's a factor here, but seems safer to me.

You probably missed my newer message in this thread. WindowFromPoint fails with
the static controls as well. So, the drag&drop operations needs special implementation,
as I have hade it in the discussed patch.

Also, thanks for the hint about GetParent. I will think about it, but
at first glance, we really don't need owner windows to be messed in drop target search.
So, GetAncestor looks better. Will test it.

-- 
John Found <johnfound(a)asm32.info>

[John Found]

On Thu, 18 Apr 2019 12:53:35 -0500
Zebediah Figura <z.figura12(a)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(a)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.
-- 
John Found <johnfound(a)asm32.info>

[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(a)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(a)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.

[John Found]

On Fri, 19 Apr 2019 15:43:46 -0500
Zebediah Figura <z.figura12(a)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(a)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(a)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(a)asm32.info>

[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(a)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(a)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(a)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 :-)