On 19.08.2016 11:43, Alex Henrie wrote:
Cc: Sebastian Lackner sebastian@fds-team.de
Signed-off-by: Alex Henrie alexhenrie24@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");
2016-08-22 4:16 GMT-06:00 Sebastian Lackner sebastian@fds-team.de:
On 19.08.2016 11:43, Alex Henrie wrote:
Cc: Sebastian Lackner sebastian@fds-team.de
Signed-off-by: Alex Henrie alexhenrie24@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
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 ---
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
-
Alex Henrie -
Sebastian Lackner