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

[Sebastian Lackner]

On 19.08.2016 11:43, Alex Henrie wrote:
> Cc: Sebastian Lackner <sebastian(a)fds-team.de>
>
> Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com>
> ---
>  dlls/shell32/shelldispatch.c       | 133 +++++++++++++++++++++++++++++++++++--
>  dlls/shell32/tests/shelldispatch.c |  12 ----
>  2 files changed, 127 insertions(+), 18 deletions(-)
>
> diff --git a/dlls/shell32/shelldispatch.c b/dlls/shell32/shelldispatch.c
> index ac79302..a456ad4 100644
> --- a/dlls/shell32/shelldispatch.c
> +++ b/dlls/shell32/shelldispatch.c
> @@ -68,6 +68,9 @@ typedef struct {
>  typedef struct {
>      FolderItems3 FolderItems3_iface;
>      LONG ref;
> +    VARIANT dir;
> +    BSTR *item_paths;
> +    LONG item_count;
>  } FolderItemsImpl;
>
> typedef struct {
> @@ -989,11 +992,18 @@ static ULONG WINAPI FolderItemsImpl_Release(FolderItems3 *iface)
>  {
>      FolderItemsImpl *This = impl_from_FolderItems(iface);
>      ULONG ref = InterlockedDecrement(&This->ref);
> +    int i;
>
> TRACE("(%p), new refcount=%i\n", iface, ref);
>
> if (!ref)
> +    {
> +        VariantClear(&This->dir);
> +        for (i = 0; i < This->item_count; i++)
> +            SysFreeString(This->item_paths[i]);
> +        HeapFree(GetProcessHeap(), 0, This->item_paths);
>          HeapFree(GetProcessHeap(), 0, This);
> +    }
>      return ref;
>  }
>
> @@ -1079,10 +1089,47 @@ static HRESULT WINAPI FolderItemsImpl_get_Parent(FolderItems3 *iface, IDispatch
>
> 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);
> +    VARIANT path;
> +    int i;
> +
> +    TRACE("(%p,%s,%p)\n", iface, debugstr_variant(&index), ppid);
>
> *ppid = NULL;
> -    return E_NOTIMPL;
> +
> +    switch (V_VT(&index))
> +    {
> +        case VT_I2:
> +            VariantChangeType(&index, &index, 0, VT_I4);
> +            /* fall through */
> +        case VT_I4:
> +            if (V_I4(&index) < 0 || V_I4(&index) >= This->item_count)
> +                return S_FALSE;
> +
> +            V_VT(&path) = VT_BSTR;
> +            V_BSTR(&path) = This->item_paths[V_I4(&index)];
> +            return FolderItem_Constructor(&path, ppid);
> +
> +        case VT_BSTR:
> +            for (i = 0; i < This->item_count; i++)
> +            {
> +                if (!lstrcmpW(V_BSTR(&index), PathFindFileNameW(This->item_paths[i])))

Usually strcmpW is preferred in Wine code. Its better to do NULL pointer
checks explicitly.
> +                {
> +                    V_VT(&path) = VT_BSTR;
> +                    V_BSTR(&path) = This->item_paths[i];
> +                    return FolderItem_Constructor(&path, ppid);
> +                }
> +            }
> +            return S_FALSE;
> +
> +        case VT_ERROR:
> +            V_VT(&path) = VT_BSTR;
> +            V_BSTR(&path) = V_BSTR(&This->dir);
> +            return FolderItem_Constructor(&path, ppid);

You can also pass &This->dir directly to the constructor.
> +
> +        default:
> +            return E_NOTIMPL;
> +    }
>  }
>
> static HRESULT WINAPI FolderItemsImpl__NewEnum(FolderItems3 *iface, IUnknown **ppunk)
> @@ -1139,21 +1186,93 @@ static const FolderItems3Vtbl FolderItemsImpl_Vtbl = {
>      FolderItemsImpl_get_Verbs
>  };
>
> -static HRESULT FolderItems_Constructor(FolderItems **ppfi)
> +static HRESULT FolderItems_Constructor(VARIANT *dir, FolderItems **ppfi)
>  {
> +    static const WCHAR backslash_star[] = {'\\','*',0};
> +    static const WCHAR dot[] = {'.',0};
> +    static const WCHAR dot_dot[] = {'.','.',0};
>      FolderItemsImpl *This;
> +    WCHAR path[MAX_PATH + 2];
> +    HANDLE first_file;
> +    WIN32_FIND_DATAW file_info;
> +    BSTR *paths;
> +    HRESULT ret;
>
> TRACE("\n");

Not really an issue, but you could add a trace for the function arguments here.
> *ppfi = NULL;
>
> +    if (V_VT(dir) == VT_I4)
> +    {
> +        FIXME("special folder constants are not supported\n");
> +        return E_NOTIMPL;
> +    }
> +
>      This = HeapAlloc(GetProcessHeap(), 0, sizeof(FolderItemsImpl));
>      if (!This) return E_OUTOFMEMORY;
>      This->FolderItems3_iface.lpVtbl = &FolderItemsImpl_Vtbl;
>      This->ref = 1;
>
> +    VariantInit(&This->dir);
> +    ret = VariantCopy(&This->dir, dir);
> +    if (FAILED(ret))
> +    {
> +        HeapFree(GetProcessHeap(), 0, This);
> +        return E_OUTOFMEMORY;
> +    }
> +
> +    This->item_paths = HeapAlloc(GetProcessHeap(), 0, sizeof(BSTR));
> +    if (!This->item_paths)
> +    {
> +        VariantClear(&This->dir);
> +        HeapFree(GetProcessHeap(), 0, This);
> +        return E_OUTOFMEMORY;
> +    }
> +    This->item_count = 0;
> +
> +    lstrcpyW(path, V_BSTR(dir));
> +    lstrcatW(path, backslash_star);
> +    first_file = FindFirstFileW(path, &file_info);
> +    if (first_file != INVALID_HANDLE_VALUE)
> +    {
> +        do
> +        {
> +            if (!lstrcmpW(file_info.cFileName, dot) || !lstrcmpW(file_info.cFileName, dot_dot))
> +                continue;

Similar to above, strcmpW is usually preferred.
> +
> +            if (!PathCombineW(path, V_BSTR(dir), file_info.cFileName))
> +            {
> +                ret = E_OUTOFMEMORY;
> +                goto fail;
> +            }

I think it makes more sense to store the relative paths, there is no
need to prepend the directory name in advance. FolderItemsImpl_Item
also expects a relative path for example.
> +
> +            paths = HeapReAlloc(GetProcessHeap(), 0, This->item_paths, (This->item_count + 1) * sizeof(BSTR));

It seems more reasonable to increase the buffer size in bigger chunks.
Its better to have a few entries overhead than to do frequent reallocs.
> +            if (!paths)
> +            {
> +                ret = E_OUTOFMEMORY;
> +                goto fail;
> +            }
> +            This->item_paths = paths;
> +
> +            This->item_paths[This->item_count] = SysAllocString(path);
> +            if (!This->item_paths[This->item_count])
> +            {
> +                ret = E_OUTOFMEMORY;
> +                goto fail;
> +            }
> +            This->item_count++;
> +        }
> +        while (FindNextFileW(first_file, &file_info));
> +
> +        FindClose(first_file);
> +    }
> +
>      *ppfi = (FolderItems*)&This->FolderItems3_iface;
> -    return S_OK;
> +    return ret;

It is better to return S_OK explicitly because !FAILED(ret) could still be
any other value >= 0, like S_FALSE for example.
> +
> +fail:

You are leaking the FindFirstFile handle here.
> +    FolderItems3_Release(&This->FolderItems3_iface);
> +    return ret;
>  }
>
> static HRESULT WINAPI FolderImpl_QueryInterface(Folder3 *iface, REFIID riid,
> @@ -1308,9 +1427,11 @@ static HRESULT WINAPI FolderImpl_get_ParentFolder(Folder3 *iface, Folder **ppsf)
>
> static HRESULT WINAPI FolderImpl_Items(Folder3 *iface, FolderItems **ppid)
>  {
> -    FIXME("(%p,%p)\n", iface, ppid);
> +    FolderImpl *This = impl_from_Folder(iface);
> +
> +    TRACE("(%p,%p)\n", iface, ppid);
>
> -    return FolderItems_Constructor(ppid);
> +    return FolderItems_Constructor(&This->dir, ppid);
>  }
>
> static HRESULT WINAPI FolderImpl_ParseName(Folder3 *iface, BSTR name, FolderItem **item)
> diff --git a/dlls/shell32/tests/shelldispatch.c b/dlls/shell32/tests/shelldispatch.c
> index 6715794..dc4fda4 100644
> --- a/dlls/shell32/tests/shelldispatch.c
> +++ b/dlls/shell32/tests/shelldispatch.c
> @@ -390,7 +390,6 @@ todo_wine
>          r = FolderItems_Item(items, var, NULL);
>
> r = FolderItems_Item(items, var, &item);
> -todo_wine
>      ok(r == S_FALSE, "expected S_FALSE, got %08x\n", r);
>      ok(!item, "item is not null\n");
>
> @@ -470,14 +469,11 @@ todo_wine
>          r = FolderItems_Item(items, var, &item);
>          if (i == VT_I2 || i == VT_I4 || i == VT_ERROR)
>          {
> -todo_wine
>              ok(r == S_OK, "type %d: expected S_OK, got %08x\n", i, r);
> -todo_wine
>              ok(!!item, "item is null\n");
>          }
>          else if (i == VT_BSTR)
>          {
> -todo_wine
>              ok(r == S_FALSE, "type %d: expected S_FALSE, got %08x\n", i, r);
>              ok(!item, "item is not null\n");
>          }
> @@ -495,11 +491,8 @@ todo_wine
>          V_VT(&var) = VT_ERROR;
>          V_ERROR(&var) = i;
>          r = FolderItems_Item(items, var, &item);
> -todo_wine
>          ok(r == S_OK, "expected S_OK, got %08x\n", r);
> -todo_wine
>          ok(!!item, "item is null\n");
> -        if (!item) continue;
>
> bstr = NULL;
>          r = FolderItem_get_Path(item, &bstr);
> @@ -518,7 +511,6 @@ todo_wine
>      V_I4(&var) = -1;
>      item = (FolderItem*)0xdeadbeef;
>      r = FolderItems_Item(items, var, &item);
> -todo_wine
>      ok(r == S_FALSE, "expected S_FALSE, got %08x\n", r);
>      ok(!item, "item is not null\n");
>
> @@ -541,11 +533,8 @@ todo_wine
>
> item = NULL;
>              r = FolderItems_Item(items, var, &item);
> -todo_wine
>              ok(r == S_OK, "file_defs[%d], %d: FolderItems::Item failed: %08x\n", i, j, r);
> -todo_wine
>              ok(!!item, "file_defs[%d], %d: item is null\n", i, j);
> -            if (!item) continue;
>
> item2 = NULL;
>              r = FolderItems_Item(items, var, &item2);
> @@ -572,7 +561,6 @@ todo_wine
>      V_I4(&var) = sizeof(file_defs)/sizeof(file_defs[0]);
>      item = (FolderItem*)0xdeadbeef;
>      r = FolderItems_Item(items, var, &item);
> -todo_wine
>      ok(r == S_FALSE, "expected S_FALSE, got %08x\n", r);
>      ok(!item, "item is not null\n");

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

[Alex Henrie]

2016-08-22 4:16 GMT-06:00 Sebastian Lackner <sebastian(a)fds-team.de>:
> On 19.08.2016 11:43, Alex Henrie wrote:
> > Cc: Sebastian Lackner <sebastian(a)fds-team.de>
> >
> > Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com>
> > ---
> >  dlls/shell32/shelldispatch.c       | 133 +++++++++++++++++++++++++++++++++++--
> >  dlls/shell32/tests/shelldispatch.c |  12 ----
> >  2 files changed, 127 insertions(+), 18 deletions(-)
> >
> > diff --git a/dlls/shell32/shelldispatch.c b/dlls/shell32/shelldispatch.c
> > index ac79302..a456ad4 100644
> > --- a/dlls/shell32/shelldispatch.c
> > +++ b/dlls/shell32/shelldispatch.c
> > @@ -68,6 +68,9 @@ typedef struct {
> >  typedef struct {
> >      FolderItems3 FolderItems3_iface;
> >      LONG ref;
> > +    VARIANT dir;
> > +    BSTR *item_paths;
> > +    LONG item_count;
> >  } FolderItemsImpl;
> >
> > typedef struct {
> > @@ -989,11 +992,18 @@ static ULONG WINAPI FolderItemsImpl_Release(FolderItems3 *iface)
> >  {
> >      FolderItemsImpl *This = impl_from_FolderItems(iface);
> >      ULONG ref = InterlockedDecrement(&This->ref);
> > +    int i;
> >
> > TRACE("(%p), new refcount=%i\n", iface, ref);
> >
> > if (!ref)
> > +    {
> > +        VariantClear(&This->dir);
> > +        for (i = 0; i < This->item_count; i++)
> > +            SysFreeString(This->item_paths[i]);
> > +        HeapFree(GetProcessHeap(), 0, This->item_paths);
> >          HeapFree(GetProcessHeap(), 0, This);
> > +    }
> >      return ref;
> >  }
> >
> > @@ -1079,10 +1089,47 @@ static HRESULT WINAPI FolderItemsImpl_get_Parent(FolderItems3 *iface, IDispatch
> >
> > 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);
> > +    VARIANT path;
> > +    int i;
> > +
> > +    TRACE("(%p,%s,%p)\n", iface, debugstr_variant(&index), ppid);
> >
> > *ppid = NULL;
> > -    return E_NOTIMPL;
> > +
> > +    switch (V_VT(&index))
> > +    {
> > +        case VT_I2:
> > +            VariantChangeType(&index, &index, 0, VT_I4);
> > +            /* fall through */
> > +        case VT_I4:
> > +            if (V_I4(&index) < 0 || V_I4(&index) >= This->item_count)
> > +                return S_FALSE;
> > +
> > +            V_VT(&path) = VT_BSTR;
> > +            V_BSTR(&path) = This->item_paths[V_I4(&index)];
> > +            return FolderItem_Constructor(&path, ppid);
> > +
> > +        case VT_BSTR:
> > +            for (i = 0; i < This->item_count; i++)
> > +            {
> > +                if (!lstrcmpW(V_BSTR(&index), PathFindFileNameW(This->item_paths[i])))
>
> Usually strcmpW is preferred in Wine code. Its better to do NULL pointer
> checks explicitly.

Okay.
> > +                {
> > +                    V_VT(&path) = VT_BSTR;
> > +                    V_BSTR(&path) = This->item_paths[i];
> > +                    return FolderItem_Constructor(&path, ppid);
> > +                }
> > +            }
> > +            return S_FALSE;
> > +
> > +        case VT_ERROR:
> > +            V_VT(&path) = VT_BSTR;
> > +            V_BSTR(&path) = V_BSTR(&This->dir);
> > +            return FolderItem_Constructor(&path, ppid);
>
> You can also pass &This->dir directly to the constructor.

Good point.
> > +
> > +        default:
> > +            return E_NOTIMPL;
> > +    }
> >  }
> >
> > static HRESULT WINAPI FolderItemsImpl__NewEnum(FolderItems3 *iface, IUnknown **ppunk)
> > @@ -1139,21 +1186,93 @@ static const FolderItems3Vtbl FolderItemsImpl_Vtbl = {
> >      FolderItemsImpl_get_Verbs
> >  };
> >
> > -static HRESULT FolderItems_Constructor(FolderItems **ppfi)
> > +static HRESULT FolderItems_Constructor(VARIANT *dir, FolderItems **ppfi)
> >  {
> > +    static const WCHAR backslash_star[] = {'\\','*',0};
> > +    static const WCHAR dot[] = {'.',0};
> > +    static const WCHAR dot_dot[] = {'.','.',0};
> >      FolderItemsImpl *This;
> > +    WCHAR path[MAX_PATH + 2];
> > +    HANDLE first_file;
> > +    WIN32_FIND_DATAW file_info;
> > +    BSTR *paths;
> > +    HRESULT ret;
> >
> > TRACE("\n");
>
> Not really an issue, but you could add a trace for the function arguments here.

Okay.
> > *ppfi = NULL;
> >
> > +    if (V_VT(dir) == VT_I4)
> > +    {
> > +        FIXME("special folder constants are not supported\n");
> > +        return E_NOTIMPL;
> > +    }
> > +
> >      This = HeapAlloc(GetProcessHeap(), 0, sizeof(FolderItemsImpl));
> >      if (!This) return E_OUTOFMEMORY;
> >      This->FolderItems3_iface.lpVtbl = &FolderItemsImpl_Vtbl;
> >      This->ref = 1;
> >
> > +    VariantInit(&This->dir);
> > +    ret = VariantCopy(&This->dir, dir);
> > +    if (FAILED(ret))
> > +    {
> > +        HeapFree(GetProcessHeap(), 0, This);
> > +        return E_OUTOFMEMORY;
> > +    }
> > +
> > +    This->item_paths = HeapAlloc(GetProcessHeap(), 0, sizeof(BSTR));
> > +    if (!This->item_paths)
> > +    {
> > +        VariantClear(&This->dir);
> > +        HeapFree(GetProcessHeap(), 0, This);
> > +        return E_OUTOFMEMORY;
> > +    }
> > +    This->item_count = 0;
> > +
> > +    lstrcpyW(path, V_BSTR(dir));
> > +    lstrcatW(path, backslash_star);
> > +    first_file = FindFirstFileW(path, &file_info);
> > +    if (first_file != INVALID_HANDLE_VALUE)
> > +    {
> > +        do
> > +        {
> > +            if (!lstrcmpW(file_info.cFileName, dot) || !lstrcmpW(file_info.cFileName, dot_dot))
> > +                continue;
>
> Similar to above, strcmpW is usually preferred.

Okay.
> > +
> > +            if (!PathCombineW(path, V_BSTR(dir), file_info.cFileName))
> > +            {
> > +                ret = E_OUTOFMEMORY;
> > +                goto fail;
> > +            }
>
> I think it makes more sense to store the relative paths, there is no
> need to prepend the directory name in advance. FolderItemsImpl_Item
> also expects a relative path for example.

Are you sure? In the common case, FolderItemsImpl_Item receives a
VT_I4 index and can simply pass the saved path from the array to
FolderItem_Constructor. If we save filenames instead, we'll have to
call PathCombineW, SysAllocString, and SysFreeString in both the VT_I4
and the VT_BSTR cases.
> > +
> > +            paths = HeapReAlloc(GetProcessHeap(), 0, This->item_paths, (This->item_count + 1) * sizeof(BSTR));
>
> It seems more reasonable to increase the buffer size in bigger chunks.
> Its better to have a few entries overhead than to do frequent reallocs.

How about allocating 128 string pointers (1 kibibyte) at a time? The
code would be:

if (This->item_count % 128)
{
    paths = HeapReAlloc(GetProcessHeap(), 0, This->item_paths,
                        (1 + This->item_count / 128) * 128 * sizeof(BSTR));
    if (!paths)
        goto fail;
    This->item_paths = paths;
}
> > +            if (!paths)
> > +            {
> > +                ret = E_OUTOFMEMORY;
> > +                goto fail;
> > +            }
> > +            This->item_paths = paths;
> > +
> > +            This->item_paths[This->item_count] = SysAllocString(path);
> > +            if (!This->item_paths[This->item_count])
> > +            {
> > +                ret = E_OUTOFMEMORY;
> > +                goto fail;
> > +            }
> > +            This->item_count++;
> > +        }
> > +        while (FindNextFileW(first_file, &file_info));
> > +
> > +        FindClose(first_file);
> > +    }
> > +
> >      *ppfi = (FolderItems*)&This->FolderItems3_iface;
> > -    return S_OK;
> > +    return ret;
>
> It is better to return S_OK explicitly because !FAILED(ret) could still be
> any other value >= 0, like S_FALSE for example.

Okay.
> > +
> > +fail:
>
> You are leaking the FindFirstFile handle here.

Good catch.
> > +    FolderItems3_Release(&This->FolderItems3_iface);
> > +    return ret;
> >  }
> >
> > static HRESULT WINAPI FolderImpl_QueryInterface(Folder3 *iface, REFIID riid,
> > @@ -1308,9 +1427,11 @@ static HRESULT WINAPI FolderImpl_get_ParentFolder(Folder3 *iface, Folder **ppsf)
> >
> > static HRESULT WINAPI FolderImpl_Items(Folder3 *iface, FolderItems **ppid)
> >  {
> > -    FIXME("(%p,%p)\n", iface, ppid);
> > +    FolderImpl *This = impl_from_Folder(iface);
> > +
> > +    TRACE("(%p,%p)\n", iface, ppid);
> >
> > -    return FolderItems_Constructor(ppid);
> > +    return FolderItems_Constructor(&This->dir, ppid);
> >  }
> >
> > static HRESULT WINAPI FolderImpl_ParseName(Folder3 *iface, BSTR name, FolderItem **item)
> > diff --git a/dlls/shell32/tests/shelldispatch.c b/dlls/shell32/tests/shelldispatch.c
> > index 6715794..dc4fda4 100644
> > --- a/dlls/shell32/tests/shelldispatch.c
> > +++ b/dlls/shell32/tests/shelldispatch.c
> > @@ -390,7 +390,6 @@ todo_wine
> >          r = FolderItems_Item(items, var, NULL);
> >
> > r = FolderItems_Item(items, var, &item);
> > -todo_wine
> >      ok(r == S_FALSE, "expected S_FALSE, got %08x\n", r);
> >      ok(!item, "item is not null\n");
> >
> > @@ -470,14 +469,11 @@ todo_wine
> >          r = FolderItems_Item(items, var, &item);
> >          if (i == VT_I2 || i == VT_I4 || i == VT_ERROR)
> >          {
> > -todo_wine
> >              ok(r == S_OK, "type %d: expected S_OK, got %08x\n", i, r);
> > -todo_wine
> >              ok(!!item, "item is null\n");
> >          }
> >          else if (i == VT_BSTR)
> >          {
> > -todo_wine
> >              ok(r == S_FALSE, "type %d: expected S_FALSE, got %08x\n", i, r);
> >              ok(!item, "item is not null\n");
> >          }
> > @@ -495,11 +491,8 @@ todo_wine
> >          V_VT(&var) = VT_ERROR;
> >          V_ERROR(&var) = i;
> >          r = FolderItems_Item(items, var, &item);
> > -todo_wine
> >          ok(r == S_OK, "expected S_OK, got %08x\n", r);
> > -todo_wine
> >          ok(!!item, "item is null\n");
> > -        if (!item) continue;
> >
> > bstr = NULL;
> >          r = FolderItem_get_Path(item, &bstr);
> > @@ -518,7 +511,6 @@ todo_wine
> >      V_I4(&var) = -1;
> >      item = (FolderItem*)0xdeadbeef;
> >      r = FolderItems_Item(items, var, &item);
> > -todo_wine
> >      ok(r == S_FALSE, "expected S_FALSE, got %08x\n", r);
> >      ok(!item, "item is not null\n");
> >
> > @@ -541,11 +533,8 @@ todo_wine
> >
> > item = NULL;
> >              r = FolderItems_Item(items, var, &item);
> > -todo_wine
> >              ok(r == S_OK, "file_defs[%d], %d: FolderItems::Item failed: %08x\n", i, j, r);
> > -todo_wine
> >              ok(!!item, "file_defs[%d], %d: item is null\n", i, j);
> > -            if (!item) continue;
> >
> > item2 = NULL;
> >              r = FolderItems_Item(items, var, &item2);
> > @@ -572,7 +561,6 @@ todo_wine
> >      V_I4(&var) = sizeof(file_defs)/sizeof(file_defs[0]);
> >      item = (FolderItem*)0xdeadbeef;
> >      r = FolderItems_Item(items, var, &item);
> > -todo_wine
> >      ok(r == S_FALSE, "expected S_FALSE, got %08x\n", r);
> >      ok(!item, "item is not null\n");

Thanks again.

-Alex

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

[Sebastian Lackner]

On 22.08.2016 18:09, Alex Henrie wrote:
> > > +
> > > +            if (!PathCombineW(path, V_BSTR(dir), file_info.cFileName))
> > > +            {
> > > +                ret = E_OUTOFMEMORY;
> > > +                goto fail;
> > > +            }
> >
> > I think it makes more sense to store the relative paths, there is no
> > need to prepend the directory name in advance. FolderItemsImpl_Item
> > also expects a relative path for example.
>
> Are you sure? In the common case, FolderItemsImpl_Item receives a
> VT_I4 index and can simply pass the saved path from the array to
> FolderItem_Constructor. If we save filenames instead, we'll have to
> call PathCombineW, SysAllocString, and SysFreeString in both the VT_I4
> and the VT_BSTR cases.

I definitely think this is better. It is wasting a lot of memory when
the full path is computed in advance. Also, it would be more consistent
with the existing calls to FolderItem_Constructor(). If you are concerned
about having the code twice, you could either implement it in the
constructor itself (and simplify the existing code) or by using a common
implementation for both VT_I4 and VT_BSTR. Something like this should work
(pseudo-code):

--- snip ---
    switch (V_VT(&index))
    {
        case VT_I2:
            i = V_I2(&index);
            break;

        case VT_I4:
            i = V_I4(&index);
            break;

        case VT_BSTR:
            [...]
            break;

        case VT_ERROR:
            return FolderItem_Constructor(&This->dir, ppid);

        default:
            return E_NOTIMPL;
    }

    if (i < 0 || i >= This->item_count)
        return S_FALSE;

    [... call constructor ...]
--- snip ---

Please note that after this change it might no longer be necessary to
use SysAllocString for the internal variables.
> > > +
> > > +            paths = HeapReAlloc(GetProcessHeap(), 0, This->item_paths, (This->item_count + 1) * sizeof(BSTR));
> >
> > It seems more reasonable to increase the buffer size in bigger chunks.
> > Its better to have a few entries overhead than to do frequent reallocs.
>
> How about allocating 128 string pointers (1 kibibyte) at a time? The
> code would be:
>
> if (This->item_count % 128)
> {
>     paths = HeapReAlloc(GetProcessHeap(), 0, This->item_paths,
>                         (1 + This->item_count / 128) * 128 * sizeof(BSTR));
>     if (!paths)
>         goto fail;
>     This->item_paths = paths;
> }

It is probably fine, but actually it might be easier to just add a "item_size"
variable corresponding to the current amount of allocated memory. For example:

--- snip ---
    item_size = 128;
    This->item_paths = HeapAlloc(GetProcessHeap(), 0, item_size * sizeof(...));
    [...]
            if (This->item_count >= item_size)
            {
                paths = HeapReAlloc(GetProcessHeap(), 0, This->item_paths, (item_size + 128) * sizeof(...));
                if (!paths)
                {
                    ret = E_OUTOFMEMORY;
                    goto fail;
                }
                This->item_paths = paths;
                item_size += 128;
            }
--- snip ---

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

[Alex Henrie]

Sebastian,

I incorporated all of the changes that you suggested, please take a
look at v9. But if you are still busy working with Aric on device
support (very exciting!), I can wait.

-Alex