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