Re: [PATCH] user32: Allow WM_CAPTURECHANGED indicating SetCapture() parameter

[Ken Thomases]

On Jan 31, 2016, at 7:18 PM, Christopher Thielen <cthielen(a)gmail.com> wrote:
> A window may be notified with WM_CAPTURECHANGED about itself
> gaining mouse capture if it calls SetCapture() twice.
>
> Tested on Windows XP and Fedora Workstation 22.
>
> Revised to simplify based on wine-devel feedback.
> Revised to address any side-effects of new WM_CAPTURECHANGED
> messages.

The changes to comctl32 should be in a separate patch.  In general, changes to different DLLs should be in separate patches unless it can't be avoided.  The comctl32 patch should come before the change to when WM_CAPTURECHANGED is sent so that things aren't broken part-way through the patch series.  You might also put the button.c changes in a separate patch, too.
> case WM_CAPTURECHANGED:
> -	    return TOOLBAR_CaptureChanged(infoPtr);
> +	    if(hwnd != lParam)
> +	    {
> +	        return TOOLBAR_CaptureChanged(infoPtr);
> +	    }
> +	    else
> +	    {
> +	        return 0;
> +	    }

The second return can be hoisted out of the else since the true branch can't fall through.  Also, you can reverse the logic, so the code can look like:

    case WM_CAPTURECHANGED:
        if(hwnd == lParam)
            return 0;
        return TOOLBAR_CaptureChanged(infoPtr);

I think that's clearer and also a smaller diff.  Same for the other files.

Cheers,
Ken

[PATCH] user32: Allow WM_CAPTURECHANGED indicating SetCapture() parameter

[Chris Thielen]

Thanks Ken, I'll make those changes.

On Sunday, January 31, 2016, Ken Thomases <ken(a)codeweavers.com> wrote:
> On Jan 31, 2016, at 7:18 PM, Christopher Thielen <cthielen(a)gmail.com
> <javascript:;>> wrote:
> > A window may be notified with WM_CAPTURECHANGED about itself
> > gaining mouse capture if it calls SetCapture() twice.
> >
> > Tested on Windows XP and Fedora Workstation 22.
> >
> > Revised to simplify based on wine-devel feedback.
> > Revised to address any side-effects of new WM_CAPTURECHANGED
> > messages.
>
> The changes to comctl32 should be in a separate patch.  In general,
> changes to different DLLs should be in separate patches unless it can't be
> avoided.  The comctl32 patch should come before the change to when
> WM_CAPTURECHANGED is sent so that things aren't broken part-way through the
> patch series.  You might also put the button.c changes in a separate patch,
> too.
> > case WM_CAPTURECHANGED:
> > -         return TOOLBAR_CaptureChanged(infoPtr);
> > +         if(hwnd != lParam)
> > +         {
> > +             return TOOLBAR_CaptureChanged(infoPtr);
> > +         }
> > +         else
> > +         {
> > +             return 0;
> > +         }
>
> The second return can be hoisted out of the else since the true branch
> can't fall through.  Also, you can reverse the logic, so the code can look
> like:
>
> case WM_CAPTURECHANGED:
>         if(hwnd == lParam)
>             return 0;
>         return TOOLBAR_CaptureChanged(infoPtr);
>
> I think that's clearer and also a smaller diff.  Same for the other files.
>
> Cheers,
> Ken