Re: [1/3] shell32: Implement SHCreateItemFromRelativeName.

[Huw Davies]

On Sat, Aug 12, 2017 at 11:42:40PM +0800, Jactry Zeng wrote:
> +HRESULT WINAPI SHCreateItemFromRelativeName(IShellItem *parent, PCWSTR name, IBindCtx *pbc,
> -
>                                        REFIID riid, void **ppv)
>
> +{
> - LPITEMIDLIST pidl_folder = NULL, pidl = NULL;
> - IShellFolder *desktop = NULL, *folder = NULL;
> - HRESULT hr;
> -
> - TRACE("(%p, %p, %p, %s, %p)\n", parent, name, pbc, debugstr_guid(riid), ppv);

Use debugstr_w() to trace 'name'.
> -
> - if(!ppv)
> -
>    return E_INVALIDARG;
> - *ppv = NULL;
> - if(!name)
> -
>    return E_INVALIDARG;
> -
> - hr = SHGetIDListFromObject((IUnknown*)parent, &pidl_folder);
> - if(hr != S_OK)
> -
>    return hr;
> -
> - hr = SHGetDesktopFolder(&desktop);
> - if(hr != S_OK)
> -
>    goto cleanup;
> -
> - if(!_ILIsDesktop(pidl_folder))
> - {
> -
>    hr = IShellFolder_BindToObject(desktop, pidl_folder, pbc, &IID_IShellFolder,
> -
>                                   (void**)&folder);

You should pass a NULL IBindCtx here.  pbc is associated with binding the name.
> -
>    if(hr != S_OK)
> -
>        goto cleanup;
> - }
> -
> - hr = IShellFolder_ParseDisplayName(folder ? folder : desktop, NULL, pbc, (LPWSTR)name,
> -
>                                   NULL, &pidl, NULL);
> - if(hr != S_OK)
> -
>    goto cleanup;
> - hr = SHCreateItemFromIDList(pidl, riid, ppv);
> -
> diff --git a/dlls/shell32/tests/shlfolder.c b/dlls/shell32/tests/shlfolder.c
> index 9e7127dfea..cbab88035a 100644
> --- a/dlls/shell32/tests/shlfolder.c
> +++ b/dlls/shell32/tests/shlfolder.c
> @@ -2519,6 +2523,86 @@ static void test_SHCreateShellItem(void)
>      else
>          win_skip("No SHCreateItemFromIDList\n");
> - /* SHCreateItemFromRelativeName */
> - if(pSHCreateItemFromRelativeName && pSHGetKnownFolderPath)
> - {
> -
>    IShellItem *shellitem_desktop = NULL;
> -
>    WCHAR *desktop_path, *displayname;
> -
>    WCHAR testfile_path[MAX_PATH] = {0};
> -
>    HANDLE file;
> -
>    LPITEMIDLIST pidl_desktop_testfile = NULL;
> -
>    int order;
> -
> -
>    ret = pSHCreateShellItem(NULL, NULL, pidl_desktop, &shellitem_desktop);
> -
>    ok(ret == S_OK, "SHCreateShellItem failed: 0x%08x.\n", ret);
> -
> -
>    shellitem = (void*)0xdeadbeef;
> -
>    ret = pSHCreateItemFromRelativeName(shellitem_desktop, NULL, NULL, &IID_IShellItem,
> -
>                                        (void**)&shellitem);
> -
>    ok(ret == E_INVALIDARG, "Expected 0x%08x but SHCreateItemFromRelativeName return: 0x%08x.\n",
> -
>       E_INVALIDARG, ret);
> -
>    ok(shellitem == NULL, "shellitem was %p.\n", shellitem);
> -
> -
>    /* Test with a non-existent file */
> -
>    shellitem = (void*)0xdeadbeef;
> -
>    ret = pSHCreateItemFromRelativeName(shellitem_desktop, testfileW, NULL, &IID_IShellItem,
> -
>                                        (void**)&shellitem);
> -
>    ok(ret == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND),
> -
>       "Expected 0x%08x but SHCreateItemFromRelativeName return: 0x%08x.\n",
> -
>       HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND), ret);
> -
>    ok(shellitem == NULL, "shellitem was %p.\n", shellitem);
> -
> -
>    /* Create a file for testing in desktop folder */
> -
>    pSHGetKnownFolderPath(&FOLDERID_Desktop, 0, NULL, &desktop_path);
> -
>    lstrcatW(testfile_path, desktop_path);
> -
>    myPathAddBackslashW(testfile_path);
> -
>    lstrcatW(testfile_path, testfileW);
> -
>    file = CreateFileW(testfile_path, GENERIC_WRITE, 0, NULL, CREATE_NEW, 0, NULL);
> -
>    ok(file != INVALID_HANDLE_VALUE, "CreateFileW failed! Last error: 0x%08x.\n", GetLastError());
> -
>    CloseHandle(file);
> -
> -
>    shellitem = (void*)0xdeadbeef;
> -
>    ret = pSHCreateItemFromRelativeName(shellitem_desktop, testfileW, NULL, &IID_IShellItem,
> -
>                                        (void**)&shellitem);
> -
>    ok(ret == S_OK, "SHCreateItemFromRelativeName failed: 0x%08x.\n", ret);
> -
>    ok(shellitem != NULL, "shellitem was %p.\n", shellitem);
> -
>    if(SUCCEEDED(ret))
> -
>    {
> -
>        ret = IShellItem_GetDisplayName(shellitem, 0, &displayname);
> -
>        ok(ret == S_OK, "IShellItem_GetDisplayName failed: 0x%08x.\n", ret);
> -
>        ok(!lstrcmpW(displayname, testfileW), "got wrong display name: %s.\n", wine_dbgstr_w(displayname));
> -
> -
>        shellitem2 = (void*)0xdeadbeef;
> -
>        ret = pSHCreateItemFromRelativeName(shellitem_desktop, testfileW, NULL, &IID_IShellItem,
> -
>                                            (void**)&shellitem2);
> -
>        ok(ret == S_OK, "SHCreateItemFromRelativeName failed: 0x%08x.\n", ret);
> -
>        ret = IShellItem_Compare(shellitem, shellitem2, 0, &order);
> -
>        ok(ret == S_OK, "IShellItem_Compare failed: 0x%08x.\n", ret);
> -
>        ok(!order, "order got wrong value: %d.\n", order);
> -
>        IShellItem_Release(shellitem2);
> -
> -
>        shellitem2 = (void*)0xdeadbeef;
> -
>        ret = IShellFolder_ParseDisplayName(desktopfolder, NULL, NULL, testfileW, NULL,
> -
>                                            &pidl_desktop_testfile, NULL);
> -
>        ok(ret == S_OK, "ParseDisplayName failed 0x%08x.\n", ret);
> -
>        ret = pSHCreateItemFromIDList(pidl_desktop_testfile, &IID_IShellItem, (void**)&shellitem2);
> -
>        ret = IShellItem_Compare(shellitem, shellitem2, 0, &order);
> -
>        ok(ret == S_OK, "IShellItem_Compare fail: 0x%08x.\n", ret);
> -
>        ok(!order, "order got wrong value: %d.\n", order);
> -
>        pILFree(pidl_desktop_testfile);
> -
>        IShellItem_Release(shellitem2);
> -
> -
>        IShellItem_Release(shellitem);
> -
>    }
> -
> -
>    DeleteFileW(testfile_path);
> -
>    CoTaskMemFree(desktop_path);
> -
>    CoTaskMemFree(displayname);

dipslayname is uninitialized if the 'if' block isn't executed.  Either
move this inside the 'if' block, or just get rid of the 'if'
condition.  We expect the condition to succeed in all cases anyway.

Huw.

[1/3] shell32: Implement SHCreateItemFromRelativeName.

[Jactry Zeng]

Thanks! I sent another try.

2017-08-14 18:41 GMT+08:00 Huw Davies :
> On Sat, Aug 12, 2017 at 11:42:40PM +0800, Jactry Zeng wrote:
> > +HRESULT WINAPI SHCreateItemFromRelativeName(IShellItem *parent, PCWSTR
>
> name, IBindCtx *pbc,
> > -
> >                                        REFIID riid, void **ppv)
> >
> > +{
> > - LPITEMIDLIST pidl_folder = NULL, pidl = NULL;
> > - IShellFolder *desktop = NULL, *folder = NULL;
> > - HRESULT hr;
> > -
> > - TRACE("(%p, %p, %p, %s, %p)\n", parent, name, pbc,
>
> debugstr_guid(riid), ppv);
>
> Use debugstr_w() to trace 'name'.
> > -
> > - if(!ppv)
> > -
> >    return E_INVALIDARG;
> > - *ppv = NULL;
> > - if(!name)
> > -
> >    return E_INVALIDARG;
> > -
> > - hr = SHGetIDListFromObject((IUnknown*)parent, &pidl_folder);
> > - if(hr != S_OK)
> > -
> >    return hr;
> > -
> > - hr = SHGetDesktopFolder(&desktop);
> > - if(hr != S_OK)
> > -
> >    goto cleanup;
> > -
> > - if(!_ILIsDesktop(pidl_folder))
> > - {
> > -
> >    hr = IShellFolder_BindToObject(desktop, pidl_folder, pbc,
>
> &IID_IShellFolder,
> > -
> >                                   (void**)&folder);
>
> You should pass a NULL IBindCtx here.  pbc is associated with binding the
> name.
> > -
> >    if(hr != S_OK)
> > -
> >        goto cleanup;
> > - }
> > -
> > - hr = IShellFolder_ParseDisplayName(folder ? folder : desktop,
>
> NULL, pbc, (LPWSTR)name,
> > -
> >                                   NULL, &pidl, NULL);
> > - if(hr != S_OK)
> > -
> >    goto cleanup;
> > - hr = SHCreateItemFromIDList(pidl, riid, ppv);
> > -
> > diff --git a/dlls/shell32/tests/shlfolder.c b/dlls/shell32/tests/
>
> shlfolder.c
> > index 9e7127dfea..cbab88035a 100644
> > --- a/dlls/shell32/tests/shlfolder.c
> > +++ b/dlls/shell32/tests/shlfolder.c
> > @@ -2519,6 +2523,86 @@ static void test_SHCreateShellItem(void)
> >      else
> >          win_skip("No SHCreateItemFromIDList\n");
> > - /* SHCreateItemFromRelativeName */
> > - if(pSHCreateItemFromRelativeName && pSHGetKnownFolderPath)
> > - {
> > -
> >    IShellItem *shellitem_desktop = NULL;
> > -
> >    WCHAR *desktop_path, *displayname;
> > -
> >    WCHAR testfile_path[MAX_PATH] = {0};
> > -
> >    HANDLE file;
> > -
> >    LPITEMIDLIST pidl_desktop_testfile = NULL;
> > -
> >    int order;
> > -
> > -
> >    ret = pSHCreateShellItem(NULL, NULL, pidl_desktop,
>
> &shellitem_desktop);
> > -
> >    ok(ret == S_OK, "SHCreateShellItem failed: 0x%08x.\n", ret);
> > -
> > -
> >    shellitem = (void*)0xdeadbeef;
> > -
> >    ret = pSHCreateItemFromRelativeName(shellitem_desktop, NULL,
>
> NULL, &IID_IShellItem,
> > -
> >                                        (void**)&shellitem);
> > -
> >    ok(ret == E_INVALIDARG, "Expected 0x%08x but
>
> SHCreateItemFromRelativeName return: 0x%08x.\n",
> > -
> >       E_INVALIDARG, ret);
> > -
> >    ok(shellitem == NULL, "shellitem was %p.\n", shellitem);
> > -
> > -
> >    /* Test with a non-existent file */
> > -
> >    shellitem = (void*)0xdeadbeef;
> > -
> >    ret = pSHCreateItemFromRelativeName(shellitem_desktop,
>
> testfileW, NULL, &IID_IShellItem,
> > -
> >                                        (void**)&shellitem);
> > -
> >    ok(ret == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND),
> > -
> >       "Expected 0x%08x but SHCreateItemFromRelativeName return:
>
> 0x%08x.\n",
> > -
> >       HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND), ret);
> > -
> >    ok(shellitem == NULL, "shellitem was %p.\n", shellitem);
> > -
> > -
> >    /* Create a file for testing in desktop folder */
> > -
> >    pSHGetKnownFolderPath(&FOLDERID_Desktop, 0, NULL,
>
> &desktop_path);
> > -
> >    lstrcatW(testfile_path, desktop_path);
> > -
> >    myPathAddBackslashW(testfile_path);
> > -
> >    lstrcatW(testfile_path, testfileW);
> > -
> >    file = CreateFileW(testfile_path, GENERIC_WRITE, 0, NULL,
>
> CREATE_NEW, 0, NULL);
> > -
> >    ok(file != INVALID_HANDLE_VALUE, "CreateFileW failed! Last
>
> error: 0x%08x.\n", GetLastError());
> > -
> >    CloseHandle(file);
> > -
> > -
> >    shellitem = (void*)0xdeadbeef;
> > -
> >    ret = pSHCreateItemFromRelativeName(shellitem_desktop,
>
> testfileW, NULL, &IID_IShellItem,
> > -
> >                                        (void**)&shellitem);
> > -
> >    ok(ret == S_OK, "SHCreateItemFromRelativeName failed:
>
> 0x%08x.\n", ret);
> > -
> >    ok(shellitem != NULL, "shellitem was %p.\n", shellitem);
> > -
> >    if(SUCCEEDED(ret))
> > -
> >    {
> > -
> >        ret = IShellItem_GetDisplayName(shellitem, 0,
>
> &displayname);
> > -
> >        ok(ret == S_OK, "IShellItem_GetDisplayName failed:
>
> 0x%08x.\n", ret);
> > -
> >        ok(!lstrcmpW(displayname, testfileW), "got wrong display
>
> name: %s.\n", wine_dbgstr_w(displayname));
> > -
> > -
> >        shellitem2 = (void*)0xdeadbeef;
> > -
> >        ret = pSHCreateItemFromRelativeName(shellitem_desktop,
>
> testfileW, NULL, &IID_IShellItem,
> > -
> >                                            (void**)&shellitem2);
> > -
> >        ok(ret == S_OK, "SHCreateItemFromRelativeName failed:
>
> 0x%08x.\n", ret);
> > -
> >        ret = IShellItem_Compare(shellitem, shellitem2, 0, &order);
> > -
> >        ok(ret == S_OK, "IShellItem_Compare failed: 0x%08x.\n",
>
> ret);
> > -
> >        ok(!order, "order got wrong value: %d.\n", order);
> > -
> >        IShellItem_Release(shellitem2);
> > -
> > -
> >        shellitem2 = (void*)0xdeadbeef;
> > -
> >        ret = IShellFolder_ParseDisplayName(desktopfolder, NULL,
>
> NULL, testfileW, NULL,
> > -
> >                                            &pidl_desktop_testfile,
>
> NULL);
> > -
> >        ok(ret == S_OK, "ParseDisplayName failed 0x%08x.\n", ret);
> > -
> >        ret = pSHCreateItemFromIDList(pidl_desktop_testfile,
>
> &IID_IShellItem, (void**)&shellitem2);
> > -
> >        ret = IShellItem_Compare(shellitem, shellitem2, 0, &order);
> > -
> >        ok(ret == S_OK, "IShellItem_Compare fail: 0x%08x.\n", ret);
> > -
> >        ok(!order, "order got wrong value: %d.\n", order);
> > -
> >        pILFree(pidl_desktop_testfile);
> > -
> >        IShellItem_Release(shellitem2);
> > -
> > -
> >        IShellItem_Release(shellitem);
> > -
> >    }
> > -
> > -
> >    DeleteFileW(testfile_path);
> > -
> >    CoTaskMemFree(desktop_path);
> > -
> >    CoTaskMemFree(displayname);
>
> dipslayname is uninitialized if the 'if' block isn't executed.  Either
> move this inside the 'if' block, or just get rid of the 'if'
> condition.  We expect the condition to succeed in all cases anyway.
>
> Huw.