Re: shell32: Partial implementation of IShellItem::BindToHandler.

[Nikolay Sivov]

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.

shell32: Partial implementation of IShellItem::BindToHandler.

[David Hedberg]

Hi,

On Mon, Aug 2, 2010 at 7:04 AM, Nikolay Sivov  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..

shell32: Partial implementation of IShellItem::BindToHandler.

[Nikolay Sivov]

On 8/2/2010 12:07, David Hedberg wrote:
> Hi,
>
> On Mon, Aug 2, 2010 at 7:04 AM, Nikolay Sivov  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).

shell32: Partial implementation of IShellItem::BindToHandler.

[David Hedberg]

On Mon, Aug 2, 2010 at 10:50 AM, Nikolay Sivov  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.

shell32: Partial implementation of IShellItem::BindToHandler.

[Nikolay Sivov]

On 8/2/2010 16:07, David Hedberg wrote:
> On Mon, Aug 2, 2010 at 10:50 AM, Nikolay Sivov  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).