bugfix(globaldata): Fix the handling of documents folder redirection by using SHGetKnownFolderPath()

[Mauller]

This PR improves folder redirection handling that can occur due to Group Policy or OneDrive redirecting the users documents folder.

This only works for non retail builds as it required a function not supported under VC6 as it only appeared in Vista onwards.
SHGetKnownFolderPath() has better handling of folder redirection built into it compared to the legacy SHGetSpecialFolderPath()

Generals and zero hour use different ways to obtain the game folder name which is why the code is different between them.

Edit - To support building in VC6 without compilation conditional, SHGetSpecialFolderPath is obtained as a function pointer if it is available.
Defines for the GUID of the documents folder path are also added locally as these do not exist pre Vista.

[greptile-apps]

Greptile Summary

This PR improves user-data directory resolution in both Generals and Zero Hour by preferring SHGetKnownFolderPath() (Vista+), loaded at runtime via GetProcAddress for backward compatibility with VC6 and pre-Vista systems, over the legacy SHGetSpecialFolderPath(). The path-building logic is extracted into dedicated helper functions (BuildUserDataPathFromIni in Generals, BuildUserDataPathFromRegistry in Zero Hour), with VC6-safe local definitions for FOLDERID_Documents and KF_FLAG_DEFAULT under #if _MSC_VER < 1300. An early-return guard is added for the case where both APIs fail, preventing a garbled path from reaching CreateDirectory.

• BuildUserDataPathFromIni() is correctly declared static in Generals; the mirrored BuildUserDataPathFromRegistry() in Zero Hour is non-static despite using no instance state — a minor inconsistency worth aligning.
• Prior review feedback on CoTaskMemFree placement and the off-by-one preprocessor guard has been acknowledged and addressed by the developer.
• The FOLDERID_Documents GUID {FDD39AD0-238F-46AF-ADB4-6C85480369C7} in the local VC6 defines is correct.

Confidence Score: 4/5

• PR is safe to merge; the one remaining P2 (static declaration inconsistency) does not affect runtime behaviour.
• The core logic is sound: runtime detection of SHGetKnownFolderPath, correct VC6 fallback, matching GUID, and the early-return guard on failure all look good. Prior critical feedback (CoTaskMemFree, garbled path) has been addressed. The only open item is a minor style inconsistency (BuildUserDataPathFromRegistry not being static), which does not impact correctness.
• GeneralsMD/Code/GameEngine/Include/Common/GlobalData.h — BuildUserDataPathFromRegistry should be declared static for consistency with the Generals module.

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Include/Common/GlobalData.h	Adds static AsciiString BuildUserDataPathFromIni() private declaration; header copyright and #pragma once guard are correct.
Generals/Code/GameEngine/Source/Common/GlobalData.cpp	Replaces inline SHGetSpecialFolderPath call in parseGameDataDefinition with a new static helper that prefers SHGetKnownFolderPath (runtime-loaded) and falls back gracefully; early-return guard on empty path is in place.
GeneralsMD/Code/GameEngine/Include/Common/GlobalData.h	Adds AsciiString BuildUserDataPathFromRegistry() as a non-static member, inconsistent with the analogous static declaration in Generals; no other concerns.
GeneralsMD/Code/GameEngine/Source/Common/GlobalData.cpp	Mirrors the Generals refactor for Zero Hour; BuildUserDataPathFromRegistry() is declared non-static despite using no instance state, unlike its Generals counterpart.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["parseGameDataDefinition / GlobalData()"] --> B["BuildUserDataPathFromIni/Registry()"]
    B --> C{"GetProcAddress\nSHGetKnownFolderPath"}
    C -- "found (Vista+)" --> D["pSHGetKnownFolderPath(FOLDERID_Documents)"]
    D -- "SUCCEEDED && pszPath" --> E["translate wide path → AsciiString\nCoTaskMemFree(pszPath)"]
    D -- "failed" --> F["myDocumentsDirectory is empty"]
    C -- "not found (pre-Vista)" --> G["SHGetSpecialFolderPath(CSIDL_PERSONAL)"]
    G -- "success" --> H["myDocumentsDirectory = temp"]
    G -- "failed" --> F
    E --> I{"isEmpty?"}
    H --> I
    F --> I
    I -- "yes" --> J["return TheEmptyString"]
    I -- "no" --> K["append '\\' + leaf name + '\\'"]
    K --> L["return full path"]
    L --> M["m_userDataDir = result"]
    M --> N["CreateDirectory(m_userDataDir.str(), nullptr)"]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Include/Common/GlobalData.h
Line: 579

Comment:
**`BuildUserDataPathFromRegistry` should be `static`**

`BuildUserDataPathFromRegistry()` accesses no instance members — it uses only `GetStringFromRegistry()` (a free function) and Win32 APIs. The analogous function in the Generals module is correctly declared `static`:

```cpp
static AsciiString BuildUserDataPathFromIni();   // Generals
```

Declaring this one `static` as well makes the intent explicit (no `this` dependency), keeps the two modules consistent, and prevents accidental future reliance on instance state.

```suggestion
	static AsciiString BuildUserDataPathFromRegistry();
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (7): Last reviewed commit: "bugfix(globaldata): Fix the handling of ..." | Re-trigger Greptile}

[Mauller]

Ah i didn't check building with VC6, will work on this further

[Mauller]

Updated by making the fix conditoinal so it only works in non VC6 builds

[Mauller]

Cleaned up the implementations by replicating the whole code block

[Mauller]

Removed controversial as this fix does not work in VC6 in the end anyway, so i had to make the fix conditional at compilation

[Skyaero42]

Can hardcoded path separators be an issue for (future) linux support?

[Mauller]

All of this will need altering to support linux in the future, the main functions for getting the documents folder are windows API.

[Mauller]

This was pain, but it works for building in VC6.

[Mauller]

Fixed accidental variable shadowing in the legacy pathway.

[OmniBlade]

Do Generals and Zero Hour provide the correct data for both methods even though their code only uses one path? If so, can we not unify this code so it tries one path first and falls back to the second path?

[Mauller]

Do Generals and Zero Hour provide the correct data for both methods even though their code only uses one path? If so, can we not unify this code so it tries one path first and falls back to the second path?

No, zero hour only has the registry entry and generals only has the ini entry.

[Mauller]

Updated based on feedback.

[OmniBlade]

Do Generals and Zero Hour provide the correct data for both methods even though their code only uses one path? If so, can we not unify this code so it tries one path first and falls back to the second path?

No, zero hour only has the registry entry and generals only has the ini entry.

Okay, so then if you unified the code to try the registry first then fall back to ini or vice versa, the code would cover both games and can be made common?

[xezon]

Okay, so then if you unified the code to try the registry first then fall back to ini or vice versa, the code would cover both games and can be made common?

Yes but that is beyond the scope of this change.

Mauller could do a Unify change before this change however to streamline the behavior.

[xezon]

Looks ok to me.

[xezon]

Explicit UnicodeString construction should not be necessary.

[xezon]

Can be simplified with

if (!myDocumentsDirectory.isEmpty())
{
 ...
}

return myDocumentsDirectory;