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@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@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);elseIVideoWindow_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