[PATCH] Move wxWidgets Unix config files to XDG basedir location

[Philip]

Atlas uses wxConfig (in source/tools/atlas/AtlasUI/Misc/DLLInterface.cpp) which is wxFileConfig on Unixes. By default that uses the config file "~/.Atlas Editor". It would be cleaner to store it alongside the game's own config files in probably ~/.config/0ad/config/atlas.ini (unless overridden by environment variables, as per ​basedir).

[leper]

Implemented this using a workaround as the build system allows for external wxWidgets. I did exclude OSX as I am not sure if the table in ​BuildAndDeploymentEnvironment is the final version or if the XDG_CONFIG_HOME environment variable is set to ~/Documents/0ad/config or not.

[historic_bruno]

Looks like wxConfig is not being initialized properly in the patch, see the ​wxConfigBase documentation. The first argument is the app name, the second is vendor name, I think those are already OK, but then there are optional arguments for specifying the file name and path. Looks like it can support either relative or absolute paths, so it shouldn't be necessary to do anything like: "/../../../../../../../../../../..".

Is it defined whether XDG_CONFIG_HOME should be a relative or absolute path?

I think the patch should handle OS X the same way for now, we can come back and update that later when the paths change.

[leper]

Thanks for the link... seems like i have missed that part of the wxConfig documentation.

XDG_CONFIG_HOME should be an absolute path in all cases.

The new patch handles OS X in the same way as linux (or all unix systems) so the atlas config file should show up in ~/.config/0ad/config/.

[historic_bruno]

I was looking at path-related code and I noticed we call Atlas_SetDataDirectory with the game's path data (in BeginAtlas). Could we have an Atlas_SetConfigDirectory? There's already a Paths::Config() to get the local config directory. This would be nice because we wouldn't have to duplicate and construct the paths in two different locations (and the OS X / Linux distinction could be contained within Paths). It already handles the XDG variables. Mostly I'm suggesting this for OS X, because we will probably use the Cocoa API to get the standard paths, so if we can reuse the Paths interface, that would work out great.

Possible problems with that approach:

• More global variables - blah
• I'm not sure of the init order, when does OnInit() get called relative to BeginAtlas()?
• Would we have to handle the case of the config directory not being set in OnInit(), like we do for the data directory?

[leper]

Replying to historic_bruno:

Could we have an Atlas_SetConfigDirectory? There's already a Paths::Config() to get the local config directory.

I implemented this and I think it is a cleaner solution than the previous ones. Now it is just using one code path for all OSs.

Possible problems with that approach:

• More global variables - blah

It's just one additional global variable so that shouldn't be a problem.

• I'm not sure of the init order, when does OnInit() get called relative to BeginAtlas()?

I didn't check the init order but from my testing BeginAtlas() is called before OnInit() so the config path gets set before being accessed.

• Would we have to handle the case of the config directory not being set in OnInit(), like we do for the data directory?

Since the config dir is set and the check for the data dir is just in case of a execution mode that doesn't set pwd right I don't think we should bother.

[historic_bruno]

Hmm I was testing your latest patch, getting ready to commit it, but unfortunately it breaks things :(

On Windows, settings get stored in the registry and supplying the localFilename parameter in wxConfig (defined as wxRegConfig on Windows) actually overrides the default registry path rather than being ignored, so we don't want to do that.

Also we forgot about Actor Editor, it doesn't run as part of the engine so Atlas_SetConfigDirectory is indeed never called. The worse part is we can't share the Paths logic because Actor Editor is a completely separate application, it only shares AtlasUI :/ Maybe your previous patches can be combined, if scenario editor is being run and the config path gets set, then we use Paths, otherwise if the config path wasn't set, we construct the path with XDG variables. (Some day we should integrate Actor Editor with the scenario editor, but not likely in the near future)

[leper]

This version should fix the problems. It special cases Actor Editor to use the logic from the 2nd patch and only applies the filename if on Unix (I am not sure if using !OS_WINDOWS would be better).

Disregard the first version as it contains a redundant comment.

[historic_bruno]

Thanks for the update :)

About DLLInterface.cpp changes:

• It won't compile on at least OS X, due to mixing C-strings with wxStrings. It seems wxT is responsible, we generally use _T() instead, it's a macro for converting internal C-strings to wxStrings.
• I don't know that ​wxGetenv is being used correctly. With a single argument, it seems to return a wxChar*, but the docs don't specify that it's safe to assume that will be NULL if the variable is not set. Instead it suggests the two argument form, where if the second argument is NULL, it returns whether the variable was set (as a bool).
• Maybe instead of including sysdep stuff, we should follow the norm in AtlasUI and use the WX platform defines instead of OS_UNIX: there's no Unix-specific macro but there is __WXMSW__ which should always be set on Windows.

[leper]

I used __WXMSW__ to detect if we are using Windows and omit the path. ​The wxWidgets documentation (that page is not available for versions < 2.9 but definitions probably apply to 2.8 as well) lists some OS specific macros but I think using __WXMSW__ is better to express the intent. Config paths for OSX could use one of those macros once those paths are decided upon.

It is now using _T exclusivley and using wxGetEnv for the test and wxGetenv to get the value.

How does ActorEditor decide on the config file/registry on Windows? And why are we using the registry to store the Atlas config at all? We could just replace `#include "wx/config.h" with wx/fileconfig.h to use a config file on Windows.

[historic_bruno]

Replying to leper:

How does ActorEditor decide on the config file/registry on Windows? And why are we using the registry to store the Atlas config at all? We could just replace `#include "wx/config.h" with wx/fileconfig.h to use a config file on Windows.

wxConfig is defined in wx\confbase.h:

// under Windows we prefer to use the native implementation
// wxIniConfig isn't native anywhere after droping win16 in wxWidgets 2.6
#if defined(__WXMSW__) && wxUSE_CONFIG_NATIVE
    #define wxConfig  wxRegConfig
#else // either we're under Unix or wish to use files even under Windows
  #define wxConfig  wxFileConfig
#endif

I'm not sure why we use that instead of wxFileConfig (other then the purpose of wxWidgets being to have sensible behavior on different platforms, and we usually don't mess with the defaults).

Is there a compelling reason not to use wxConfig/wxRegConfig on Windows? :) A few extra lines of code is no big deal.

[leper]

Replying to historic_bruno:

Is there a compelling reason not to use wxConfig/wxRegConfig on Windows? :) A few extra lines of code is no big deal.

We already store config files on Windows (local.cfg if the user creates it), so the sensible behavior of wxWidgets is defeated by the game itself and storing information in two different locations doesn't seem intuitive.

[historic_bruno]

Hmm I can think of a good reason to use wxRegConfig on Windows, it saves us having to construct %appdata% in Atlas (seems potentially very yucky if we have to do that, and we'd have to maintain compatibility with older and newer Windows versions).

[leper]

I just wanted to note that we're already storing files on Windows but I think adding more OS specific code to Atlas isn't good. So just leave wxRegConfig and maybe change that behavior when ActorEditor? gets merged.

[Philip]

Replying to historic_bruno:

• I'm not sure of the init order, when does OnInit() get called relative to BeginAtlas()?

GameInterface/GameLoop.cpp's BeginAtlas calls Atlas_StartWindow, implemented in AtlasUI/Misc/DLLInterface.cpp, which calls wxEntry, which instantiates the AtlasDLLApp and calls OnInit. Then GameLoop loops a few times, until the UI thread gets around to sending the Init message (GraphicsSetupHandlers.cpp) which calls GameSetup.cpp's Init.

So GameLoop.cpp's current call of Atlas_SetDataDirectory occurs before any AtlasUI code runs (not counting DllMain which gets called when the .dll is first loaded).

• Would we have to handle the case of the config directory not being set in OnInit(), like we do for the data directory?

That happens when launching a standalone executable (ActorEditor.exe etc), since no engine code is run. Presumably the actor editor should use XDG paths too for config data, so OnInit needs to handle that.

About the latest patch:

• Config.h/cpp looks like unnecessary boilerplate - all it needs is a single g_ConfigDir defined in DLLInterface.cpp, since it's not used anywhere else.
• Instead of hardcoding the comparison to "ActorEditor" (which will break if we add more standalone executables in the future), it'd be cleaner to just check whether g_ConfigDir has been set yet (probably with g_ConfigDir.IsEmpty()).

[leper]

I did what Philip suggested and it is cleaner.

[philip]

In 11374:

Fix #868 (move wxWidgets Unix config files to XDG basedir location), based on patch from leper.
Fix handling of set but empty XDG environment variables.