Re: [PATCH 2/3] shell32: Implement FolderItems_Item.

[Nikolay Sivov]

On 08.09.2017 8:29, Alex Henrie wrote:
> typedef struct {
>      FolderItems3 FolderItems3_iface;
>      LONG ref;
> - VARIANT dir;
> - WCHAR **item_filenames;
> - LONG item_count;
>
> } FolderItemsImpl;

If 'dir' is always a string maybe store it as such?
> static HRESULT WINAPI FolderItemsImpl_Item(FolderItems3 *iface, VARIANT index, FolderItem **ppid)
>  {
> - FIXME("(%p,%s,%p)\n", iface, debugstr_variant(&index), ppid);
> -
>
> FolderItemsImpl *This = impl_from_FolderItems(iface);
> -
>
> WCHAR path_str[MAX_PATH];
> -
>
> VARIANT path_var;
> -
>
> HRESULT ret;
> -
> -
>
> TRACE("(%p,%s,%p)\n", iface, debugstr_variant(&index), ppid);
>
> *ppid = NULL;
> - return E_NOTIMPL;
> -
> - if (!PathIsDirectoryW(V_BSTR(&This->dir)))
> -
>    return S_FALSE;

How could it not be a directory?
> - TRACE("(%p,%s,%p)\n", iface, debugstr_variant(&index), ppid);

Could add some spaces in argument list? Most of the wine code is doing
that I think.
> -
>        if (!PathCombineW(path_str, V_BSTR(&This->dir), This->item_filenames[V_I4(&index)]))
> -
>            return E_OUTOFMEMORY;

Why E_OUTOFMEMORY?
> -
>    do
> -
>    {
> -
>        if (!strcmpW(file_info.cFileName, dot) || !strcmpW(file_info.cFileName, dot_dot))
> -
>            continue;
> -
> -
>        if (This->item_count >= item_size)
> -
>        {
> -
>            item_size *= 2;
> -
>            filenames = HeapReAlloc(GetProcessHeap(), 0, This->item_filenames, item_size * sizeof(WCHAR*));
> -
>            if (!filenames)
> -
>                goto fail;
> -
>            This->item_filenames = filenames;
> -
>        }
> -
> -
>        This->item_filenames[This->item_count] = strdupW(file_info.cFileName);
> -
>        if (!This->item_filenames[This->item_count])
> -
>            goto fail;
> -
>        This->item_count++;
> -
>    }
> -
>    while (FindNextFileW(first_file, &file_info));

Do we have tests showing that it takes a snapshot like that? For example
if new file is created after FolderItems was created, is it accessible
with BSTR index for example?

[PATCH 2/3] shell32: Implement FolderItems_Item.

[Alex Henrie]

2017-09-08 2:28 GMT-06:00 Nikolay Sivov :
> On 08.09.2017 8:29, Alex Henrie wrote:
> > typedef struct {
> >      FolderItems3 FolderItems3_iface;
> >      LONG ref;
> > - VARIANT dir;
> > - WCHAR **item_filenames;
> > - LONG item_count;
> >
> > } FolderItemsImpl;
>
> If 'dir' is always a string maybe store it as such?

dir could also be a magic number, e.g. ssfCONTROLS for the control
panel. The FIXME I added in FolderItems_Constructor is the same as the
one in FolderImpl_get_Title.
> > static HRESULT WINAPI FolderItemsImpl_Item(FolderItems3 *iface, VARIANT index, FolderItem **ppid)
> >  {
> > - FIXME("(%p,%s,%p)\n", iface, debugstr_variant(&index), ppid);
> > -
> >
> > FolderItemsImpl *This = impl_from_FolderItems(iface);
> > -
> >
> > WCHAR path_str[MAX_PATH];
> > -
> >
> > VARIANT path_var;
> > -
> >
> > HRESULT ret;
> > -
> > -
> >
> > TRACE("(%p,%s,%p)\n", iface, debugstr_variant(&index), ppid);
> >
> > *ppid = NULL;
> > - return E_NOTIMPL;
> > -
> > - if (!PathIsDirectoryW(V_BSTR(&This->dir)))
> > -
> >    return S_FALSE;
>
> How could it not be a directory?

If the directory was deleted, FolderItems_Item stops working. There
are tests for this.
> > - TRACE("(%p,%s,%p)\n", iface, debugstr_variant(&index), ppid);
>
> Could add some spaces in argument list? Most of the wine code is doing
> that I think.

All of the other FolderItemsImpl functions do not have spaces in their
TRACE statements. If we were going to change this one, we would want
to change them all.
> > -
> >        if (!PathCombineW(path_str, V_BSTR(&This->dir), This->item_filenames[V_I4(&index)]))
> > -
> >            return E_OUTOFMEMORY;
>
> Why E_OUTOFMEMORY?

The exact error code doesn't really matter here, but now that you
mention it, I think ERROR_BUFFER_OVERFLOW is a better fit.
> > -
> >    do
> > -
> >    {
> > -
> >        if (!strcmpW(file_info.cFileName, dot) || !strcmpW(file_info.cFileName, dot_dot))
> > -
> >            continue;
> > -
> > -
> >        if (This->item_count >= item_size)
> > -
> >        {
> > -
> >            item_size *= 2;
> > -
> >            filenames = HeapReAlloc(GetProcessHeap(), 0, This->item_filenames, item_size * sizeof(WCHAR*));
> > -
> >            if (!filenames)
> > -
> >                goto fail;
> > -
> >            This->item_filenames = filenames;
> > -
> >        }
> > -
> > -
> >        This->item_filenames[This->item_count] = strdupW(file_info.cFileName);
> > -
> >        if (!This->item_filenames[This->item_count])
> > -
> >            goto fail;
> > -
> >        This->item_count++;
> > -
> >    }
> > -
> >    while (FindNextFileW(first_file, &file_info));
>
> Do we have tests showing that it takes a snapshot like that? For example
> if new file is created after FolderItems was created, is it accessible
> with BSTR index for example?

Yes, an integer index is for looking up an item in the snapshot, and a
string index is for looking up an item in the actual filesystem. As
the tests show, FolderItems_Item will grab a file by string index even
if the file was created after the FolderItems object was created, and
with a string index it can even grab a file in a subdirectory.
Finally, there is a test that shows that FolderItems_Item will not
grab an item by string index after it has been deleted, even if it is
still in the snapshot.

I will add comments to the tests to make it more obvious what they're
testing. Your comments also made me realize that the string index
cannot be a relative path, so I'm adding a test for that too.

-Alex