[PATCH v3 0/4] MR566: shell32: Implement SHBindToFolderIDListParent.
Called by IE11. Signed-off-by: Mohamad Al-Jaf <mohamadaljaf(a)gmail.com> v3: Fix failed test. Don't know how to change the subject version number. -- v3: shell32: Reimplement SHBindToParent. shell32/tests: Test SHBindToParent last parameter behavior. shell32/tests: Add SHBindToFolderIDListParent tests. shell32: Implement SHBindToFolderIDListParent. https://gitlab.winehq.org/wine/wine/-/merge_requests/566
From: Mohamad Al-Jaf <mohamadaljaf(a)gmail.com> Called by IE11. Signed-off-by: Mohamad Al-Jaf <mohamadaljaf(a)gmail.com> --- dlls/shell32/pidl.c | 50 +++++++++++++++++++++++++++++++++++++++ dlls/shell32/shell32.spec | 1 + include/shlobj.h | 2 ++ 3 files changed, 53 insertions(+) diff --git a/dlls/shell32/pidl.c b/dlls/shell32/pidl.c index f9c91a79132..aedce020e06 100644 --- a/dlls/shell32/pidl.c +++ b/dlls/shell32/pidl.c @@ -1285,6 +1285,56 @@ BOOL WINAPI SHGetPathFromIDListEx(LPCITEMIDLIST pidl, WCHAR *path, DWORD path_si return SUCCEEDED(hr); } +/************************************************************************* + * SHBindToFolderIDListParent [SHELL32.@] + */ +HRESULT WINAPI SHBindToFolderIDListParent(IShellFolder *psf, LPCITEMIDLIST pidl, REFIID riid, LPVOID *ppv, LPCITEMIDLIST *ppidlLast) +{ + IShellFolder *psfDesktop = NULL; + HRESULT hr; + + TRACE_(shell)("%p,%p,%s\n", psf, pidl, debugstr_guid(riid)); + pdump(pidl); + + if (ppidlLast) + *ppidlLast = NULL; + + if (!pidl || !ppv) + return E_INVALIDARG; + + *ppv = NULL; + + if (!psf) + { + hr = SHGetDesktopFolder(&psfDesktop); + if (FAILED(hr)) + return hr; + psf = psfDesktop; + } + + if (_ILIsPidlSimple(pidl)) + { + /* we are on desktop level */ + hr = IShellFolder_QueryInterface(psf, riid, ppv); + } + else + { + LPITEMIDLIST pidlParent = ILClone(pidl); + ILRemoveLastID(pidlParent); + hr = IShellFolder_BindToObject(psf, pidlParent, NULL, riid, ppv); + SHFree (pidlParent); + } + + if (psfDesktop) + IShellFolder_Release(psfDesktop); + + if (SUCCEEDED(hr) && ppidlLast) + *ppidlLast = ILFindLastID(pidl); + + TRACE_(shell)("-- ppv=%p pidl=%p ret=0x%08lx\n", *ppv, (ppidlLast)?*ppidlLast:NULL, hr); + return hr; +} + /************************************************************************* * SHBindToParent [shell version 5.0] */ diff --git a/dlls/shell32/shell32.spec b/dlls/shell32/shell32.spec index f8bf8f246e8..5b812774e1f 100644 --- a/dlls/shell32/shell32.spec +++ b/dlls/shell32/shell32.spec @@ -334,6 +334,7 @@ @ stdcall SHAssocEnumHandlers(wstr long ptr) @ stdcall SHBindToObject(ptr ptr ptr ptr ptr) @ stdcall SHBindToParent(ptr ptr ptr ptr) +@ stdcall SHBindToFolderIDListParent(ptr ptr ptr ptr ptr) @ stdcall SHBrowseForFolder(ptr) SHBrowseForFolderA @ stdcall SHBrowseForFolderA(ptr) @ stdcall SHBrowseForFolderW(ptr) diff --git a/include/shlobj.h b/include/shlobj.h index feee6cd9b98..724a2b66c11 100644 --- a/include/shlobj.h +++ b/include/shlobj.h @@ -1742,6 +1742,8 @@ HRESULT WINAPI SHGetFolderPathW(HWND hwnd, int nFolder, HANDLE hToken, DWORD dwF */ HRESULT WINAPI SHGetDesktopFolder(IShellFolder * *); +HRESULT WINAPI SHBindToFolderIDListParent(IShellFolder *psf, LPCITEMIDLIST pidl, REFIID riid, LPVOID *ppv, LPCITEMIDLIST *ppidlLast); + /**************************************************************************** * SHBindToParent API */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/566
From: Mohamad Al-Jaf <mohamadaljaf(a)gmail.com> From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Mohamad Al-Jaf <mohamadaljaf(a)gmail.com> --- dlls/shell32/tests/shlfolder.c | 53 ++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/dlls/shell32/tests/shlfolder.c b/dlls/shell32/tests/shlfolder.c index 248fbd1df62..63e3b708e3e 100644 --- a/dlls/shell32/tests/shlfolder.c +++ b/dlls/shell32/tests/shlfolder.c @@ -60,6 +60,7 @@ static BOOL (WINAPI *pIsWow64Process)(HANDLE, PBOOL); static HRESULT (WINAPI *pSHCreateDefaultContextMenu)(const DEFCONTEXTMENU*,REFIID,void**); static BOOL (WINAPI *pSHGetPathFromIDListEx)(PCIDLIST_ABSOLUTE,WCHAR*,DWORD,GPFIDL_FLAGS); static HRESULT (WINAPI *pSHGetSetFolderCustomSettings)(LPSHFOLDERCUSTOMSETTINGS,PCWSTR,DWORD); +static HRESULT (WINAPI *pSHBindToFolderIDListParent)(IShellFolder*,LPCITEMIDLIST,REFIID,void **,LPCITEMIDLIST*); static WCHAR *make_wstr(const char *str) { @@ -120,6 +121,7 @@ static void init_function_pointers(void) hmod = GetModuleHandleA("shell32.dll"); #define MAKEFUNC(f) (p##f = (void*)GetProcAddress(hmod, #f)) + MAKEFUNC(SHBindToFolderIDListParent); MAKEFUNC(SHCreateItemFromIDList); MAKEFUNC(SHCreateItemFromParsingName); MAKEFUNC(SHCreateItemFromRelativeName); @@ -5497,6 +5499,56 @@ static void test_SHOpenFolderAndSelectItems(void) ILFree(folder); } +static void test_SHBindToFolderIDListParent(void) +{ + IShellFolder *psf_desktop; + LPITEMIDLIST pidl; + HRESULT hr; + WCHAR path[MAX_PATH]; + SHITEMID empty_item = { 0, { 0 } }; + LPITEMIDLIST pidl_empty = (LPITEMIDLIST)&empty_item; + LPCITEMIDLIST pidl_last; + IShellFolder *psf; + + if (!pSHBindToFolderIDListParent) + { + win_skip("SHBindToFolderIDListParent not available\n"); + return; + } + + GetTempPathW(ARRAY_SIZE(path), path); + SHGetDesktopFolder(&psf_desktop); + + hr = IShellFolder_ParseDisplayName(psf_desktop, NULL, NULL, path, NULL, &pidl, 0); + ok(hr == S_OK, "got %#lx\n", hr); + + pidl_last = NULL; + hr = pSHBindToFolderIDListParent(psf_desktop, pidl, &IID_IShellFolder, (void **)&psf, &pidl_last); + ok(hr == S_OK, "got %#lx\n", hr); + ok(pidl_last != NULL, "got %p\n", pidl_last); + IShellFolder_Release(psf); + + hr = pSHBindToFolderIDListParent(NULL, pidl_empty, &IID_IShellFolder, (void **)&psf, &pidl_last); + ok(hr == S_OK, "got %#lx\n", hr); + ok(pidl_last == pidl_empty, "got %p\n", pidl_last); + IShellFolder_Release(psf); + + hr = pSHBindToFolderIDListParent(NULL, pidl, &IID_IShellFolder, (void **)&psf, NULL); + ok(hr == S_OK, "got %#lx\n", hr); + IShellFolder_Release(psf); + + if (0) /* crashes under Windows */ + hr = pSHBindToFolderIDListParent(NULL, pidl, &IID_IShellFolder, NULL, NULL); + + ILFree(pidl); + IShellFolder_Release(psf_desktop); + + pidl_last = (LPITEMIDLIST)0xdeadbeef; + hr = pSHBindToFolderIDListParent(NULL, NULL, &IID_IShellFolder, (void **)&psf, &pidl_last); + ok(hr == E_INVALIDARG, "got %#lx\n", hr); + ok(pidl_last == NULL, "got %p\n", pidl_last); +} + START_TEST(shlfolder) { init_function_pointers(); @@ -5504,6 +5556,7 @@ START_TEST(shlfolder) CO_E_NOTINITIALIZED for malformed directory names */ OleInitialize(NULL); + test_SHBindToFolderIDListParent(); test_ParseDisplayName(); test_SHParseDisplayName(); test_BindToObject(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/566
From: Mohamad Al-Jaf <mohamadaljaf(a)gmail.com> --- dlls/shell32/tests/shlfolder.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/shell32/tests/shlfolder.c b/dlls/shell32/tests/shlfolder.c index 63e3b708e3e..306909cc6a0 100644 --- a/dlls/shell32/tests/shlfolder.c +++ b/dlls/shell32/tests/shlfolder.c @@ -775,9 +775,11 @@ static void test_GetDisplayName(void) ok (!lstrcmpiW(wszTestFile, wszTestFile2), "SHGetPathFromIDListW returns incorrect path!\n"); /* SHBindToParent fails, if called with a NULL PIDL. */ + pidlLast = (LPITEMIDLIST)0xdeadbeef; hr = SHBindToParent(NULL, &IID_IShellFolder, (void **)&psfPersonal, &pidlLast); ok (hr == E_INVALIDARG || broken(hr == E_OUTOFMEMORY) /* XP */, "SHBindToParent(NULL) should fail! hr = %08lx\n", hr); + todo_wine ok(pidlLast == NULL, "got %p\n", pidlLast); /* But it succeeds with an empty PIDL. */ hr = SHBindToParent(pidlEmpty, &IID_IShellFolder, (void **)&psfPersonal, &pidlLast); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/566
From: Mohamad Al-Jaf <mohamadaljaf(a)gmail.com> --- dlls/shell32/pidl.c | 38 +--------------------------------- dlls/shell32/tests/shlfolder.c | 2 +- 2 files changed, 2 insertions(+), 38 deletions(-) diff --git a/dlls/shell32/pidl.c b/dlls/shell32/pidl.c index aedce020e06..73141c50a0b 100644 --- a/dlls/shell32/pidl.c +++ b/dlls/shell32/pidl.c @@ -1340,43 +1340,7 @@ HRESULT WINAPI SHBindToFolderIDListParent(IShellFolder *psf, LPCITEMIDLIST pidl, */ HRESULT WINAPI SHBindToParent(LPCITEMIDLIST pidl, REFIID riid, LPVOID *ppv, LPCITEMIDLIST *ppidlLast) { - IShellFolder * psfDesktop; - HRESULT hr=E_FAIL; - - TRACE_(shell)("pidl=%p\n", pidl); - pdump(pidl); - - if (!pidl || !ppv) - return E_INVALIDARG; - - *ppv = NULL; - if (ppidlLast) - *ppidlLast = NULL; - - hr = SHGetDesktopFolder(&psfDesktop); - if (FAILED(hr)) - return hr; - - if (_ILIsPidlSimple(pidl)) - { - /* we are on desktop level */ - hr = IShellFolder_QueryInterface(psfDesktop, riid, ppv); - } - else - { - LPITEMIDLIST pidlParent = ILClone(pidl); - ILRemoveLastID(pidlParent); - hr = IShellFolder_BindToObject(psfDesktop, pidlParent, NULL, riid, ppv); - SHFree (pidlParent); - } - - IShellFolder_Release(psfDesktop); - - if (SUCCEEDED(hr) && ppidlLast) - *ppidlLast = ILFindLastID(pidl); - - TRACE_(shell)("-- psf=%p pidl=%p ret=0x%08lx\n", *ppv, (ppidlLast)?*ppidlLast:NULL, hr); - return hr; + return SHBindToFolderIDListParent(NULL, pidl, riid, ppv, ppidlLast); } HRESULT WINAPI SHBindToObject(IShellFolder *psf, LPCITEMIDLIST pidl, IBindCtx *pbc, REFIID riid, void **ppv) diff --git a/dlls/shell32/tests/shlfolder.c b/dlls/shell32/tests/shlfolder.c index 306909cc6a0..c9e79aa9b36 100644 --- a/dlls/shell32/tests/shlfolder.c +++ b/dlls/shell32/tests/shlfolder.c @@ -779,7 +779,7 @@ static void test_GetDisplayName(void) hr = SHBindToParent(NULL, &IID_IShellFolder, (void **)&psfPersonal, &pidlLast); ok (hr == E_INVALIDARG || broken(hr == E_OUTOFMEMORY) /* XP */, "SHBindToParent(NULL) should fail! hr = %08lx\n", hr); - todo_wine ok(pidlLast == NULL, "got %p\n", pidlLast); + ok(pidlLast == NULL, "got %p\n", pidlLast); /* But it succeeds with an empty PIDL. */ hr = SHBindToParent(pidlEmpty, &IID_IShellFolder, (void **)&psfPersonal, &pidlLast); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/566
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=125625 Your paranoid android. === w1064v1809 (32 bit report) === shell32: shlfolder.c:4994: Test failed: RMDIR: expected notification type 10, got: 40000 === w1064 (32 bit report) === shell32: shlfolder.c:4994: Test failed: RMDIR: expected notification type 10, got: 40000 === w1064v1809 (64 bit report) === shell32: shlfolder.c:4994: Test failed: RMDIR: expected notification type 10, got: 40000 === w1064_adm (64 bit report) === shell32: shlfolder.c:4994: Test failed: MKDIR: expected notification type 8, got: 40000 === w1064_tsign (64 bit report) === shell32: shlfolder.c:5011: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 2) === debian11 (32 bit report) === ntoskrnl.exe: driver_pnp.c:737: Test failed: expected IRP_MN_REMOVE_DEVICE === debian11 (32 bit zh:CN report) === shell32: autocomplete.c:614: Test failed: Expected (null), got L"ab" autocomplete.c:637: Test failed: Expected (null), got L"www.ax"
On Fri Oct 28 19:30:58 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 10/26/22 20:34, Mohamad Al-Jaf wrote:
+ if (ppidlLast) + *ppidlLast = NULL; + if (!pidl || !ppv) return E_INVALIDARG; - + *ppv = NULL; - if (ppidlLast) - *ppidlLast = NULL; Why are these changes being made? At least they shouldn't be part of this patch.
Yes, you're right. Those changes were made in order to conform to the behavior of Windows and pass the tests, specifically the last test which checks if ppidlLast is NULL after the function returns E_INVALIDARG. Though, this only applied to SHBindToFolderIDListParent. There was no test for SHBindToParent for this behavior. I've added one now.
I'm still not really sure what Alexandre meant by merging the two code change patches. Merging the commits just made it unclear IMO. I've added a separate commit for reimplementing SHBindToParent as well as a separate test commit. Please let me know if this is okay. I can always just start a new merge request for reimplementing SHBindToParent if necessary. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/566#note_12706
On Wed Nov 2 01:06:36 2022 +0000, Mohamad Al-Jaf wrote:
Yes, you're right. Those changes were made in order to conform to the behavior of Windows and pass the tests, specifically the last test which checks if ppidlLast is NULL after the function returns E_INVALIDARG. Though, this only applied to SHBindToFolderIDListParent. There was no test for SHBindToParent for this behavior. I've added one now. I'm still not really sure what Alexandre meant by merging the two code change patches. Merging the commits just made it unclear IMO. I've added a separate commit for reimplementing SHBindToParent as well as a separate test commit. Please let me know if this is okay. I can always just start a new merge request for reimplementing SHBindToParent if necessary. The point is that merging the two changes makes it clear how SHBindToFolderIDListParent() differs from SHBindToParent(). I.e. in this case, the implementation is the same, except that you're using a custom shell folder instead of the desktop. Changing around the error handling muddies that, and if it's necessary then it should be its own patch.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/566#note_13048
On Thu Nov 3 19:36:26 2022 +0000, Zebediah Figura wrote:
The point is that merging the two changes makes it clear how SHBindToFolderIDListParent() differs from SHBindToParent(). I.e. in this case, the implementation is the same, except that you're using a custom shell folder instead of the desktop. Changing around the error handling muddies that, and if it's necessary then it should be its own patch. I'm not sure what is meant by merging the two changes. Does it mean to merge the two commits, 'Implement SHBindToFolderIDListParent' and 'Reimplement SHBindToParent'? I did that with the last version and it didn't make it any clearer. What exactly should I do?
Well it does appear to be necessary to change the error handling of SHBindToParent since it doesn't conform to the behavior of Windows. But it is it's own patch as seen in the commits tab, "shell32/tests: Test SHBindToParent last parameter behavior." and "shell32: Reimplement SHBindToParent.". I don't know what else to do. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/566#note_13103
participants (4)
-
Marvin -
Mohamad Al-Jaf -
Mohamad Al-Jaf (＠maljaf)
-
Zebediah Figura (＠zfigura)