[PATCH v8 2/3] shell32: Implement FolderItems_Item.
Alex Henrie
alexhenrie24 at gmail.com
Mon Aug 22 11:09:26 CDT 2016
2016-08-22 4:16 GMT-06:00 Sebastian Lackner <sebastian at fds-team.de>:
> On 19.08.2016 11:43, Alex Henrie wrote:
>> Cc: Sebastian Lackner <sebastian at fds-team.de>
>>
>> Signed-off-by: Alex Henrie <alexhenrie24 at 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
More information about the wine-devel
mailing list