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.