On 8 June 2018 at 12:52, Huw Davies huw@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.
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).
--------------------------------------------------------------------------------------------------------------------------------
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. :-)
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.