[PATCH v10 0/2] MR2068: comdlg32: Enable visual styles when showing IFileDialog.
Required for !1857 to not break when comctl32 version 6 isn't requested by application (in particular, found this in [qapitrace](https://github.com/apitrace/apitrace)). Manifest resource id and assembly identity name match with Windows. For isolation purposes, activation context is disabled while emitting events. -- v10: comdlg32: Enable visual styles when showing IFileDialog. https://gitlab.winehq.org/wine/wine/-/merge_requests/2068
From: Vladislav Timonin <timoninvlad(a)yandex.ru> --- dlls/comdlg32/itemdlg.c | 3 +++ dlls/comdlg32/tests/itemdlg.c | 48 ++++++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/dlls/comdlg32/itemdlg.c b/dlls/comdlg32/itemdlg.c index 1feb1deda00..b483193d76b 100644 --- a/dlls/comdlg32/itemdlg.c +++ b/dlls/comdlg32/itemdlg.c @@ -2246,6 +2246,9 @@ static HRESULT create_dialog(FileDialogImpl *This, HWND parent) { INT_PTR res; + if (This->dlg_hwnd) + return E_UNEXPECTED; + SetLastError(0); res = DialogBoxParamW(COMDLG32_hInstance, MAKEINTRESOURCEW(NEWFILEOPENV3ORD), diff --git a/dlls/comdlg32/tests/itemdlg.c b/dlls/comdlg32/tests/itemdlg.c index 8ce0ea806b9..ec681cf3c33 100644 --- a/dlls/comdlg32/tests/itemdlg.c +++ b/dlls/comdlg32/tests/itemdlg.c @@ -27,12 +27,14 @@ #define IDT_CHANGEFILETYPE 500 #define IDT_CLOSEDIALOG 501 +#define IDT_SHOWDIALOG 502 typedef enum { IFDEVENT_TEST_NONE = 0, IFDEVENT_TEST1 = 0x1, IFDEVENT_TEST2 = 0x2, - IFDEVENT_TEST3 = 0x3 + IFDEVENT_TEST3 = 0x3, + IFDEVENT_TEST4 = 0x4, } FileDialogEventsTest; static HRESULT (WINAPI *pSHCreateShellItem)(LPCITEMIDLIST,IShellFolder*,LPCITEMIDLIST,IShellItem**); @@ -193,6 +195,15 @@ static LRESULT CALLBACK test_customize_dlgproc(HWND hwnd, UINT message, WPARAM w br = PostMessageW(hwnd, WM_COMMAND, IDCANCEL, 0); ok(br, "Failed\n"); return TRUE; + case IDT_SHOWDIALOG: + { + HRESULT hr; + KillTimer(hwnd, IDT_SHOWDIALOG); + hr = IFileDialog_Show(pfd, NULL); + ok(hr == E_UNEXPECTED, "got 0x%08lx.\n", hr); + SetTimer(hwnd, IDT_CLOSEDIALOG, 100, 0); + return TRUE; + } } } @@ -249,6 +260,9 @@ static HRESULT WINAPI IFileDialogEvents_fnOnFolderChange(IFileDialogEvents *ifac case IFDEVENT_TEST3: SetTimer(dlg_hwnd, IDT_CHANGEFILETYPE, 100, 0); break; + case IFDEVENT_TEST4: + SetTimer(dlg_hwnd, IDT_SHOWDIALOG, 100, 0); + break; default: ok(FALSE, "Should not happen (%d)\n", This->events_test); } @@ -2496,6 +2510,37 @@ static void test_customize_remove_from_empty_combobox(void) IFileDialog_Release(pfod); } +static void test_double_show(void) +{ + IFileDialogEventsImpl *pfdeimpl; + IFileDialogEvents *pfde; + IFileDialog *pfd; + DWORD cookie; + HRESULT hr; + + hr = CoCreateInstance(&CLSID_FileOpenDialog, NULL, CLSCTX_INPROC_SERVER, + &IID_IFileDialog, (void**)&pfd); + ok(hr == S_OK, "got 0x%08lx.\n", hr); + + pfde = IFileDialogEvents_Constructor(); + pfdeimpl = impl_from_IFileDialogEvents(pfde); + pfdeimpl->events_test = IFDEVENT_TEST4; + + hr = IFileDialog_Advise(pfd, pfde, &cookie); + ok(hr == S_OK, "got 0x%08lx.\n", hr); + + ensure_zero_events(pfdeimpl); + + hr = IFileDialog_Show(pfd, NULL); + ok(hr == HRESULT_FROM_WIN32(ERROR_CANCELLED), "got 0x%08lx.\n", hr); + + hr = IFileDialog_Unadvise(pfd, cookie); + ok(hr == S_OK, "got 0x%08lx.\n", hr); + + IFileDialogEvents_Release(pfde); + IFileDialog_Release(pfd); +} + START_TEST(itemdlg) { OleInitialize(NULL); @@ -2521,6 +2566,7 @@ START_TEST(itemdlg) test_persistent_state(); test_overwrite(); test_customize_remove_from_empty_combobox(); + test_double_show(); } else skip("Skipping all Item Dialog tests.\n"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2068
From: Vladislav Timonin <timoninvlad(a)yandex.ru> --- dlls/comdlg32/cdlg.h | 1 + dlls/comdlg32/cdlg32.c | 16 +++++- dlls/comdlg32/comdlg32.manifest | 16 ++++++ dlls/comdlg32/comdlg32.rc | 3 ++ dlls/comdlg32/itemdlg.c | 94 +++++++++++++++++++++++++++++++++ 5 files changed, 129 insertions(+), 1 deletion(-) create mode 100644 dlls/comdlg32/comdlg32.manifest diff --git a/dlls/comdlg32/cdlg.h b/dlls/comdlg32/cdlg.h index 8039d6d6bd0..ae3c3d1ef37 100644 --- a/dlls/comdlg32/cdlg.h +++ b/dlls/comdlg32/cdlg.h @@ -27,6 +27,7 @@ #define COMDLG32_Atom MAKEINTATOM(0xa000) /* MS uses this one to identify props */ extern HINSTANCE COMDLG32_hInstance DECLSPEC_HIDDEN; +extern HANDLE COMDLG32_hActCtx DECLSPEC_HIDDEN; void COMDLG32_SetCommDlgExtendedError(DWORD err) DECLSPEC_HIDDEN; LPVOID COMDLG32_AllocMem(int size) __WINE_ALLOC_SIZE(1) DECLSPEC_HIDDEN; diff --git a/dlls/comdlg32/cdlg32.c b/dlls/comdlg32/cdlg32.c index b9e19b78a34..19f4333acf5 100644 --- a/dlls/comdlg32/cdlg32.c +++ b/dlls/comdlg32/cdlg32.c @@ -40,6 +40,7 @@ WINE_DEFAULT_DEBUG_CHANNEL(commdlg); DECLSPEC_HIDDEN HINSTANCE COMDLG32_hInstance = 0; +HANDLE COMDLG32_hActCtx = INVALID_HANDLE_VALUE; static DWORD COMDLG32_TlsIndex = TLS_OUT_OF_INDEXES; @@ -59,13 +60,26 @@ BOOL WINAPI DllMain(HINSTANCE hInstance, DWORD Reason, LPVOID Reserved) switch(Reason) { case DLL_PROCESS_ATTACH: + { + ACTCTXW actctx = {0}; + COMDLG32_hInstance = hInstance; DisableThreadLibraryCalls(hInstance); - break; + actctx.cbSize = sizeof(actctx); + actctx.hModule = COMDLG32_hInstance; + actctx.lpResourceName = MAKEINTRESOURCEW(123); + actctx.dwFlags = ACTCTX_FLAG_HMODULE_VALID | ACTCTX_FLAG_RESOURCE_NAME_VALID; + COMDLG32_hActCtx = CreateActCtxW(&actctx); + if (COMDLG32_hActCtx == INVALID_HANDLE_VALUE) + ERR("failed to create activation context, last error %lu\n", GetLastError()); + + break; + } case DLL_PROCESS_DETACH: if (Reserved) break; if (COMDLG32_TlsIndex != TLS_OUT_OF_INDEXES) TlsFree(COMDLG32_TlsIndex); + if (COMDLG32_hActCtx != INVALID_HANDLE_VALUE) ReleaseActCtx(&COMDLG32_hActCtx); break; } return TRUE; diff --git a/dlls/comdlg32/comdlg32.manifest b/dlls/comdlg32/comdlg32.manifest new file mode 100644 index 00000000000..f9e7b7cc115 --- /dev/null +++ b/dlls/comdlg32/comdlg32.manifest @@ -0,0 +1,16 @@ +<?xml version="1.0" encoding="UTF-8" standalone="yes"?> +<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0"> +<assemblyIdentity type="win32" name="Microsoft.Windows.Shell.comdlg32" version="0.0.0.0"/> +<dependency> + <dependentAssembly> + <assemblyIdentity + type="win32" + name="Microsoft.Windows.Common-Controls" + version="6.0.0.0" + processorArchitecture="*" + publicKeyToken="6595b64144ccf1df" + language="*" + /> + </dependentAssembly> +</dependency> +</assembly> diff --git a/dlls/comdlg32/comdlg32.rc b/dlls/comdlg32/comdlg32.rc index a5d09faa563..474bafd8760 100644 --- a/dlls/comdlg32/comdlg32.rc +++ b/dlls/comdlg32/comdlg32.rc @@ -602,3 +602,6 @@ NETWORK ICON network.ico /* @makedep: fontpics.bmp */ 38 BITMAP fontpics.bmp + +/* @makedep: comdlg32.manifest */ +123 RT_MANIFEST comdlg32.manifest diff --git a/dlls/comdlg32/itemdlg.c b/dlls/comdlg32/itemdlg.c index b483193d76b..be662276db4 100644 --- a/dlls/comdlg32/itemdlg.c +++ b/dlls/comdlg32/itemdlg.c @@ -145,6 +145,8 @@ typedef struct FileDialogImpl { DWORD opendropdown_selection; GUID client_guid; + + HANDLE user_actctx; } FileDialogImpl; /************************************************************************** @@ -152,10 +154,14 @@ typedef struct FileDialogImpl { */ static HRESULT events_OnFileOk(FileDialogImpl *This) { + ULONG_PTR ctx_cookie = 0; events_client *cursor; HRESULT hr = S_OK; TRACE("%p\n", This); + if (This->user_actctx != INVALID_HANDLE_VALUE && !ActivateActCtx(This->user_actctx, &ctx_cookie)) + ERR("Failed to activate user activation context, last error %lx\n", GetLastError()); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { TRACE("Notifying %p\n", cursor); @@ -164,6 +170,9 @@ static HRESULT events_OnFileOk(FileDialogImpl *This) break; } + if (ctx_cookie && !DeactivateActCtx(0, ctx_cookie)) + ERR("Failed to deactivate user activation context, last error %lx\n", GetLastError()); + if(hr == E_NOTIMPL) hr = S_OK; @@ -172,10 +181,14 @@ static HRESULT events_OnFileOk(FileDialogImpl *This) static HRESULT events_OnFolderChanging(FileDialogImpl *This, IShellItem *folder) { + ULONG_PTR ctx_cookie = 0; events_client *cursor; HRESULT hr = S_OK; TRACE("%p (%p)\n", This, folder); + if (This->user_actctx != INVALID_HANDLE_VALUE && !ActivateActCtx(This->user_actctx, &ctx_cookie)) + ERR("Failed to activate user activation context, last error %lx\n", GetLastError()); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { TRACE("Notifying %p\n", cursor); @@ -184,6 +197,9 @@ static HRESULT events_OnFolderChanging(FileDialogImpl *This, IShellItem *folder) break; } + if (ctx_cookie && !DeactivateActCtx(0, ctx_cookie)) + ERR("Failed to deactivate user activation context, last error %lx\n", GetLastError()); + if(hr == E_NOTIMPL) hr = S_OK; @@ -192,47 +208,72 @@ static HRESULT events_OnFolderChanging(FileDialogImpl *This, IShellItem *folder) static void events_OnFolderChange(FileDialogImpl *This) { + ULONG_PTR ctx_cookie = 0; events_client *cursor; TRACE("%p\n", This); + if (This->user_actctx != INVALID_HANDLE_VALUE && !ActivateActCtx(This->user_actctx, &ctx_cookie)) + ERR("Failed to activate user activation context, last error %lx\n", GetLastError()); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { TRACE("Notifying %p\n", cursor); IFileDialogEvents_OnFolderChange(cursor->pfde, (IFileDialog*)&This->IFileDialog2_iface); } + + if (ctx_cookie && !DeactivateActCtx(0, ctx_cookie)) + ERR("Failed to deactivate user activation context, last error %lx\n", GetLastError()); } static void events_OnSelectionChange(FileDialogImpl *This) { + ULONG_PTR ctx_cookie = 0; events_client *cursor; TRACE("%p\n", This); + if (This->user_actctx != INVALID_HANDLE_VALUE && !ActivateActCtx(This->user_actctx, &ctx_cookie)) + ERR("Failed to activate user activation context, last error %lx\n", GetLastError()); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { TRACE("Notifying %p\n", cursor); IFileDialogEvents_OnSelectionChange(cursor->pfde, (IFileDialog*)&This->IFileDialog2_iface); } + + if (ctx_cookie && !DeactivateActCtx(0, ctx_cookie)) + ERR("Failed to deactivate user activation context, last error %lx\n", GetLastError()); } static void events_OnTypeChange(FileDialogImpl *This) { + ULONG_PTR ctx_cookie = 0; events_client *cursor; TRACE("%p\n", This); + if (This->user_actctx != INVALID_HANDLE_VALUE && !ActivateActCtx(This->user_actctx, &ctx_cookie)) + ERR("Failed to activate user activation context, last error %lx\n", GetLastError()); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { TRACE("Notifying %p\n", cursor); IFileDialogEvents_OnTypeChange(cursor->pfde, (IFileDialog*)&This->IFileDialog2_iface); } + + if (ctx_cookie && !DeactivateActCtx(0, ctx_cookie)) + ERR("Failed to deactivate user activation context, last error %lx\n", GetLastError()); } static HRESULT events_OnOverwrite(FileDialogImpl *This, IShellItem *shellitem) { + ULONG_PTR ctx_cookie = 0; events_client *cursor; HRESULT hr = S_OK; FDE_OVERWRITE_RESPONSE response = FDEOR_DEFAULT; TRACE("%p %p\n", This, shellitem); + if (This->user_actctx != INVALID_HANDLE_VALUE && !ActivateActCtx(This->user_actctx, &ctx_cookie)) + ERR("Failed to activate user activation context, last error %lx\n", GetLastError()); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { TRACE("Notifying %p\n", cursor); @@ -242,6 +283,9 @@ static HRESULT events_OnOverwrite(FileDialogImpl *This, IShellItem *shellitem) break; } + if (ctx_cookie && !DeactivateActCtx(0, ctx_cookie)) + ERR("Failed to deactivate user activation context, last error %lx\n", GetLastError()); + if(hr == E_NOTIMPL) hr = S_OK; @@ -274,9 +318,13 @@ static inline HRESULT get_cctrl_event(IFileDialogEvents *pfde, IFileDialogContro static HRESULT cctrl_event_OnButtonClicked(FileDialogImpl *This, DWORD ctl_id) { + ULONG_PTR ctx_cookie = 0; events_client *cursor; TRACE("%p\n", This); + if (This->user_actctx != INVALID_HANDLE_VALUE && !ActivateActCtx(This->user_actctx, &ctx_cookie)) + ERR("Failed to activate user activation context, last error %lx\n", GetLastError()); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { IFileDialogControlEvents *pfdce; @@ -288,14 +336,21 @@ static HRESULT cctrl_event_OnButtonClicked(FileDialogImpl *This, DWORD ctl_id) } } + if (ctx_cookie && !DeactivateActCtx(0, ctx_cookie)) + ERR("Failed to deactivate user activation context, last error %lx\n", GetLastError()); + return S_OK; } static HRESULT cctrl_event_OnItemSelected(FileDialogImpl *This, DWORD ctl_id, DWORD item_id) { + ULONG_PTR ctx_cookie = 0; events_client *cursor; TRACE("%p %li %li\n", This, ctl_id, item_id); + if (This->user_actctx != INVALID_HANDLE_VALUE && !ActivateActCtx(This->user_actctx, &ctx_cookie)) + ERR("Failed to activate user activation context, last error %lx\n", GetLastError()); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { IFileDialogControlEvents *pfdce; @@ -307,14 +362,21 @@ static HRESULT cctrl_event_OnItemSelected(FileDialogImpl *This, DWORD ctl_id, DW } } + if (ctx_cookie && !DeactivateActCtx(0, ctx_cookie)) + ERR("Failed to deactivate user activation context, last error %lx\n", GetLastError()); + return S_OK; } static HRESULT cctrl_event_OnCheckButtonToggled(FileDialogImpl *This, DWORD ctl_id, BOOL checked) { + ULONG_PTR ctx_cookie = 0; events_client *cursor; TRACE("%p\n", This); + if (This->user_actctx != INVALID_HANDLE_VALUE && !ActivateActCtx(This->user_actctx, &ctx_cookie)) + ERR("Failed to activate user activation context, last error %lx\n", GetLastError()); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { IFileDialogControlEvents *pfdce; @@ -326,15 +388,22 @@ static HRESULT cctrl_event_OnCheckButtonToggled(FileDialogImpl *This, DWORD ctl_ } } + if (ctx_cookie && !DeactivateActCtx(0, ctx_cookie)) + ERR("Failed to deactivate user activation context, last error %lx\n", GetLastError()); + return S_OK; } static HRESULT cctrl_event_OnControlActivating(FileDialogImpl *This, DWORD ctl_id) { + ULONG_PTR ctx_cookie = 0; events_client *cursor; TRACE("%p\n", This); + if (This->user_actctx != INVALID_HANDLE_VALUE && !ActivateActCtx(This->user_actctx, &ctx_cookie)) + ERR("Failed to activate user activation context, last error %lx\n", GetLastError()); + LIST_FOR_EACH_ENTRY(cursor, &This->events_clients, events_client, entry) { IFileDialogControlEvents *pfdce; @@ -346,6 +415,9 @@ static HRESULT cctrl_event_OnControlActivating(FileDialogImpl *This, } } + if (ctx_cookie && !DeactivateActCtx(0, ctx_cookie)) + ERR("Failed to deactivate user activation context, last error %lx\n", GetLastError()); + return S_OK; } @@ -2244,16 +2316,36 @@ static INT_PTR CALLBACK itemdlg_dlgproc(HWND hwnd, UINT umessage, WPARAM wparam, static HRESULT create_dialog(FileDialogImpl *This, HWND parent) { + ULONG_PTR ctx_cookie = 0; INT_PTR res; if (This->dlg_hwnd) return E_UNEXPECTED; + if (!GetCurrentActCtx(&This->user_actctx)) + { + ERR("Failed to get current activation context, last error %lx\n", GetLastError()); + This->user_actctx = INVALID_HANDLE_VALUE; + } + + if (!ActivateActCtx(COMDLG32_hActCtx, &ctx_cookie)) + ERR("Failed to activate activation context, last error %lx\n", GetLastError()); + SetLastError(0); res = DialogBoxParamW(COMDLG32_hInstance, MAKEINTRESOURCEW(NEWFILEOPENV3ORD), parent, itemdlg_dlgproc, (LPARAM)This); This->dlg_hwnd = NULL; + + if (ctx_cookie && !DeactivateActCtx(0, ctx_cookie)) + ERR("Failed to deactivate activation context, last error %lx\n", GetLastError()); + + if (This->user_actctx != INVALID_HANDLE_VALUE) + { + ReleaseActCtx(This->user_actctx); + This->user_actctx = INVALID_HANDLE_VALUE; + } + if(res == -1) { ERR("Failed to show dialog (LastError: %ld)\n", GetLastError()); @@ -4650,6 +4742,8 @@ static HRESULT FileDialog_constructor(IUnknown *pUnkOuter, REFIID riid, void **p return E_FAIL; } + fdimpl->user_actctx = INVALID_HANDLE_VALUE; + hr = IFileDialog2_QueryInterface(&fdimpl->IFileDialog2_iface, riid, ppv); IFileDialog2_Release(&fdimpl->IFileDialog2_iface); return hr; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2068
v10: Dropped the helper function, also added `GetLastError` in the error message. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2068#note_37316
On Wed Feb 1 19:59:12 2023 +0000, Vladislav Timonin wrote:
changed this line in [version 3 of the diff](/wine/wine/-/merge_requests/2068/diffs?diff_id=30401&start_sha=1e1350db62129a7e02268ab12d897086be9f849b#941712bdad220aba7d6798d478d2fe9a0ed5c6d0_2330_2338) Did we (or someone else) decide that `0` is always an invalid cookie?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2068#note_37329
On Thu Jun 29 13:19:27 2023 +0000, Jinoh Kang wrote:
Did we (or someone else) decide that `0` is always an invalid cookie? Don't remember, think it was us, current implementation wise (I know, shouldn't rely on that too much), it's a pointer to a heap allocation, which I assume can't have `0` as a valid value.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2068#note_37330
On Thu Jun 29 13:40:48 2023 +0000, Vladislav Timonin wrote:
Don't remember, think it was us, current implementation wise (I know, shouldn't rely on that too much), it's a pointer to a heap allocation, which I assume can't have `0` as a valid value. I'd say the heap allocation thing is a hack in Wine. It looks like Windows have sequence numbers for cookies instead. If we ever have an app that relies on `0` being returned on newly created threads or fibers, then we won't be able to fix the app without regressing, until all callsites that assume `0` is an invalid cookie is fixex.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2068#note_37334
On Thu Jun 29 13:59:27 2023 +0000, Jinoh Kang wrote:
I'd say the heap allocation thing is a hack in Wine. It looks like Windows have sequence numbers for cookies instead. If we ever have an app that relies on `0` being returned on newly created threads or fibers, then we won't be able to fix the app without regressing, until all callsites that assume `0` is an invalid cookie is fixex. The cookie checks and ERRs are not useful, you can simply assume that the calls will succeed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2068#note_37340
participants (4)
-
Alexandre Julliard (＠julliard) -
Jinoh Kang (＠iamahuman)
-
Vladislav Timonin
-
Vladislav Timonin (＠vt)