August 2017 - Wine-Devel - list.winehq.org

Re: [PATCH 3/3] d3dx9: Return E_FAIL from ValidateTechnique() for techniques with unsupported shaders.
by Matteo Bruni Aug. 31, 2017

Aug. 31, 2017

2017-08-31 19:40 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > static HRESULT WINAPI ID3DXEffectImpl_ValidateTechnique(ID3DXEffect* iface, D3DXHANDLE technique) > { > - struct ID3DXEffectImpl *This = impl_from_ID3DXEffect(iface); > + struct ID3DXEffectImpl *effect = impl_from_ID3DXEffect(iface); > + struct d3dx9_base_effect *base = &effect->base_effect; > + struct d3dx_technique *tech = get_valid_technique(base, technique); > + HRESULT ret = D3D_OK; > + unsigned int i, j; > > - FIXME("(%p)->(%p): stub\n", This, technique); > + FIXME("iface %p, technique %p semi-stub.\n", iface, technique); Do you know what else is missing for ValidateTechnique()? Replying to myself: the return values mentioned in https://msdn.microsoft.com/en-us/library/windows/desktop/bb205835(v=vs.85).… give some ideas (although E_FAIL is not in the list, figures...)

2 1

Re: [PATCH 1/3] d3dx9/tests: Add test for effect with unsupported shader.
by Matteo Bruni Aug. 31, 2017

Aug. 31, 2017

2017-08-31 19:40 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> > --- > dlls/d3dx9_36/tests/effect.c | 185 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 185 insertions(+) > > diff --git a/dlls/d3dx9_36/tests/effect.c b/dlls/d3dx9_36/tests/effect.c > index 9658d2dc82..3e8df11d24 100644 > --- a/dlls/d3dx9_36/tests/effect.c > +++ b/dlls/d3dx9_36/tests/effect.c > @@ -6981,6 +6981,190 @@ static void test_effect_skip_constants(IDirect3DDevice9 *device) > effect->lpVtbl->Release(effect); > } > > +#if 0 > +vertexshader vs_arr[2] = > +{ > + asm > + { > + vs_3_0 > + def c0, 1, 1, 1, 1 > + dcl_position o0 > + mov o0, c0 > + }, > + > + asm > + { > + vs_3_sw > + def c256, 1, 1, 1, 1 > + dcl_position o0 > + mov o0, c256 > + }, > +}; > + > +int i; > + > +technique tech0 > +{ > + pass p0 > + { > + VertexShader = vs_arr[1]; > + } > +} > +technique tech1 > +{ > + pass p0 > + { > + VertexShader = vs_arr[i]; > + } > +} > +#endif > +static const DWORD test_effect_unsupported_shader_blob[] = > +{ > + 0xfeff0901, 0x000000ac, 0x00000000, 0x00000010, 0x00000004, 0x00000020, 0x00000000, 0x00000002, > + 0x00000001, 0x00000002, 0x00000007, 0x615f7376, 0x00007272, 0x00000002, 0x00000000, 0x0000004c, > + 0x00000000, 0x00000000, 0x00000001, 0x00000001, 0x00000000, 0x00000002, 0x00000069, 0x00000003, > + 0x00000010, 0x00000004, 0x00000000, 0x00000000, 0x00000000, 0x00000003, 0x00003070, 0x00000006, > + 0x68636574, 0x00000030, 0x00000004, 0x00000010, 0x00000004, 0x00000000, 0x00000000, 0x00000000, > + 0x00000003, 0x00003070, 0x00000006, 0x68636574, 0x00000031, 0x00000002, 0x00000002, 0x00000006, > + 0x00000005, 0x00000004, 0x00000018, 0x00000000, 0x00000000, 0x0000002c, 0x00000048, 0x00000000, > + 0x00000000, 0x00000074, 0x00000000, 0x00000001, 0x0000006c, 0x00000000, 0x00000001, 0x00000092, > + 0x00000000, 0x00000058, 0x00000054, 0x000000a0, 0x00000000, 0x00000001, 0x00000098, 0x00000000, > + 0x00000001, 0x00000092, 0x00000000, 0x00000084, 0x00000080, 0x00000002, 0x00000002, 0x00000001, > + 0x00000038, 0xfffe0300, 0x05000051, 0xa00f0000, 0x3f800000, 0x3f800000, 0x3f800000, 0x3f800000, > + 0x0200001f, 0x80000000, 0xe00f0000, 0x02000001, 0xe00f0000, 0xa0e40000, 0x0000ffff, 0x00000002, > + 0x00000038, 0xfffe0300, 0x05000051, 0xa00f0100, 0x3f800000, 0x3f800000, 0x3f800000, 0x3f800000, > + 0x0200001f, 0x80000000, 0xe00f0000, 0x02000001, 0xe00f0000, 0xa0e40100, 0x0000ffff, 0x00000001, > + 0x00000000, 0xffffffff, 0x00000000, 0x00000002, 0x000000e4, 0x00000008, 0x615f7376, 0x00007272, > + 0x46580200, 0x0023fffe, 0x42415443, 0x0000001c, 0x00000057, 0x46580200, 0x00000001, 0x0000001c, > + 0x00000100, 0x00000054, 0x00000030, 0x00000002, 0x00000001, 0x00000034, 0x00000044, 0xabab0069, > + 0x00020000, 0x00010001, 0x00000001, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, > + 0x4d007874, 0x6f726369, 0x74666f73, 0x29522820, 0x534c4820, 0x6853204c, 0x72656461, 0x6d6f4320, > + 0x656c6970, 0x2e392072, 0x392e3932, 0x332e3235, 0x00313131, 0x0002fffe, 0x54494c43, 0x00000000, > + 0x000cfffe, 0x434c5846, 0x00000001, 0x10000001, 0x00000001, 0x00000000, 0x00000002, 0x00000000, > + 0x00000000, 0x00000004, 0x00000000, 0xf0f0f0f0, 0x0f0f0f0f, 0x0000ffff, 0x00000000, 0x00000000, > + 0xffffffff, 0x00000000, 0x00000001, 0x0000000a, 0x615f7376, 0x315b7272, 0x0000005d, > +}; > + > +#define TEST_EFFECT_UNSUPPORTED_SHADER_VSHADER_POS 81 > +#define TEST_EFFECT_UNSUPPORTED_SHADER_VSHADER_LEN 14 > + > +static void test_effect_unsupported_shader(void) > +{ > + D3DPRESENT_PARAMETERS present_parameters = {0}; > + IDirect3DVertexShader9 *vshader; > + unsigned int passes_count; > + IDirect3DDevice9 *device; > + UINT byte_code_size; > + ID3DXEffect *effect; > + IDirect3D9 *d3d; > + void *byte_code; > + ULONG refcount; > + HWND window; > + HRESULT hr; > + > + if (!(window = CreateWindowA("static", "d3dx9_test", WS_OVERLAPPEDWINDOW, 0, 0, > + 640, 480, NULL, NULL, NULL, NULL))) > + { > + skip("Couldn't create application window\n"); > + return; > + } > + if (!(d3d = Direct3DCreate9(D3D_SDK_VERSION))) > + { > + skip("Couldn't create IDirect3D9 object\n"); > + DestroyWindow(window); > + return; > + } > + present_parameters.Windowed = TRUE; > + present_parameters.SwapEffect = D3DSWAPEFFECT_DISCARD; > + hr = IDirect3D9_CreateDevice(d3d, D3DADAPTER_DEFAULT, D3DDEVTYPE_HAL, window, > + D3DCREATE_HARDWARE_VERTEXPROCESSING, &present_parameters, &device); > + if (FAILED(hr)) { > + skip("Failed to create IDirect3DDevice9 object, hr %#x\n", hr); > + IDirect3D9_Release(d3d); > + DestroyWindow(window); > + return; > + } > + > + hr = D3DXCreateEffectEx(device, test_effect_unsupported_shader_blob, sizeof(test_effect_unsupported_shader_blob), > + NULL, NULL, NULL, 0, NULL, &effect, NULL); > + todo_wine > + ok(hr == D3D_OK, "Got result %#x.\n", hr); > + if (FAILED(hr)) > + { > + skip("Failed to create effect, skipping test.\n"); > + goto cleanup; > + } > + > + hr = effect->lpVtbl->ValidateTechnique(effect, "missing_technique"); > + ok(hr == D3DERR_INVALIDCALL, "Got result %#x.\n", hr); > + hr = effect->lpVtbl->ValidateTechnique(effect, "tech0"); > + ok(hr == E_FAIL, "Got result %#x.\n", hr); > + > + hr = effect->lpVtbl->ValidateTechnique(effect, "tech1"); > + ok(hr == D3D_OK, "Got result %#x.\n", hr); > + effect->lpVtbl->SetInt(effect, "i", 1); > + ok(hr == D3D_OK, "Got result %#x.\n", hr); > + hr = effect->lpVtbl->ValidateTechnique(effect, "tech1"); > + ok(hr == E_FAIL, "Got result %#x.\n", hr); > + effect->lpVtbl->SetInt(effect, "i", 0); > + hr = effect->lpVtbl->ValidateTechnique(effect, "tech1"); > + ok(hr == D3D_OK, "Got result %#x.\n", hr); > + > + hr = effect->lpVtbl->SetTechnique(effect, "tech0"); > + ok(hr == D3D_OK, "Got result %#x.\n", hr); > + hr = effect->lpVtbl->Begin(effect, &passes_count, 0); > + ok(hr == D3D_OK, "Got result %#x.\n", hr); > + hr = effect->lpVtbl->BeginPass(effect, 0); > + ok(hr == D3D_OK, "Got result %#x.\n", hr); > + > + hr = IDirect3DDevice9_GetVertexShader(device, &vshader); > + ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr); > + ok(!vshader, "Got non NULL vshader.\n"); > + > + hr = effect->lpVtbl->EndPass(effect); > + ok(hr == D3D_OK, "Got result %#x.\n", hr); > + hr = effect->lpVtbl->End(effect); > + ok(hr == D3D_OK, "Got result %#x.\n", hr); > + > + hr = effect->lpVtbl->SetTechnique(effect, "tech1"); > + ok(hr == D3D_OK, "Got result %#x.\n", hr); > + hr = effect->lpVtbl->Begin(effect, &passes_count, 0); > + ok(hr == D3D_OK, "Got result %#x.\n", hr); > + hr = effect->lpVtbl->BeginPass(effect, 0); > + ok(hr == D3D_OK, "Got result %#x.\n", hr); > + > + hr = IDirect3DDevice9_GetVertexShader(device, &vshader); > + ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr); > + ok(!!vshader, "Got NULL vshader.\n"); > + hr = IDirect3DVertexShader9_GetFunction(vshader, NULL, &byte_code_size); > + ok(hr == D3D_OK, "Got result %x.\n", hr); > + byte_code = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, byte_code_size); > + hr = IDirect3DVertexShader9_GetFunction(vshader, byte_code, &byte_code_size); > + ok(hr == D3D_OK, "Got result %x.\n", hr); > + ok(byte_code_size == TEST_EFFECT_UNSUPPORTED_SHADER_VSHADER_LEN * sizeof(DWORD), > + "Got unexpected byte code size %u.\n", byte_code_size); > + ok(!memcmp(byte_code, > + &test_effect_unsupported_shader_blob[TEST_EFFECT_UNSUPPORTED_SHADER_VSHADER_POS], > + byte_code_size), "Incorrect shader selected.\n"); I was confused by this at first, I thought somehow the unsupported vs_3_sw shader was set. That's not the case, TEST_EFFECT_UNSUPPORTED_SHADER_VSHADER_POS is the position of the vs_3_0 shader, but the naming throw me off. Can you rename those defines to "BYTECODE_VS_3_0_POS" or something that makes it clear that it refers to the hardware shader in vs_arr[0]?

2 2

Re: [PATCH v2 10/11] progman: Use proper size for combobox controls
by Alexandre Julliard Aug. 31, 2017

Aug. 31, 2017

Fabian Maurer <dark.shadow4(a)web.de> writes: > Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> > --- > programs/progman/progman.rc | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/programs/progman/progman.rc b/programs/progman/progman.rc > index 4804330aed..9f451a736b 100644 > --- a/programs/progman/progman.rc > +++ b/programs/progman/progman.rc > @@ -119,7 +119,7 @@ LTEXT "", PM_PROGRAM, 95, 5, 90, 15 > LTEXT "From group:", -1, 5, 13, 90, 15 > LTEXT "", PM_FROM_GROUP, 95, 13, 90, 15 > LTEXT "&To group:", PM_TO_GROUP_TXT, 5, 28, 140, 15 > -COMBOBOX PM_TO_GROUP, 5, 38, 140, 50, WS_TABSTOP | CBS_DROPDOWNLIST > +COMBOBOX PM_TO_GROUP, 5, 38, 140, 60, WS_TABSTOP | CBS_DROPDOWNLIST That kind of change doesn't seem necessary. We don't need all comboboxes to have the exact same height, only something reasonable. -- Alexandre Julliard julliard(a)winehq.org

3 2

Wine fails to compile in kernel32
by Fabian Maurer Aug. 30, 2017

Aug. 30, 2017

Hello wine-devel, I'm running ArchLinux with gcc 7.2.0. Since commit e99811aee2dff88c41d6e2875401810a85325839 wine fails to build with > ../../../wine-git/dlls/kernel32/module.c: In function ‘load_library’: > ../../../wine-git/dlls/kernel32/module.c:987:44: error: initializer element is not constant > static const DWORD unsupported_flags = load_library_search_flags | Minimal test-program to show the error, just use something like "gcc main.c" to compile. ---------- static const int load_library_search_flags = 0; int main() { static const int unsupported_flags = load_library_search_flags; return 0; } ---------- Looks like an compiler error, but I'm not sure. Mind looking into that? Regards, Fabian Maurer

1 1

Re: [PATCH] winecfg: Use proper size for combobox controls
by Nikolay Sivov Aug. 30, 2017

Aug. 30, 2017

On 29.08.2017 20:06, Fabian Maurer wrote: > Fixes Bug 43632. > winecfg used the broken behavior that was fixed > in f7f7b89e2e9117811c91269643868c6d063db5e9, > so we need to adjust it. > > Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> > --- > programs/winecfg/winecfg.rc | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > Interesting thing to test I think is to verify if current cross-built winecfg shows the same issue on Windows, before changing them all.

2 1

Re: [PATCH] winecfg: Use proper size for combobox controls
by Alexandre Julliard Aug. 30, 2017

Aug. 30, 2017

Fabian Maurer <dark.shadow4(a)web.de> writes: > Fixes Bug 43632. > winecfg used the broken behavior that was fixed > in f7f7b89e2e9117811c91269643868c6d063db5e9, > so we need to adjust it. > > Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> > --- > programs/winecfg/winecfg.rc | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) There are resources in other modules that look like they would have the same problem, a grep over the codebase would probably be useful. -- Alexandre Julliard julliard(a)winehq.org

2 3

WINED3DFMT_RESZ in wined3d.
by lorenzo Skotos Aug. 30, 2017

Aug. 30, 2017

Hi. I was investigating a regression that cause the CheckDeviceFormat function to return WINED3DERR_NOTAVAILABLE when quering for RESZ surfaces. (WINED3DFMT_RESZ). This is causing OblivionReloaded (that worked properly until wine 2.5) to not work properly (it detect an incompatible hardware in wine or NVIDIA in wine-staging). I tacked the regression to this commit : https://github.com/wine-mirror/wine/commit/8c98be4791f18f31b04a4a0f08d18979… [https://camo.githubusercontent.com/db3db2bf09d082a1f4244859890e83954d8410ab…]<https://github.com/wine-mirror/wine/commit/8c98be4791f18f31b04a4a0f08d18979…> wined3d: Allow all formats with "glInternal" set in CheckSurfaceCapab… · wine-mirror/wine(a)8c98be4<https://github.com/wine-mirror/wine/commit/8c98be4791f18f31b04a4a0f08d18979…> github.com …ility(). Rendertarget, depth/stencil and texturing restrictions are applied later in wined3d_check_device_format(), so to the extent this make a difference it mostly affects off-screen plain surf... I tried to fix this issue. The last patch I made in my local branch is adding a wined3d_format_texture_info in the format_texture_info array: + {WINED3DFMT_RESZ, GL_DEPTH24_STENCIL8, GL_DEPTH24_STENCIL8, 0, + GL_DEPTH_STENCIL, GL_UNSIGNED_INT, 0, + WINED3DFMT_FLAG_TEXTURE | WINED3DFMT_FLAG_DEPTH | WINED3DFMT_FLAG_STENCIL | WINED3DFMT_FLAG_RENDERTARGET | WINED3DFMT_FLAG_FBO_ATTACHABLE, + ARB_FRAMEBUFFER_OBJECT, NULL}, However I know almost nothing about the internal work of wined3d and opengl and don't know if this kind of addition is correct. OblivionReloaded works and can properly use the RESZ depth texture. I based this patch on the INTZ and RAWZ surfaces that are somewhat reported to be similar in scope in some docs. Also I don't know if the flags are correct or even meaningful in this scope. I could not find any documentation about the internal meaning of these flags. As the good Jon Snow I know nothing. Note I never did a patch in wine before (except adding some debug message). So please tell me if I'm in a completly wrong direction. Also note that I didn't run the test suite. Using make test is failing hard on an advapi32 test even without this patch applied. I attached the full patch file.

1 0

Re: [PATCH] user32: In GetMinMaxInfo, use the default X pos even if the Y pos changes
by Alexandre Julliard Aug. 30, 2017

Aug. 30, 2017

Andrew Eikum <aeikum(a)codeweavers.com> writes: > This fixes maximizing to the wrong monitor on a Mac with multiple > monitors. Maximizing with multiple monitors continues to work correctly > on Linux for me. > > Signed-off-by: Andrew Eikum <aeikum(a)codeweavers.com> > --- > dlls/user32/winpos.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) I'm not sure it makes sense to use only one coordinate of the monitor rectangle. What are the coordinate values that trigger this? -- Alexandre Julliard julliard(a)winehq.org

1 0

Re: [PATCH 2/2] d3dx9: Get rid of constants modification bitmasks.
by Matteo Bruni Aug. 29, 2017

Aug. 29, 2017

2017-08-10 12:02 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> The patch is fine overall, I only have a few questions / nitpicks. I'll skip the part about the delay with the review because it's old news by now... :/ > --- > dlls/d3dx9_36/d3dx9_private.h | 3 +- > dlls/d3dx9_36/preshader.c | 312 ++++++++++++++++++------------------------ > 2 files changed, 135 insertions(+), 180 deletions(-) > > diff --git a/dlls/d3dx9_36/d3dx9_private.h b/dlls/d3dx9_36/d3dx9_private.h > index 2727463a18..5ac5e63aeb 100644 > --- a/dlls/d3dx9_36/d3dx9_private.h > +++ b/dlls/d3dx9_36/d3dx9_private.h > @@ -230,7 +230,7 @@ enum pres_reg_tables > struct d3dx_const_param_eval_output > { > struct d3dx_parameter *param; > - unsigned int table; > + enum pres_reg_tables table; The change is good but (unrelated to this patch) the enum type should really use the singular form. > static void dump_bytecode(void *data, unsigned int size) > @@ -674,6 +614,40 @@ static HRESULT append_const_set(struct d3dx_const_tab *const_tab, struct d3dx_co > return D3D_OK; > } > > +static void append_pres_const_set(struct d3dx_const_tab *const_tab, struct d3dx_preshader *pres) Nitpick: this function appends all the struct d3dx_const_param_eval_output from the preshader so calling it "append_pres_const_sets" (or even something more explicit to that effect, e.g. clarifying that you're appending entries about the preshader outputs, which are shader inputs) would be preferable. > +{ > + unsigned int i; > + struct d3dx_const_param_eval_output const_set; > + > + memset(&const_set, 0, sizeof(const_set)); You could initialize const_set in the definition instead, like: struct d3dx_const_param_eval_output const_set = {NULL}; which looks slightly better to me. It doesn't matter much of course. > @@ -971,10 +946,30 @@ static HRESULT get_constants_desc(unsigned int *byte_code, struct d3dx_const_tab > if (FAILED(hr = init_set_constants_param(out, ctab, hc, inputs_param[index]))) > goto cleanup; > } > + if (pres) > + append_pres_const_set(out, pres); > if (out->const_set_count) > { > struct d3dx_const_param_eval_output *new_alloc; > > + qsort(out->const_set, out->const_set_count, sizeof(*out->const_set), compare_const_set); > + for (i = 0; i < out->const_set_count - 1; ++i) > + { > + if (out->const_set[i].constant_class == D3DXPC_FORCE_DWORD > + && out->const_set[i + 1].constant_class == D3DXPC_FORCE_DWORD > + && out->const_set[i].table == out->const_set[i + 1].table > + && out->const_set[i].register_index + out->const_set[i].register_count > + >= out->const_set[i + 1].register_index) > + { > + out->const_set[i].register_count = out->const_set[i + 1].register_index > + + out->const_set[i + 1].register_count - out->const_set[i].register_index; I think this might lose account of some registers. E.g. consider the case where const_set[i] has register_index == 0 and register_count == 4 while const_set[i + 1] has register_index == 1 and register_count == 1. This would seem to lose track of the last two components. I know that preshaders found in the wild seem not to overwrite already written output registers but I wouldn't feel safe with (silently) depending on it. > + memmove(&out->const_set[i + 1], &out->const_set[i + 2], sizeof(out->const_set[i]) > + * (out->const_set_count - i - 2)); > + --out->const_set_count; > + --i; Mostly matter of personal preference but rather than decrementing the for counter in the if when coalescing I'd write this as a while and increment "i" only in the (currently absent) else case. > @@ -1529,22 +1534,56 @@ static void set_constants(struct d3dx_regstore *rs, struct d3dx_const_tab *const > data += param->rows * param->columns; > } > start_offset = get_offset_reg(table, const_set->register_index); > - if (table_info[table].type == param_type) > - regstore_set_modified(rs, table, start_offset, > - get_offset_reg(table, const_set->register_count) * const_set->element_count); > - else > + if (table_info[table].type != param_type) > regstore_set_data(rs, table, start_offset, (unsigned int *)rs->tables[table] + start_offset, > get_offset_reg(table, const_set->register_count) * const_set->element_count, param_type); > } > + > + > const_tab->update_version = new_update_version; One blank line too many. > -} > + if (!update_device) > + return D3D_OK; > > + for (const_idx = 0; const_idx < const_tab->const_set_count; ++const_idx) > + { > + struct d3dx_const_param_eval_output *const_set = &const_tab->const_set[const_idx]; > + > + if (device_update_all || (const_set->param > + ? is_param_dirty(const_set->param, update_version) : pres_dirty)) > + { > + enum pres_reg_tables table = const_set->table; > + > + if (table == device_table && device_start + device_count == const_set->register_index) Again nitpicking: are there better names for those variables? Maybe use the "current_" prefix instead? It's pretty clear what's going on either way, so up to you.

2 6

Re: [PATCH] comctl32/taskdialog: Add callback and tests
by Nikolay Sivov Aug. 28, 2017

Aug. 28, 2017

On 28.07.2017 19:53, Fabian Maurer wrote: > > +static HRESULT callback(struct taskdialog_info *dialog_info, UINT uNotification, WPARAM wParam, LPARAM lParam) > +{ > + if(dialog_info->callback) > + return dialog_info->callback(dialog_info->hwnd, uNotification, wParam, lParam, dialog_info->callback_data); > + return S_OK; > +} Please use consistent naming and formatting, like space after 'if' for example, and uNotification -> notification. > + > static INT_PTR CALLBACK taskdialog_proc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam) > { > + static const WCHAR taskdialog_info_propnameW[] = {'T','a','s','k','D','i','a','l','o','g','I','n','f','o',0}; > + struct taskdialog_info *dialog_info; > + > TRACE("hwnd=%p msg=0x%04x wparam=%lx lparam=%lx\n", hwnd, msg, wParam, lParam); > > + if(msg != WM_INITDIALOG && msg != WM_NCDESTROY) > + dialog_info = GetPropW(hwnd, taskdialog_info_propnameW); > + Why WM_NCDESTROY is a special case? > switch (msg) > { > + case WM_INITDIALOG: > + dialog_info = (struct taskdialog_info *)lParam; > + dialog_info->hwnd = hwnd; > + SetPropW(hwnd, taskdialog_info_propnameW, dialog_info); > + > + callback(dialog_info, TDN_DIALOG_CONSTRUCTED, 0, 0); > + callback(dialog_info, TDN_CREATED, 0, 0); > + break; This looks suspicious, documentation is not strict on this, but it seems TDN_CREATED corresponds for to WM_SHOWWINDOW stage. > case WM_COMMAND: > if (HIWORD(wParam) == BN_CLICKED) > { > WORD command_id = LOWORD(wParam); > - EndDialog(hwnd, command_id); > + click_button(dialog_info, command_id); > return TRUE; > } > break; > + case WM_DESTROY: > + callback(dialog_info, TDN_DESTROYED, 0, 0); > + RemovePropW(hwnd, taskdialog_info_propnameW); > + break; > + > + /* Custom messages*/ What do you mean by "custom"? > + > + case TDM_CLICK_BUTTON: > + click_button(dialog_info, wParam); > + break; > } > return FALSE; > } > @@ -507,17 +556,24 @@ HRESULT WINAPI TaskDialogIndirect(const TASKDIALOGCONFIG *taskconfig, int *butto > int *radio_button, BOOL *verification_flag_checked) > { > DLGTEMPLATE *template; > + struct taskdialog_info dialog_info; > INT ret; > > TRACE("%p, %p, %p, %p\n", taskconfig, button, radio_button, verification_flag_checked); > > + if(!taskconfig || taskconfig->cbSize != sizeof(TASKDIALOGCONFIG)) > + return E_INVALIDARG; Tests you added only check cbSize == 0 case. > + > + dialog_info.callback = taskconfig->pfCallback; > + dialog_info.callback_data = taskconfig->lpCallbackData; > + > template = create_taskdialog_template(taskconfig); > - ret = DialogBoxIndirectParamW(taskconfig->hInstance, template, taskconfig->hwndParent, taskdialog_proc, 0); > + ret = DialogBoxIndirectParamW(taskconfig->hInstance, template, taskconfig->hwndParent, taskdialog_proc, (LPARAM)&dialog_info); > Free(template); > > if (button) *button = ret; > if (radio_button) *radio_button = taskconfig->nDefaultButton; > - if (verification_flag_checked) *verification_flag_checked = TRUE; > + if (verification_flag_checked) *verification_flag_checked = FALSE; Why do you need this flag value change? > > return S_OK; > } > diff --git a/dlls/comctl32/tests/taskdialog.c b/dlls/comctl32/tests/taskdialog.c > index c937127897..867361617c 100644 > --- a/dlls/comctl32/tests/taskdialog.c > +++ b/dlls/comctl32/tests/taskdialog.c > @@ -24,13 +24,250 @@ > #include "winuser.h" > #include "commctrl.h" > > +#include "wine/list.h" > #include "wine/test.h" > #include "v6util.h" > +#include "msg.h" > + > +#define WM_TD_CALLBACK (WM_APP) /* Custom dummy message to wrap callback notifications */ > + > +#define NUM_MSG_SEQUENCES 1 > +#define TASKDIALOG_SEQ_INDEX 0 > + > +#define TEST_NUM_BUTTONS 20 /* Number of custom buttons to test with */ > + > +#define ID_START 20 /* Lower IDs might be used by the system */ > +#define ID_START_BUTTON (ID_START + 0) > > static HRESULT (WINAPI *pTaskDialogIndirect)(const TASKDIALOGCONFIG *, int *, int *, BOOL *); > static HRESULT (WINAPI *pTaskDialog)(HWND, HINSTANCE, const WCHAR *, const WCHAR *, const WCHAR *, > TASKDIALOG_COMMON_BUTTON_FLAGS, const WCHAR *, int *); > > +static struct msg_sequence *sequences[NUM_MSG_SEQUENCES]; > + > +/* Message lists to test against */ > + > +struct message_send_info > +{ > + UINT send_message; > + WPARAM send_wparam; > + LPARAM send_lparam; > + > + BOOL post; /* post instead of send */ > + const CHAR *title_target; /* control text, 0 means it's send to the dialog form instead */ > +}; > + > +struct message_info > +{ > + UINT recv_message; /* Message the callback receives */ > + WPARAM recv_wparam; > + LPARAM recv_lparam; > + > + HRESULT ret; /* Value the callback should return */ > + > + struct message_send_info send[9]; /* Message to send to trigger the next callback message */ > +}; > + > +static const struct message_info *current_message_info; > + > +static const struct message_info mes_return_press_ok[] = { > + { TDN_CREATED, 0, 0, S_OK, { > + { WM_KEYDOWN, VK_RETURN, 0, TRUE }, > + { 0 }}}, > + { TDN_BUTTON_CLICKED, IDOK, 0, S_OK, {{ 0 }}}, > + { 0 } > +}; > + > +static const struct message_info mes_cancel_button_press[] = { > + { TDN_CREATED, 0, 0, S_OK, { > + { TDM_CLICK_BUTTON, IDOK, 0, TRUE }, > + { TDM_CLICK_BUTTON, IDOK, 0, TRUE }, > + { TDM_CLICK_BUTTON, IDOK, 0, TRUE }, > + { 0 }}}, > + { TDN_BUTTON_CLICKED, IDOK, 0, S_FALSE, {{ 0 }}}, > + { TDN_BUTTON_CLICKED, IDOK, 0, 0xFF, {{ 0 }}}, /* Random return value tested here */ > + { TDN_BUTTON_CLICKED, IDOK, 0, S_OK, {{ 0 }}}, > + { 0 } > +}; > + > +static const struct message_info mes_send_button_directly[] = { > + { TDN_CREATED, 0, 0, S_OK, { > + { WM_LBUTTONDOWN, MK_LBUTTON, 0, TRUE, "02" }, > + { WM_LBUTTONUP, 0, 0, TRUE, "02" }, > + { 0 }}}, > + { TDN_BUTTON_CLICKED, ID_START + 2, 0, S_OK, {{ 0 }}}, > + { 0 } > +}; This looks too complicated, I'd just check notifications callback received, using 'struct message' data, same way WM_NOTIFY tests work. > + > +/* Create a message to test against */ > +static struct message create_test_message(UINT message, WPARAM wParam, LPARAM lParam) > +{ > + struct message mes; > + > + mes.message = WM_TD_CALLBACK; > + mes.id = message; > + mes.flags = sent|wparam|lparam|id; > + mes.wParam = wParam; > + mes.lParam = lParam; > + > + return mes; > +} > + > +/* Our only way to get a button handle, since GetDlgItem and FindWindowEx don't work for the official taskdialog */ > + > +static HWND taskdialog_child; > +BOOL CALLBACK enum_taskdialog_children_proc(HWND hwnd, LPARAM lParam) > +{ > + CHAR text[100]; > + const CHAR *title = (const CHAR *)lParam; > + > + GetWindowTextA(hwnd, text, sizeof(text)); > + > + if(lstrcmpA(text, title) == 0) > + { > + taskdialog_child = hwnd; > + return FALSE; > + } > + > + return TRUE; > +} > + > +static HWND get_child_from_title(HWND hwnd_parent, const CHAR *title) > +{ > + taskdialog_child = NULL; > + EnumChildWindows(hwnd_parent, enum_taskdialog_children_proc, (LPARAM)title); > + return taskdialog_child; > +} I don't think we need to get that deep. > > + init_msg_sequences(sequences, NUM_MSG_SEQUENCES); > + test_TaskDialogIndirect(); This name is too generic, in a test function like that you can test invalid parameter handling for example. Then have another function for callback notifications, and another one for TDM_* messages. I think for first iteration test should simply check creation/destruction notifications and callback user data value.

2 1