Re: shell32: Partial implementation of IShellItem::BindToHandler.
On 8/2/2010 02:36, David Hedberg wrote:
+static HRESULT ShellItem_get_shellfolder(ShellItem *This, IBindCtx *pbc, IShellFolder **ppsf) +{ + IShellFolder *desktop; + HRESULT ret; + + ret = SHGetDesktopFolder(&desktop); + if (SUCCEEDED(ret)) + { + if (_ILIsDesktop(This->pidl)) + { + *ppsf = desktop; + IShellFolder_AddRef(*ppsf); + } + else + { + ret = IShellFolder_BindToObject(desktop, This->pidl, pbc,&IID_IShellFolder, (void**)ppsf); + } + + IShellFolder_Release(desktop); + } + + return ret; +} +
I don't think this explicit check for desktop folder is needed here, as I understand ::BindToObject() it should handle this case internally.
Hi, On Mon, Aug 2, 2010 at 7:04 AM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
On 8/2/2010 02:36, David Hedberg wrote:
+static HRESULT ShellItem_get_shellfolder(ShellItem *This, IBindCtx *pbc, IShellFolder **ppsf) +{ + IShellFolder *desktop; + HRESULT ret; + + ret = SHGetDesktopFolder(&desktop); + if (SUCCEEDED(ret)) + { + if (_ILIsDesktop(This->pidl)) + { + *ppsf = desktop; + IShellFolder_AddRef(*ppsf); + } + else + { + ret = IShellFolder_BindToObject(desktop, This->pidl, pbc,&IID_IShellFolder, (void**)ppsf); + } + + IShellFolder_Release(desktop); + } + + return ret; +} +
I don't think this explicit check for desktop folder is needed here, as I understand ::BindToObject() it should handle this case internally.
I wrote a quick testcase and tested it with wine, Windows XP and Windows 7. BindToObject() does not seem to handle this case in any of these, instead they all return E_INVALIDARG. Wine appears to accomplish this with the check for "!pidlComplete->mkid.cb" in shlfolder.c:Shell32_BindToChild. So as far as I can tell, the explicit check here seems to be needed..
On 8/2/2010 12:07, David Hedberg wrote:
Hi,
On Mon, Aug 2, 2010 at 7:04 AM, Nikolay Sivov<nsivov(a)codeweavers.com> wrote:
On 8/2/2010 02:36, David Hedberg wrote:
+static HRESULT ShellItem_get_shellfolder(ShellItem *This, IBindCtx *pbc, IShellFolder **ppsf) +{ + IShellFolder *desktop; + HRESULT ret; + + ret = SHGetDesktopFolder(&desktop); + if (SUCCEEDED(ret)) + { + if (_ILIsDesktop(This->pidl)) + { + *ppsf = desktop; + IShellFolder_AddRef(*ppsf); + } + else + { + ret = IShellFolder_BindToObject(desktop, This->pidl, pbc,&IID_IShellFolder, (void**)ppsf); + } + + IShellFolder_Release(desktop); + } + + return ret; +} +
I don't think this explicit check for desktop folder is needed here, as I understand ::BindToObject() it should handle this case internally.
I wrote a quick testcase and tested it with wine, Windows XP and Windows 7. BindToObject() does not seem to handle this case in any of these, instead they all return E_INVALIDARG. Wine appears to accomplish this with the check for "!pidlComplete->mkid.cb" in shlfolder.c:Shell32_BindToChild. So as far as I can tell, the explicit check here seems to be needed.. Ok, this test is present already cause Desktop list is an empty list. So please change this test for mkid.cb magic value to _ILIsEmpty() in BindToObject() and all other occurrences too (InitializeTreeView() for example).
On Mon, Aug 2, 2010 at 10:50 AM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Ok, this test is present already cause Desktop list is an empty list. So please change this test for mkid.cb magic value to _ILIsEmpty() in BindToObject() and all other occurrences too (InitializeTreeView() for example).
Is something like the attached patch acceptable? I have only changed the checks where it would not introduce an extra check for NULL, and I have also not touched debughlp.c. Should I be more radical and also change places that only checks pidl->mkid.cb? Also, the check in _ILIsDesktop() seems a bit counter-intuitive to me (interpreting it as "pidl && (pidl->mkid.cb ? FALSE : TRUE)" is closer to what you would expect as the current version also considers NULL pidl's to be the desktop (or empty)), but I assume that the current meaning is the intended one.
On 8/2/2010 16:07, David Hedberg wrote:
On Mon, Aug 2, 2010 at 10:50 AM, Nikolay Sivov<nsivov(a)codeweavers.com> wrote:
Ok, this test is present already cause Desktop list is an empty list. So please change this test for mkid.cb magic value to _ILIsEmpty() in BindToObject() and all other occurrences too (InitializeTreeView() for example).
Is something like the attached patch acceptable? I have only changed the checks where it would not introduce an extra check for NULL, and I have also not touched debughlp.c. Should I be more radical and also change places that only checks pidl->mkid.cb? It's fine I think, for only pidl->mkid.cb I'd say no for now, cause I'm no sure about it. All this check thing needs to be revised and simplified probably. Also, the check in _ILIsDesktop() seems a bit counter-intuitive to me (interpreting it as "pidl&& (pidl->mkid.cb ? FALSE : TRUE)" is closer to what you would expect as the current version also considers NULL pidl's to be the desktop (or empty)), but I assume that the current meaning is the intended one. I'm not aware about assumptions in code that NULL pidl means desktop. For now your change doesn't change functionality, but only clarifies things. Again, occurrences of "NULL means desktop" should be found and tested if possible, but later (if you'll feel brave enough after your gsoc project end, for example).
participants (2)
-
David Hedberg -
Nikolay Sivov