June 2018 - Wine-Devel - list.winehq.org

[PATCH 3/7] d3d12/tests: Add option to use selected adapter.

by Józef Kucia

From: Józef Kucia <jkucia(a)codeweavers.com> Signed-off-by: Józef Kucia <jkucia(a)codeweavers.com> --- dlls/d3d12/tests/d3d12.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/dlls/d3d12/tests/d3d12.c b/dlls/d3d12/tests/d3d12.c index c8f17f314ce1..37f5c5147814 100644 --- a/dlls/d3d12/tests/d3d12.c +++ b/dlls/d3d12/tests/d3d12.c @@ -70,11 +70,47 @@ static void set_viewport(D3D12_VIEWPORT *vp, float x, float y, vp->MaxDepth = max_depth; } +static BOOL use_warp_adapter; +static unsigned int use_adapter_idx; + +static IDXGIAdapter *create_adapter(void) +{ + IDXGIFactory4 *factory; + IDXGIAdapter *adapter; + HRESULT hr; + + if (!use_warp_adapter && !use_adapter_idx) + return NULL; + + hr = CreateDXGIFactory2(0, &IID_IDXGIFactory4, (void **)&factory); + ok(hr == S_OK, "Failed to create factory, hr %#x.\n", hr); + + adapter = NULL; + if (use_warp_adapter) + { + hr = IDXGIFactory4_EnumWarpAdapter(factory, &IID_IDXGIAdapter, (void **)&adapter); + } + else + { + hr = IDXGIFactory4_EnumAdapters(factory, use_adapter_idx, &adapter); + } + IDXGIFactory4_Release(factory); + if (FAILED(hr)) + trace("Failed to get adapter, hr %#x.\n", hr); + return adapter; +} + static ID3D12Device *create_device(void) { + IDXGIAdapter *adapter; ID3D12Device *device; + HRESULT hr; - if (FAILED(D3D12CreateDevice(NULL, D3D_FEATURE_LEVEL_11_0, &IID_ID3D12Device, (void **)&device))) + adapter = create_adapter(); + hr = D3D12CreateDevice((IUnknown *)adapter, D3D_FEATURE_LEVEL_11_0, &IID_ID3D12Device, (void **)&device); + if (adapter) + IDXGIAdapter_Release(adapter); + if (FAILED(hr)) return NULL; return device; @@ -746,6 +782,18 @@ static void test_swapchain_draw(void) START_TEST(d3d12) { + unsigned int argc, i; + char **argv; + + argc = winetest_get_mainargs(&argv); + for (i = 2; i < argc; ++i) + { + if (!strcmp(argv[i], "--warp")) + use_warp_adapter = TRUE; + else if (!strcmp(argv[i], "--adapter") && i + 1 < argc) + use_adapter_idx = atoi(argv[++i]); + } + test_interfaces(); test_draw(); test_swapchain_draw(); -- 2.16.4

June 11, 2018

[PATCH 2/7] wined3d: Remove hardware level detection hacks.

by Józef Kucia

From: Józef Kucia <jkucia(a)codeweavers.com> Core contexts are enabled by default since 0db4d1c251d293333e2721a78d6156008a90ff6f. Signed-off-by: Józef Kucia <jkucia(a)codeweavers.com> --- dlls/wined3d/directx.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/dlls/wined3d/directx.c b/dlls/wined3d/directx.c index afb97184e396..9bc9c31e0a43 100644 --- a/dlls/wined3d/directx.c +++ b/dlls/wined3d/directx.c @@ -1857,22 +1857,15 @@ static enum wined3d_pci_vendor wined3d_guess_card_vendor(const char *gl_vendor_s return HW_VENDOR_NVIDIA; } -static enum wined3d_d3d_level d3d_level_from_caps(const struct shader_caps *shader_caps, const struct fragment_caps *fragment_caps, DWORD glsl_version) +static enum wined3d_d3d_level d3d_level_from_caps(const struct shader_caps *shader_caps, + const struct fragment_caps *fragment_caps) { if (shader_caps->vs_version >= 5) return WINED3D_D3D_LEVEL_11; if (shader_caps->vs_version == 4) return WINED3D_D3D_LEVEL_10; if (shader_caps->vs_version == 3) - { - /* wined3d with default settings at the moment doesn't expose SM4+ on - * Mesa drivers. */ - if (glsl_version >= MAKEDWORD_VERSION(4, 30)) - return WINED3D_D3D_LEVEL_11; - if (glsl_version >= MAKEDWORD_VERSION(1, 30)) - return WINED3D_D3D_LEVEL_10; return WINED3D_D3D_LEVEL_9_SM3; - } if (shader_caps->vs_version == 2) return WINED3D_D3D_LEVEL_9_SM2; if (shader_caps->vs_version == 1) @@ -2562,9 +2555,9 @@ static enum wined3d_pci_device wined3d_guess_card(const struct shader_caps *shad * memory behind our backs if really needed. Note that the amount of video * memory can be overruled using a registry setting. */ - unsigned int i; - enum wined3d_d3d_level d3d_level = d3d_level_from_caps(shader_caps, fragment_caps, glsl_version); + enum wined3d_d3d_level d3d_level = d3d_level_from_caps(shader_caps, fragment_caps); enum wined3d_pci_device device; + unsigned int i; for (i = 0; i < ARRAY_SIZE(card_vendor_table); ++i) { -- 2.16.4

June 11, 2018

[PATCH 1/7] wined3d: Execute all CS commands before changing swapchain window.

by Józef Kucia

From: Józef Kucia <jkucia(a)codeweavers.com> Fixes a race condition. For bug 44557. Signed-off-by: Józef Kucia <jkucia(a)codeweavers.com> --- dlls/wined3d/swapchain.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dlls/wined3d/swapchain.c b/dlls/wined3d/swapchain.c index 7a3f1ff95e90..9df40abc70ce 100644 --- a/dlls/wined3d/swapchain.c +++ b/dlls/wined3d/swapchain.c @@ -135,6 +135,8 @@ void * CDECL wined3d_swapchain_get_parent(const struct wined3d_swapchain *swapch void CDECL wined3d_swapchain_set_window(struct wined3d_swapchain *swapchain, HWND window) { + struct wined3d_device *device = swapchain->device; + if (!window) window = swapchain->device_window; if (window == swapchain->win_handle) @@ -142,6 +144,9 @@ void CDECL wined3d_swapchain_set_window(struct wined3d_swapchain *swapchain, HWN TRACE("Setting swapchain %p window from %p to %p.\n", swapchain, swapchain->win_handle, window); + + device->cs->ops->finish(device->cs, WINED3D_CS_QUEUE_DEFAULT); + swapchain->win_handle = window; } -- 2.16.4

June 11, 2018

[PATCH 5/5] mstask/tests: Add more tests for IEnumWorkItems interface.

by Dmitry Timoshkov

Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/mstask/tests/task_scheduler.c | 75 +++++++++++++++++++++++++++++++++++--- 1 file changed, 70 insertions(+), 5 deletions(-) diff --git a/dlls/mstask/tests/task_scheduler.c b/dlls/mstask/tests/task_scheduler.c index d4766382ec..f61022575d 100644 --- a/dlls/mstask/tests/task_scheduler.c +++ b/dlls/mstask/tests/task_scheduler.c @@ -212,23 +212,88 @@ done: static void test_Enum(void) { + static const WCHAR Task1[] = { 'w','i','n','e','t','a','s','k','1',0 }; ITaskScheduler *scheduler; + ITask *task; IEnumWorkItems *tasks; + WCHAR **names; + ULONG fetched; HRESULT hr; hr = CoCreateInstance(&CLSID_CTaskScheduler, NULL, CLSCTX_INPROC_SERVER, &IID_ITaskScheduler, (void **)&scheduler); - ok(hr == S_OK, "got 0x%08x\n", hr); + ok(hr == S_OK, "got %#x\n", hr); + + /* cleanup after previous runs */ + ITaskScheduler_Delete(scheduler, Task1); + + hr = ITaskScheduler_NewWorkItem(scheduler, Task1, &CLSID_CTask, &IID_ITask, (IUnknown **)&task); + ok(hr == S_OK, "got %#x\n", hr); + hr = ITaskScheduler_AddWorkItem(scheduler, Task1, (IScheduledWorkItem *)task); + ok(hr == S_OK, "got %#x\n", hr); + + ITask_Release(task); -if (0) { /* crashes on win2k */ hr = ITaskScheduler_Enum(scheduler, NULL); - ok(hr == E_INVALIDARG, "got 0x%08x\n", hr); -} + ok(hr == E_INVALIDARG, "got %#x\n", hr); hr = ITaskScheduler_Enum(scheduler, &tasks); - ok(hr == S_OK, "got 0x%08x\n", hr); + ok(hr == S_OK, "got %#x\n", hr); + + names = (void *)0xdeadbeef; + fetched = 0xdeadbeef; + hr = IEnumWorkItems_Next(tasks, 0, &names, &fetched); + ok(hr == E_INVALIDARG, "got %#x\n", hr); + ok(names == (void *)0xdeadbeef, "got %p\n", names); + ok(fetched == 0xdeadbeef, "got %#x\n", fetched); + + hr = IEnumWorkItems_Next(tasks, 1, NULL, NULL); + ok(hr == E_INVALIDARG, "got %#x\n", hr); + + names = NULL; + hr = IEnumWorkItems_Next(tasks, 1, &names, NULL); + ok(hr == S_OK, "got %#x\n", hr); + ok(names != NULL, "got NULL\n"); + ok(names[0] != NULL, "got NULL\n"); + CoTaskMemFree(names[0]); + CoTaskMemFree(names); + + names = (void *)0xdeadbeef; + hr = IEnumWorkItems_Next(tasks, 2, &names, NULL); + ok(hr == E_INVALIDARG, "got %#x\n", hr); + ok(names == (void *)0xdeadbeef, "got %p\n", names); + + hr = IEnumWorkItems_Reset(tasks); + ok(hr == S_OK, "got %#x\n", hr); + + names = NULL; + fetched = 0xdeadbeef; + hr = IEnumWorkItems_Next(tasks, 1, &names, &fetched); + ok(hr == S_OK, "got %#x\n", hr); + ok(names != NULL, "got NULL\n"); + ok(names[0] != NULL, "got NULL\n"); + ok(fetched == 1, "got %u\n", fetched); + CoTaskMemFree(names[0]); + CoTaskMemFree(names); + + while (IEnumWorkItems_Skip(tasks, 1) == S_OK) + /* do nothing*/; + + hr = IEnumWorkItems_Skip(tasks, 1); + ok(hr == S_FALSE, "got %#x\n", hr); + + names = (void *)0xdeadbeef; + fetched = 0xdeadbeef; + hr = IEnumWorkItems_Next(tasks, 1, &names, &fetched); + ok(hr == S_FALSE, "got %#x\n", hr); + ok(names == NULL, "got %p\n", names); + ok(fetched == 0, "got %u\n", fetched); + IEnumWorkItems_Release(tasks); + hr = ITaskScheduler_Delete(scheduler, Task1); + ok(hr == S_OK, "got %#x\n", hr); + ITaskScheduler_Release(scheduler); } -- 2.16.3

June 11, 2018

[PATCH 4/5] mstask: Implement IEnumWorkItems::Skip().

by Dmitry Timoshkov

Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/mstask/task_scheduler.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/dlls/mstask/task_scheduler.c b/dlls/mstask/task_scheduler.c index fcade04db0..7998b3a50e 100644 --- a/dlls/mstask/task_scheduler.c +++ b/dlls/mstask/task_scheduler.c @@ -209,9 +209,17 @@ static HRESULT WINAPI EnumWorkItems_Next(IEnumWorkItems *iface, ULONG count, LPW static HRESULT WINAPI EnumWorkItems_Skip(IEnumWorkItems *iface, ULONG count) { - EnumWorkItemsImpl *This = impl_from_IEnumWorkItems(iface); - FIXME("(%p)->(%u): stub\n", This, count); - return E_NOTIMPL; + LPWSTR *names; + ULONG fetched; + HRESULT hr; + + TRACE("(%p)->(%u)\n", iface, count); + + hr = EnumWorkItems_Next(iface, count, &names, &fetched); + if (SUCCEEDED(hr)) + free_list(names, fetched); + + return hr; } static HRESULT WINAPI EnumWorkItems_Reset(IEnumWorkItems *iface) -- 2.16.3

June 11, 2018

[PATCH 3/5] mstask: Implement IEnumWorkItems::Reset().

by Dmitry Timoshkov

Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/mstask/task_scheduler.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/dlls/mstask/task_scheduler.c b/dlls/mstask/task_scheduler.c index f867537fa1..fcade04db0 100644 --- a/dlls/mstask/task_scheduler.c +++ b/dlls/mstask/task_scheduler.c @@ -217,8 +217,16 @@ static HRESULT WINAPI EnumWorkItems_Skip(IEnumWorkItems *iface, ULONG count) static HRESULT WINAPI EnumWorkItems_Reset(IEnumWorkItems *iface) { EnumWorkItemsImpl *This = impl_from_IEnumWorkItems(iface); - FIXME("(%p): stub\n", This); - return E_NOTIMPL; + + TRACE("(%p)\n", This); + + if (This->handle != INVALID_HANDLE_VALUE) + { + FindClose(This->handle); + This->handle = INVALID_HANDLE_VALUE; + } + + return S_OK; } static HRESULT WINAPI EnumWorkItems_Clone(IEnumWorkItems *iface, IEnumWorkItems **cloned) -- 2.16.3

June 11, 2018

[PATCH 2/5] mstask: Implement IEnumWorkItems::Next(). (v2)

by Dmitry Timoshkov

v2: double the allocated size. Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/mstask/task_scheduler.c | 101 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 99 insertions(+), 2 deletions(-) diff --git a/dlls/mstask/task_scheduler.c b/dlls/mstask/task_scheduler.c index 416a3218d0..f867537fa1 100644 --- a/dlls/mstask/task_scheduler.c +++ b/dlls/mstask/task_scheduler.c @@ -43,6 +43,7 @@ typedef struct { IEnumWorkItems IEnumWorkItems_iface; LONG ref; + HANDLE handle; } EnumWorkItemsImpl; static inline TaskSchedulerImpl *impl_from_ITaskScheduler(ITaskScheduler *iface) @@ -97,6 +98,8 @@ static ULONG WINAPI EnumWorkItems_Release(IEnumWorkItems *iface) if (ref == 0) { + if (This->handle != INVALID_HANDLE_VALUE) + FindClose(This->handle); heap_free(This); InterlockedDecrement(&dll_ref); } @@ -104,11 +107,104 @@ static ULONG WINAPI EnumWorkItems_Release(IEnumWorkItems *iface) return ref; } +static void free_list(LPWSTR *list, LONG count) +{ + LONG i; + + for (i = 0; i < count; i++) + CoTaskMemFree(list[i]); + + CoTaskMemFree(list); +} + +static inline BOOL is_file(const WIN32_FIND_DATAW *data) +{ + return !(data->dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY); +} + static HRESULT WINAPI EnumWorkItems_Next(IEnumWorkItems *iface, ULONG count, LPWSTR **names, ULONG *fetched) { + static const WCHAR tasksW[] = { '\\','T','a','s','k','s','\\','*',0 }; EnumWorkItemsImpl *This = impl_from_IEnumWorkItems(iface); - FIXME("(%p)->(%u %p %p): stub\n", This, count, names, fetched); - return E_NOTIMPL; + WCHAR path[MAX_PATH]; + WIN32_FIND_DATAW data; + ULONG enumerated, allocated, dummy; + LPWSTR *list; + HRESULT hr = S_FALSE; + + TRACE("(%p)->(%u %p %p)\n", This, count, names, fetched); + + if (!count || !names || (!fetched && count > 1)) return E_INVALIDARG; + + if (!fetched) fetched = &dummy; + + *names = NULL; + *fetched = 0; + enumerated = 0; + list = NULL; + + allocated = 64; + list = CoTaskMemAlloc(allocated * sizeof(list[0])); + if (!list) return E_OUTOFMEMORY; + + if (This->handle == INVALID_HANDLE_VALUE) + { + GetWindowsDirectoryW(path, MAX_PATH); + lstrcatW(path, tasksW); + This->handle = FindFirstFileW(path, &data); + if (This->handle == INVALID_HANDLE_VALUE) + return S_FALSE; + } + else + { + if (!FindNextFileW(This->handle, &data)) + return S_FALSE; + } + + do + { + if (is_file(&data)) + { + if (enumerated >= allocated) + { + LPWSTR *new_list; + allocated *= 2; + new_list = CoTaskMemRealloc(list, allocated * sizeof(list[0])); + if (!new_list) + { + hr = E_OUTOFMEMORY; + break; + } + list = new_list; + } + + list[enumerated] = CoTaskMemAlloc((lstrlenW(data.cFileName) + 1) * sizeof(WCHAR)); + if (!list[enumerated]) + { + hr = E_OUTOFMEMORY; + break; + } + + lstrcpyW(list[enumerated], data.cFileName); + enumerated++; + + if (enumerated >= count) + { + hr = S_OK; + break; + } + } + } while (FindNextFileW(This->handle, &data)); + + if (FAILED(hr)) + free_list(list, enumerated); + else + { + *fetched = enumerated; + *names = list; + } + + return hr; } static HRESULT WINAPI EnumWorkItems_Skip(IEnumWorkItems *iface, ULONG count) @@ -154,6 +250,7 @@ static HRESULT create_task_enum(IEnumWorkItems **ret) tasks->IEnumWorkItems_iface.lpVtbl = &EnumWorkItemsVtbl; tasks->ref = 1; + tasks->handle = INVALID_HANDLE_VALUE; *ret = &tasks->IEnumWorkItems_iface; InterlockedIncrement(&dll_ref); -- 2.16.3

June 11, 2018

[PATCH 1/5] mstask: Implement ITask::GetExitCode().

by Dmitry Timoshkov

Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/mstask/task.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/dlls/mstask/task.c b/dlls/mstask/task.c index 266029a58e..7499826937 100644 --- a/dlls/mstask/task.c +++ b/dlls/mstask/task.c @@ -685,8 +685,14 @@ static HRESULT WINAPI MSTASK_ITask_GetExitCode(ITask *iface, DWORD *exit_code) TRACE("(%p, %p)\n", iface, exit_code); + if (This->status == SCHED_S_TASK_NOT_SCHEDULED) + { + *exit_code = 0; + return SCHED_S_TASK_HAS_NOT_RUN; + } + *exit_code = This->exit_code; - return SCHED_S_TASK_HAS_NOT_RUN; /* FIXME */ + return S_OK; } static HRESULT WINAPI MSTASK_ITask_SetComment(ITask *iface, LPCWSTR comment) -- 2.16.3

June 11, 2018

Re: [PATCH 2/2] user32: Fix returned class name for other processes windows.

by Alexandre Julliard

Nikolay Sivov <nsivov(a)codeweavers.com> writes: > @@ -2049,6 +2049,23 @@ DECL_HANDLER(set_window_owner) > } > > > +/* get class name for a window handle */ > +DECL_HANDLER(get_class_name) > +{ > + struct window *win = get_window( req->handle ); > + WCHAR name[256]; > + atom_t atom; > + int len; > + > + if (!win) return; > + > + atom = win->class ? get_base_class_atom( win->class ) : DESKTOP_ATOM; > + len = get_atom_string(atom, name, 256); > + > + set_reply_data( name, min( len, get_reply_max_size() )); > +} This should go into class.c., though I'd suggest returning the atom in the existing set_class_info request instead. -- Alexandre Julliard julliard(a)winehq.org

June 11, 2018

Re: [PATCH v2] shellpath.c: Fix creation of HOME directory symbolic links

by Huw Davies

On Sat, Jun 09, 2018 at 12:45:23AM +0100, Bob Wya wrote: > > > On 8 June 2018 at 12:52, Huw Davies <huw(a)codeweavers.com> wrote: > On Fri, Jun 08, 2018 at 10:58:23AM +0100, Rob Walker wrote: > > Fixes: https://bugs.winehq.org/show_bug.cgi?id=41668 > > > > The Shell Folders, that Wine symlinks to the user's HOME directory, > are > > only re-created on each Wine boot if they: do not pre-exist or are > broken > > symbolic links. > > > > Handling the DESKTOP Shell Folder is a special case as this is > created twice > > (during a standard boot), early in the Wine boot process. This is > handled by > > 2 separate processes, loading shell32.dll, in sequence. This makes > it hard to determine > > if the DESKTOP folder was created before the Wine boot (either from > a previous Wine boot > > or by the end user). > > > > The final (implemented) solution determines the exact start time of > the Wine boot process > > (using the current system time and tick-count). If the DESKTOP > directory was last written > > after this Wine boot time, we can assume that "we" (Wine) > automatically created the > > directory. Only in this instance do we attempt to set a symlink to > the DESKTOP directory > > (subdirectory of HOME). > > There's still too much going on here. > > Do you really need to refactor the code to make your change? > If not, then just send in the change to the current code. > If you need to refactor, then do the refactoring first > (I could imagine taking 3-4 patches to do the refactoring[1]) > then make the change as a final patch in the series. > > The file-time / boot-time thing seems hacky, I'm not exactly > sure what you're trying do to, but this doesn't sound right. > Hopefully that will become clearer as you tidy things up. > > Huw. > > [1] For example move the creation of My Pictures/My Videos/etc > first, then move My Documents and finally Desktop. These > final two are special cases in the current code, we'd need > to see that in any new code. > > Sorry I forgot to update the commit message! I'd updated the code (in the v2 > patch), for handling the > special case of the User profile Desktop directory, without testing file times > / boot times (the latter > didn't work as GetTickTime64 was returning the host system uptime anyway). > > ------------------------------------------------------------------------------- > ------------------------------------------------- > > I have a number of concerns with the existing implementation of > _SHCreateSymbolicLinks(): > > 1) The function is far too long, in it's present form - it's currently 165 > lines long! > > 2) The comments should be far more terse and precise. > > 3) Using infinite while (1) loops to de-mark code blocks only serves to obscure > the functionality of the code. I'd agree, the current function isn't ideal. But, unfortunately, you can't just rip it out and start again. Things need to be done incrementally. > 4) > > 4487- /* '$HOME' doesn't exist. Create 'My Pictures', 'My Videos' and > 'My Music' subdirs > 4488- * in '%USERPROFILE%\\My Documents' or fail silently if they > already exist. */ > 4489- pszHome = NULL; > 4490- strcpy(szPersonalTarget, pszPersonal); > 4491- for (i = 0; i < ARRAY_SIZE(aidsMyStuff); i++) { > 4492- strcpy(szMyStuffTarget, szPersonalTarget); > 4493- if (_SHAppendToUnixPath(szMyStuffTarget, MAKEINTRESOURCEW > (aidsMyStuff[i]))) > 4494- mkdir(szMyStuffTarget, 0777); > 4495- } > > Windows Vista (and newer) do not nest the User profile directories. Since Wine > is targeting Windows 7 by default - this > legacy behaviour should be removed. (As an end user, who used to use Windows > XP, this was simply an annoying default > layout anyway.) > > 5) > > 4447- /* '$HOME/My Documents' exists. Create 'My Pictures', > 4448- * 'My Videos' and 'My Music' subfolders or fail silently > if > 4449- * they already exist. > 4450- */ > 4451- for (i = 0; i < ARRAY_SIZE(aidsMyStuff); i++) > 4452- { > 4453- strcpy(szMyStuffTarget, szPersonalTarget); > 4454- if (_SHAppendToUnixPath(szMyStuffTarget, > MAKEINTRESOURCEW(aidsMyStuff[i]))) > 4455- mkdir(szMyStuffTarget, 0777); > 4456- } > 4457- break; > > Ditto as per (4). This is an obsolete User Profile directory layout (pre- > Vista). > > Also if "${HOME}/My Documents" exists then this block of code will start > creating subdirectories in this > folder. I don't think that Wine should be making the rather arbitrary > assumption that a Wine user wants > their "${HOME}" directory spammed with new subdirectories. > > I often read online, comments from Wine users, complaining about this sort of > behaviour... > > 6) > > 4543- /* Last but not least, the Desktop folder */ > 4544- if (pszHome) > 4545- strcpy(szDesktopTarget, pszHome); > 4546- else > 4547- strcpy(szDesktopTarget, pszPersonal); > 4548- heap_free(pszPersonal); > 4549- > 4550- xdg_desktop_dir = xdg_results ? xdg_results[num - 1] : NULL; > 4551- if (xdg_desktop_dir || > 4552- (_SHAppendToUnixPath(szDesktopTarget, DesktopW) && > 4553- !stat(szDesktopTarget, &statFolder) && S_ISDIR > (statFolder.st_mode))) > 4554- { > 4555- hr = SHGetFolderPathW(NULL, > CSIDL_DESKTOPDIRECTORY|CSIDL_FLAG_CREATE, NULL, > 4556- SHGFP_TYPE_DEFAULT, wszTempPath); > 4557- if (SUCCEEDED(hr) && (pszDesktop = wine_get_unix_file_name > (wszTempPath))) > 4558- { > 4559- remove(pszDesktop); > 4560- if (xdg_desktop_dir) > 4561- symlink(xdg_desktop_dir, pszDesktop); > 4562- else > 4563- symlink(szDesktopTarget, pszDesktop); > 4564- heap_free(pszDesktop); > 4565- } > 4566- } > > This code uses pszPersonal as a fallback target for when HOME is unset. > This will be the equivalent of: > > "${WINEPREFIX}/dosdevices/c:/users/${USER}/My Documents" > > Say the XDG_DESKTOP_DIR check fails. Then Wine tries to find the directory: > > "${WINEPREFIX}/dosdevices/c:/users/${USER}/My Documents/Desktop" > > as a symlink target. Which never succeeds because Wine never creates this > directory... > > This extra path appears to be redundant (HOME unset / XDG_DESKTOP_DIR unset). So points 4 - 6 sound like they each deserve a separate patch. > ------------------------------------------------------------------------------- > ------------------------------------------------- > > I can of course leave the variable names unchanged and the function name. > I was "told off" for not using snake case on a previous Wine commit - so I > assumed > I had to follow this standard if I wanted to refactor existing code. :-) In this case you're adding to existing code so it becomes a bit more flexible. Generally though keeping the patch as small as possible should be your aim. The reviewer needs to be able to understand what's going on; including unrelated or needless changes makes that much harder. > I'd like to tidy this function up and make it more readable (including the > comments). > Which I can do in staged patches, as you suggested. > > But obviously I'd like to feel that we are both on the "same page" about the > changes > I discussed (above). Before I email in a v3 staged patchset. Go ahead and send in those patches, then people can give you meaningful feedback about the actual implementation details. Huw.

June 11, 2018