#868 closed task (fixed)
[PATCH] Move wxWidgets Unix config files to XDG basedir location
Reported by: | Philip Taylor | Owned by: | leper |
---|---|---|---|
Priority: | Nice to Have | Milestone: | Alpha 10 |
Component: | Core engine | Keywords: | simple, patch |
Cc: | Patch: |
Description
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).
Attachments (6)
Change History (24)
comment:1 by , 12 years ago
Keywords: | review patch added |
---|---|
Owner: | set to |
Status: | new → assigned |
Summary: | Move wxWidgets Unix config files to XDG basedir location → [PATCH] Move wxWidgets Unix config files to XDG basedir location |
by , 12 years ago
Attachment: | atlas-xdg-2012-01-15.patch added |
---|
comment:2 by , 12 years ago
Keywords: | review removed |
---|---|
Milestone: | Backlog → Alpha 9 |
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.
comment:3 by , 12 years ago
Keywords: | review added |
---|
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/.
by , 12 years ago
Attachment: | atlas-xdg-2012-01-22.patch added |
---|
follow-ups: 5 15 comment:4 by , 12 years ago
Keywords: | review removed |
---|
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 toBeginAtlas()
? - Would we have to handle the case of the config directory not being set in
OnInit()
, like we do for the data directory?
by , 12 years ago
Attachment: | atlas-xdg-2012-02-03.patch added |
---|
comment:5 by , 12 years ago
Keywords: | review added |
---|
Replying to historic_bruno:
Could we have an
Atlas_SetConfigDirectory
? There's already aPaths::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 toBeginAtlas()
?
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.
comment:6 by , 12 years ago
Keywords: | review removed |
---|
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)
by , 12 years ago
Attachment: | atlas-xdg-2012-02-21.2.patch added |
---|
comment:7 by , 12 years ago
Keywords: | review added |
---|
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.
comment:8 by , 12 years ago
Thanks for the update :)
About DLLInterface.cpp
changes:
- It won't compile on at least OS X, due to mixing C-strings with
wxString
s. It seemswxT
is responsible, we generally use_T()
instead, it's a macro for converting internal C-strings towxString
s. - 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 beNULL
if the variable is not set. Instead it suggests the two argument form, where if the second argument isNULL
, it returns whether the variable was set (as abool
). - 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.
follow-up: 10 comment:9 by , 12 years ago
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.
by , 12 years ago
Attachment: | atlas-xdg-2012-02-25.patch added |
---|
follow-up: 11 comment:10 by , 12 years ago
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.
comment:11 by , 12 years ago
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.
comment:12 by , 12 years ago
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).
comment:13 by , 12 years ago
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.
comment:14 by , 12 years ago
Milestone: | Alpha 9 → Alpha 10 |
---|
comment:15 by , 12 years ago
Replying to historic_bruno:
- I'm not sure of the init order, when does
OnInit()
get called relative toBeginAtlas()
?
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 singleg_ConfigDir
defined inDLLInterface.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 whetherg_ConfigDir
has been set yet (probably withg_ConfigDir.IsEmpty()
).
by , 12 years ago
Attachment: | atlas-xdg-2012-03-04.patch added |
---|
comment:18 by , 8 years ago
Keywords: | review removed |
---|
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.