Re: Help with a patch needed

[Oliver Stieber]

--- Stefan Dösinger  wrote:
> Hi,
> I've sent the following patch to wine-patches approximatly 2 weeks ago, and it 
> wasn't applied nor any suggestions for improvement where given. As suggested 
> by some people in another thread, I send it to wine-devel to ask for help.
>
> This patch adds handling for the DDSCL_SETFOCUSWINDOW flag in 
> DirectDraw7::SetCooperativeLevel. It also adds some tests which test the 
> reactions of Windows to these flag.
>
> I've stumbled across the DDSCL_SETFOCUSWINDOW flag while debugging Empire 
> Earth. This game cancels with msvcrt_abort if SetCooperativeLevel returns an 
> error.
>
> Any comments / Suggestions from the DirectX people?

Seems ok to me, the logging could do with a but of tarting up.
It's usually really handy if the instance is logged with each FIXME / TRACE 
e.g. 
instead of 
FIXME("Poorly handled flag DDSCL_SETFOCUSWINDOW\n");
use
FIXME("(%p) : Poorly handled flag DDSCL_SETFOCUSWINDOW\n", This);

It's also usefull to log whenever a function returns an error.
e.g.
    if (!(cooplevel & (DDSCL_EXCLUSIVE|DDSCL_NORMAL|DDSCL_SETFOCUSWINDOW))) {
        TRACE("(%p) : Call to SetCooperativeLevel failed: cooplevel  !=
DDSCL_EXCLUSIVE|DDSCL_NORMAL|DDSCL_SETFOCUSWINDOW, returning DDERR_INVALIDPARAMS);
        return DDERR_INVALIDPARAMS;
    }

that way if returning an error is causing something else to fail it's fairly easy to pick up.

Oliver.
> Thanks,
> Stefan Dösinger
> > ? dlls/ddraw/tests/cooplevels.c
> Index: dlls/ddraw/ddraw_main.c
>
> RCS file: /home/wine/wine/dlls/ddraw/ddraw_main.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 ddraw_main.c
> --- dlls/ddraw/ddraw_main.c	26 Jul 2005 20:10:51 -0000	1.8
> +++ dlls/ddraw/ddraw_main.c	25 Aug 2005 13:53:20 -0000
> @@ -1116,13 +1116,54 @@ Main_DirectDraw_SetCooperativeLevel(LPDI
>       * created." Otherwise the window can be changed???
>       *
>       * This appears to be wrong - comment it out for now.
> -
> * This seems to be true at least for DDSCL_SETFOCUSWINDOW
> -
> * It looks like Windows doesn't store the HWND in all cases,
> -
> * probably if DDSCL_NORMAL is specified, but that's not sure
> if (This->window)
>  return DDERR_HWNDALREADYSET;
>    */
> - if (!(cooplevel & (DDSCL_EXCLUSIVE|DDSCL_NORMAL)))
> - return DDERR_INVALIDPARAMS;
> -
>
> /* DDSCL_EXCLUSIVE or DDSCL_NORMAL or DDSCL_SETFOCUSWINDOW must be given */
> -
>
> if (!(cooplevel & (DDSCL_EXCLUSIVE|DDSCL_NORMAL|DDSCL_SETFOCUSWINDOW)))
> -
>    return DDERR_INVALIDPARAMS;
>
> -
>
> /* Device window and focus Window. They only really matter in a
> -
> * Multi-Monitor application, but some games specify them and we
> -
> * have to react correctly. */
> -
>
> if(cooplevel & DDSCL_SETFOCUSWINDOW)
> -
>
> {
> -
>    /* This flag is a biest: It is only valid when DDSCL_NORMAL has been set
> -
>     * or no hwnd is set and no other flags are allowed, except DDSCL_NOWINDOWCHANGES
> -
>     */
> -
>    if(This->window)
> -
>        if(!(This->cooperative_level & DDSCL_NORMAL)) return DDERR_HWNDALREADYSET;
> -
>    if((cooplevel != DDSCL_SETFOCUSWINDOW))
> -
>        if(cooplevel != (DDSCL_SETFOCUSWINDOW | DDSCL_NOWINDOWCHANGES) ) return
>
> DDERR_INVALIDPARAMS;
> -
> -
>    /* Don't know what exactly to do, but it's perfectly valid
> -
>     * to pass DDSCL_SETFOCUSWINDOW only */
> -
>    FIXME("Poorly handled flag DDSCL_SETFOCUSWINDOW\n");
> -
> -
>    /* Store the flag in the cooperative level. I don't think that all other
> -
>     * flags should be overwritten, so just add it 
> -
>     * (In the most cases this will be DDSCL_SETFOCUSWINDOW | DDSCL_NORMAL) */
> -
>    cooplevel |= DDSCL_SETFOCUSWINDOW;
> -
> -
>    return DD_OK;
> - }
> -
> - /* DDSCL_EXCLUSE mode requires  DDSCL_FULLSCREEN and vice versa */
> - if((cooplevel & DDSCL_EXCLUSIVE) && !(cooplevel & DDSCL_FULLSCREEN))
> -
>    return DDERR_INVALIDPARAMS;
> - /* The other case is checked above */
> -
> - /* Unhandled flags. Give a warning */
> - if(cooplevel & DDSCL_SETDEVICEWINDOW)
> -
>    FIXME("Unhandled flag DDSCL_SETDEVICEWINDOW.\n");
> - if(cooplevel & DDSCL_CREATEDEVICEWINDOW)
> -
>    FIXME("Unhandled flag DDSCL_CREATEDEVICEWINDOW.\n");
> -
> - /* Perhaps the hwnd is only set in DDSCL_EXLUSIVE and DDSCL_FULLSCREEN mode. Not sure */
> This->window = hwnd;
> This->cooperative_level = cooplevel;
> Index: dlls/ddraw/tests/Makefile.in
>
> RCS file: /home/wine/wine/dlls/ddraw/tests/Makefile.in,v
> retrieving revision 1.5
> diff -u -p -r1.5 Makefile.in
> --- dlls/ddraw/tests/Makefile.in	7 Jun 2005 21:34:59 -0000	1.5
> +++ dlls/ddraw/tests/Makefile.in	25 Aug 2005 13:53:20 -0000
> @@ -9,7 +9,8 @@ EXTRALIBS = -ldxguid
>  CTESTS = \
>    d3d.c \
>    ddrawmodes.c \
> - dsurface.c
> - dsurface.c \
> - cooplevels.c
>
> @MAKE_TEST_RULES@
>
> --- /dev/null	2005-08-25 09:49:19.469757400 +0000
> +++ dlls/ddraw/tests/cooplevels.c	2005-08-25 15:26:27.000000000 +0000
> @@ -0,0 +1,187 @@
> +/*
> -
> - Unit tests for ddraw cooperative levels
> -
> -
> -
> - Copyright (C) 2005 Stefan Dösinger
> -
> -
> -
> - This library is free software; you can redistribute it and/or
> -
> - modify it under the terms of the GNU Lesser General Public
> -
> - License as published by the Free Software Foundation; either
> -
> - version 2.1 of the License, or (at your option) any later version.
> -
> -
> -
> - This library is distributed in the hope that it will be useful,
> -
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> -
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -
> - Lesser General Public License for more details.
> -
> -
> -
> - You should have received a copy of the GNU Lesser General Public
> -
> - License along with this library; if not, write to the Free Software
> -
> - Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> - */
> -
>
> +#include <assert.h>
> +#include "wine/test.h"
> +#include "ddraw.h"
> -
>
> +static LPDIRECTDRAW lpDD = NULL;
> +static WNDCLASS wc;
> +static HWND hwnd;
> -
>
> +static void createwindow(void)
> +{
> - wc.style = CS_HREDRAW | CS_VREDRAW;
> - wc.lpfnWndProc = DefWindowProcA;
> - wc.cbClsExtra = 0;
> - wc.cbWndExtra = 0;
> - wc.hInstance = GetModuleHandleA(0);
> - wc.hIcon = LoadIconA(wc.hInstance, IDI_APPLICATION);
> - wc.hCursor = LoadCursorA(NULL, IDC_ARROW);
> - wc.hbrBackground = (HBRUSH) GetStockObject(BLACK_BRUSH);
> - wc.lpszMenuName = NULL;
> - wc.lpszClassName = "TestWindowClass";
> - if(!RegisterClassA(&wc))
> -
>    assert(0);
> -
> - hwnd = CreateWindowExA(0, "TestWindowClass", "TestWindowClass",
> -
>    WS_POPUP, 0, 0,
> -
>    GetSystemMetrics(SM_CXSCREEN),
> -
>    GetSystemMetrics(SM_CYSCREEN),
> -
>    NULL, NULL, GetModuleHandleA(0), NULL);
> - assert(hwnd != NULL);
> -
> - ShowWindow(hwnd, SW_HIDE);
> - UpdateWindow(hwnd);
> - SetFocus(hwnd);
> -
>
> +}
> -
>
> +static void createdirectdraw(void)
> +{
> - HRESULT rc;
> -
> - rc = DirectDrawCreate(NULL, &lpDD, NULL);
> - ok(rc==DD_OK,"DirectDrawCreate returned: %lx\n",rc);
>
> +}
> -
> -
>
> +static void releasedirectdraw(void)
> +{
> - if( lpDD != NULL )
> - {
> -
> IDirectDraw_Release(lpDD);
> -
> lpDD = NULL;
> - }
>
> +}
> -
>
> +static void testcooperativelevels_normal(void)
> +{
> - HRESULT rc;
> -
> - /* Do some tests with DDSCL_NORMAL mode */
> -
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_NORMAL);
> - ok(rc==DD_OK,"SetCooperativeLevel(DDSCL_NORMAL) returned: %lx\n",rc);
> -
> - /* Set the focus window */
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_SETFOCUSWINDOW);
> - ok(rc==DD_OK,"SetCooperativeLevel(DDSCL_SETFOCUSWINDOW) returned: %lx\n",rc);
> -
> - /* Set the focus window a secound time*/
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_SETFOCUSWINDOW);
> - ok(rc==DD_OK,"SetCooperativeLevel(DDSCL_SETFOCUSWINDOW) the secound time returned:
>
> %lx\n",rc);
> -
> - /* Test DDSCL_SETFOCUSWINDOW with the other flags. They should all fail, except of
>
> DDSCL_NOWINDOWCHANGES */
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_NORMAL | DDSCL_SETFOCUSWINDOW);
> - ok(rc==DDERR_INVALIDPARAMS,"SetCooperativeLevel(DDSCL_NORMAL | DDSCL_SETFOCUSWINDOW)
>
> returned: %lx\n",rc);
> -
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_EXCLUSIVE | DDSCL_FULLSCREEN | DDSCL_SETFOCUSWINDOW);
> - ok(rc==DDERR_INVALIDPARAMS,"SetCooperativeLevel(DDSCL_EXCLUSIVE | DDSCL_FULLSCREEN |
>
> DDSCL_SETFOCUSWINDOW) returned: %lx\n",rc);
> -
> - /* This one succeeds */
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_NOWINDOWCHANGES | DDSCL_SETFOCUSWINDOW);
> - ok(rc==DD_OK,"SetCooperativeLevel(DDSCL_NOWINDOWCHANGES | DDSCL_SETFOCUSWINDOW) returned:
>
> %lx\n",rc);
> -
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_MULTITHREADED | DDSCL_SETFOCUSWINDOW);
> - ok(rc==DDERR_INVALIDPARAMS,"SetCooperativeLevel(DDSCL_MULTITHREADED | DDSCL_SETFOCUSWINDOW)
>
> returned: %lx\n",rc);
> -
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_FPUSETUP | DDSCL_SETFOCUSWINDOW);
> - ok(rc==DDERR_INVALIDPARAMS,"SetCooperativeLevel(DDSCL_FPUSETUP | DDSCL_SETFOCUSWINDOW)
>
> returned: %lx\n",rc);
> -
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_FPUPRESERVE | DDSCL_SETFOCUSWINDOW);
> - ok(rc==DDERR_INVALIDPARAMS,"SetCooperativeLevel(DDSCL_FPUPRESERVE | DDSCL_SETFOCUSWINDOW)
>
> returned: %lx\n",rc);
> -
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_ALLOWREBOOT | DDSCL_SETFOCUSWINDOW);
> - ok(rc==DDERR_INVALIDPARAMS,"SetCooperativeLevel(DDSCL_ALLOWREBOOT | DDSCL_SETFOCUSWINDOW)
>
> returned: %lx\n",rc);
> -
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_ALLOWMODEX | DDSCL_SETFOCUSWINDOW);
> - ok(rc==DDERR_INVALIDPARAMS,"SetCooperativeLevel(DDSCL_ALLOWMODEX | DDSCL_SETFOCUSWINDOW)
>
> returned: %lx\n",rc);
> -
> - /* Set the device window without any other flags. Should give an error */
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_SETDEVICEWINDOW);
> - ok(rc==DDERR_INVALIDPARAMS,"SetCooperativeLevel(DDSCL_SETDEVICEWINDOW) returned:
>
> %lx\n",rc);
> -
> - /* Set device window with DDSCL_NORMAL */
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_NORMAL | DDSCL_SETDEVICEWINDOW);
> - ok(rc==DD_OK,"SetCooperativeLevel(DDSCL_NORMAL | DDSCL_SETDEVICEWINDOW) returned:
>
> %lx\n",rc);
> -
> - /* Also set the focus window. Should give an error */
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_ALLOWMODEX | DDSCL_EXCLUSIVE | DDSCL_FULLSCREEN | DDSCL_SETDEVICEWINDOW |
>
> DDSCL_SETFOCUSWINDOW);
> - ok(rc==DDERR_INVALIDPARAMS,"SetCooperativeLevel(DDSCL_NORMAL | DDSCL_SETDEVICEWINDOW |
>
> DDSCL_SETFOCUSWINDOW) returned: %lx\n",rc);
> -
> - /* All done */
>
> +}
> -
>
> +static void testcooperativelevels_exclusive(void)
> +{
> - HRESULT rc;
> -
> - /* Do some tests with DDSCL_EXCLUSIVE | DDSCL_FULLSCREEN mode */
> -
> - /* Try to set exclusive mode only */
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_EXCLUSIVE);
> - ok(rc==DDERR_INVALIDPARAMS,"SetCooperativeLevel(DDSCL_EXCLUSIVE) returned: %lx\n",rc);
> -
> - /* Full screen mode only */
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_FULLSCREEN);
> - ok(rc==DDERR_INVALIDPARAMS,"SetCooperativeLevel(DDSCL_FULLSCREEN) returned: %lx\n",rc);
> -
> - /* Full screen mode + exclusive mode */
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_FULLSCREEN | DDSCL_EXCLUSIVE);
> - ok(rc==DD_OK,"SetCooperativeLevel(DDSCL_EXCLUSIVE | DDSCL_FULLSCREEN) returned: %lx\n",rc);
> -
> - /* Set the focus window. Should fail */
> - rc = IDirectDraw_SetCooperativeLevel(lpDD,
> -
>    hwnd, DDSCL_SETFOCUSWINDOW);
> - ok(rc==DDERR_HWNDALREADYSET,"SetCooperativeLevel(DDSCL_SETFOCUSWINDOW) returned:
>
> %lx\n",rc);
> -
> -
> - /* All done */
>
> +}
> +START_TEST(cooplevels)
> +{
> - createwindow();
> -
> - createdirectdraw();
> - testcooperativelevels_normal();
> - releasedirectdraw();
> -
> - createdirectdraw();
> - testcooperativelevels_exclusive();
> - releasedirectdraw();
>
> +}
> >

___________________________________________________________ 
Yahoo! Messenger - NEW crystal clear PC to PC calling worldwide with voicemail http://uk.messenger.yahoo.com

Help with a patch needed

[Stefan Dösinger]

Hi,
> > Any comments / Suggestions from the DirectX people?
>
> Seems ok to me, the logging could do with a but of tarting up.
> It's usually really handy if the instance is logged with each FIXME / TRACE
> e.g.
> instead of
> FIXME("Poorly handled flag DDSCL_SETFOCUSWINDOW\n");
> use
> FIXME("(%p) : Poorly handled flag DDSCL_SETFOCUSWINDOW\n", This);
>
> It's also usefull to log whenever a function returns an error.
> e.g.
>     if (!(cooplevel & (DDSCL_EXCLUSIVE|DDSCL_NORMAL|DDSCL_SETFOCUSWINDOW)))
> { TRACE("(%p) : Call to SetCooperativeLevel failed: cooplevel  !=
> DDSCL_EXCLUSIVE|DDSCL_NORMAL|DDSCL_SETFOCUSWINDOW, returning
> DDERR_INVALIDPARAMS); return DDERR_INVALIDPARAMS;
>     }
>
> that way if returning an error is causing something else to fail it's
> fairly easy to pick up.

Thanks for your help, I've just sent a new patch with the suggested 
improvements.

Stefan