[PATCH v2 1/6] mciqtz32: Simplify video window management by creating default window.

[Akihiro Sagawa]

On Thu, 7 Jul 2022 14:19:02 -0500, Zebediah Figura wrote:
> On 7/5/22 06:34, Akihiro Sagawa wrote:
> > From: Akihiro Sagawa <sagawa.aki at gmail.com>
> >
> > Now the video window by IVideoWindow always has WS_CHILD style regardless
> > open parameters.
> >
> > Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52448
> > Signed-off-by: Akihiro Sagawa <sagawa.aki at gmail.com>
> 
> What's being simplified, exactly? As far as I can tell we're changing our behaviour to be more correct, but it's certainly not getting simpler.
You're right. It's not so simple... I'll rename it.

[...]
> > +    }
> > +
> > +    case WM_CLOSE:
> > +    {
> > +        MCIQTZ_mciPause(GetWindowLongW(hWnd, 0), MCI_WAIT, NULL);
> > +        ShowWindow(hWnd, SW_HIDE);
> > +        return 0;
> > +    }
> 
> It seems probably easy enough to test this in winmm, but even failing that, manual tests would be nice.
I'll do.

> > +
> > +    case WM_ERASEBKGND:
> > +    {
> > +        RECT	rect;
> > +        GetClientRect(hWnd, &rect);
> > +        FillRect((HDC)wParam, &rect, GetStockObject(BLACK_BRUSH));
> > +        return 1;
> > +    }
> 
> Why this instead of setting the background brush to black?
That's good idea. However, manual test shows it's unnecessary. I'll get
rid of this block in revised versoin.

[...]
> > +
> > +    if (dwFlags & MCI_DGV_OPEN_PARENT)	hParent = lpParms->hWndParent;
> > +    if (dwFlags & MCI_DGV_OPEN_WS)	dwStyle = lpParms->dwStyle;
> > +
> > +    dwStyle &= ~WS_VISIBLE;
> > +
> > +    rc.left = rc.top = 0;
> > +    hr = IBasicVideo_GetVideoSize(wma->vidbasic, &rc.right, &rc.bottom);
> > +    if (FAILED(hr)) return hr == E_NOINTERFACE; /* no video renderer ? */
> 
> When can that happen?
MCIQTZ driver can also handle an audio file, such as an MP3 file.
I'd like to return early for the case.

> > +
> > +    AdjustWindowRect(&rc, dwStyle, FALSE);
> > +    if (!(dwStyle & (WS_CHILD|WS_POPUP))) /* overlapped window ? */
> 
> Where is this logic coming from? Do you have tests?
> This also loses the previous logic that uses GetClientRect() on the parent. That might be correct, but it lacks tests, and it should probably be done in a separate patch.
This logic, MCIQTZ_CreateWindow and MCIQTZ_WindowProc mostly come from
dlls/mciavi32/wnd.c. Some of them arn't applicable to MCIQTZ. I'll add
some tests regarding window dimensions.

> > +    {
> > +        rc.right -= rc.left;
> > +        rc.bottom -= rc.top;
> 
> Aren't "left" and "top" zero?
No, AdjustWindowRect() makes them negative numbers.
For instance,
SetRect(&rc, 0, 0, 640, 480);
AdjustWindowRect(&rc, WS_OVERLAPPEDWINDOW, FALSE);
It'll show, rc.left: -3 and rc.top: -22. Of course, these values are
varies caption bar height, etc.

> It seems like it'd be less confusing in general to use separate width/height (or x/y) variables instead of passing rc.right and rc.bottom directly to AdjustWindow.
Right.

[...]
> > +    wma->parent = wma->window;
> > +    IVideoWindow_put_AutoShow(wma->vidwin, OAFALSE);
> > +    IVideoWindow_put_MessageDrain(wma->vidwin, (OAHWND)wma->parent);
> > +    IVideoWindow_put_WindowStyle(wma->vidwin, WS_CHILD);
> 
> put_Owner() automatically sets WS_CHILD.
Yes, but the main point of this call is resetting the window style.
Otherwise, video renderer window inherits the former, overlapped window
style, e.g. WS_CAPTION.

> > +    IVideoWindow_put_Owner(wma->vidwin, (OAHWND)wma->parent);
> > +    IVideoWindow_put_Visible(wma->vidwin, OATRUE);
> > +
> > +    return TRUE;
> > +}
> > +
[...]
> > @@ -307,6 +400,13 @@ static DWORD MCIQTZ_mciClose(UINT wDevID, DWORD dwFlags, LPMCI_GENERIC_PARMS lpP
> >       MCIQTZ_mciStop(wDevID, MCI_WAIT, NULL);
> >  >       if (wma->opened) {
> > +        if (wma->window)
> 
> Assuming we were to check for failure in MCIQTZ_mciOpen(), which I imgaine we should do, can this happen?
> 
> There are other similar cases below.
Again, MCIQTZ can open audio files. In that case, wma->window will be
NULL.

> > +        {
> > +            IVideoWindow_put_MessageDrain(wma->vidwin, (OAHWND)NULL);
> > +            IVideoWindow_put_Owner(wma->vidwin, (OAHWND)NULL);
> > +            DestroyWindow(wma->window);
> > +            wma->window = NULL;
> > +        }
> >           IVideoWindow_Release(wma->vidwin);
> >           IBasicVideo_Release(wma->vidbasic);
> >           IBasicAudio_Release(wma->audio);
[...]
> > @@ -525,9 +626,6 @@ static DWORD MCIQTZ_mciStop(UINT wDevID, DWORD dwFlags, LPMCI_GENERIC_PARMS lpPa
> >           wma->thread = NULL;
> >       }
> >  > -    if (!wma->parent)
> > -        IVideoWindow_put_Visible(wma->vidwin, OAFALSE);
> > -
> 
> The tests bear this out, but it's also a change in behaviour that seems outside the scope of this patch.
Indeed. I'll add test and patch for this point before this patch.

> >       return 0;
> >   }
> >  > @@ -574,6 +672,9 @@ static DWORD MCIQTZ_mciResume(UINT wDevID, DWORD dwFlags, LPMCI_GENERIC_PARMS lp
> >           return MCIERR_INTERNAL;
> >       }
> >  > +    if (wma->parent)
> > +        ShowWindow(wma->parent, SW_SHOW);
> > +
> 
> This doesn't have tests, and it also seems outside the scope of this patch.
Sure. I'll split this change into another patch.

> >       return 0;
> >   }
> >  > @@ -919,21 +1020,10 @@ static DWORD MCIQTZ_mciWindow(UINT wDevID, DWORD dwFlags, LPMCI_DGV_WINDOW_PARMS
> >           return 0;
> >  >       if (dwFlags & MCI_DGV_WINDOW_HWND && (IsWindow(lpParms->hWnd) || !lpParms->hWnd)) {
> > -        LONG visible = OATRUE;
> > -        LONG style = 0;
> >           TRACE("Setting hWnd to %p\n", lpParms->hWnd);
> > -        IVideoWindow_get_Visible(wma->vidwin, &visible);
> > -        IVideoWindow_put_Visible(wma->vidwin, OAFALSE);
> > -        IVideoWindow_get_WindowStyle(wma->vidwin, &style);
> > -        style &= ~WS_CHILD;
> > -        if (lpParms->hWnd)
> > -            IVideoWindow_put_WindowStyle(wma->vidwin, style|WS_CHILD);
> > -        else
> > -            IVideoWindow_put_WindowStyle(wma->vidwin, style);
> > -        IVideoWindow_put_Owner(wma->vidwin, (OAHWND)lpParms->hWnd);
> > -        IVideoWindow_put_MessageDrain(wma->vidwin, (OAHWND)lpParms->hWnd);
> > -        IVideoWindow_put_Visible(wma->vidwin, visible);
> > -        wma->parent = lpParms->hWnd;
> > +        wma->parent = lpParms->hWnd ? lpParms->hWnd : wma->window;
> > +        IVideoWindow_put_MessageDrain(wma->vidwin, (OAHWND)wma->parent);
> > +        IVideoWindow_put_Owner(wma->vidwin, (OAHWND)wma->parent);
> >       }
> 
> Should we also hide "wma->window" here?
Good catch. We need tests at this point.

Thanks for reviewing and feedbacks.
I'll send updated patch soon.
Sorry for the delay in replying due to trial and error.

Akihiro Sagawa