I want to update VCMIDirs.h and VCMIDirs.cpp files.
Pros:
Removed ifdef’s
Tree based interface will allow to inherit some paths from ‘parent’ system. For example android can inherit Linux paths and update only some of them.
Cons:
I don’t like singleton pattern.
I make test version of it, and it’s work… on Windows. Tomorrow I commit rest of changed files (i changed std::string based paths to boost::filesystem::path) so someone can test this on other platforms.
Class structure
[ul]]IVCMIDirs
[list]]VCMIDirs_win32/:m] ]IVCMIDirs_UNIX
[list]]VCMIDirs_OSX/:m] ]VCMIDirs_Linux
[list]]VCMIDirs_Android/:m][/ul]/:m][/list:u]/:m][/list:u]/:m][/list:u]
I have some questions:
Why on Windows VCMI uses %userprofile% instead of %appdata%? 2. Why on Linux and Android points to userDataPath() / “Saves”, and on Windows and OSX to userDataPath() / “Games”? Done.
What do you think about it? Any suggestions? Errors?
// EDIT1: 4. Should VCMI search libraries and binaries in %PATH% environment variable? For example: Unnecessary. OS doing it itself.
boost::filesystem::path IVCMIDirs::binaryPath(std::string file_name);
1. Try to find file in current directory. If finded, return path as result.
2. Try to find file in user data directory (%userprofile%/vcmi). If finded, return path as result.
3. Try to find file in directories in %PATH% variable. If finded, return path as result.
4. Return file_name + ".exe"
// EDIT2: Ouch… I forgotten to create directories in IVCMIDirs constructor. (IVCMIDirs virtual destructor needed?) Done!
I think it’s a good idea to search in both the way it’s currently done (M_BIN_DIR on Linux) (for development), and in $PATH (when it’s actually expected).
However, I don’t think VCMI should know where the binary is - it should use the standard system’s way of launching from $PATH. It already calls system() to launch the server, so I don’t think a change is needed there.
Don’t know. Following guidelines (from MS in this case) is usually a good idea but this means changing behavior. I would agree to change if:
Windows developers won’t be against it
There will be code that will update data location (if new path is missing and old exists - move old dir to new location)
No reason. I suggest renaming to Saves - this is more self-explaining + this is how our code refers to that location in our VFS (virtual file system). And since we no longer use H3 location for user data there won’t be confusion on what directory is actually used.
Good question.
For libraries - probably easier to keep as it. On Linux OS is responsible for searching libvcmi.so thus we don’t need to do anything here. For “plugins” like AI - I’d rather keep one fixed path we have now.
For binaries - yes, vcmi uses system() call to start server but it passes full path into it - we’re not relying on OS for that. Using %PATH% should probably be a fallback so we won’t be breaking up development environment where VCMI may not be actually “installed”
After a quick look on your code:
system includes usually are not necessary - all of them should be included already in global.h (we use precompiled headers).
encoding conversion - you can use boost::locale for that, see our functions like Unicode::toUnicode or boost docs for examples
I’m OK with renaming these files to something like SystemDirs.h/cpp but I’m not sure if there is any need to split them - we usually use one file for all classes with same “purpose” (one “module”) instead of file per class approach.
And BTW:
if (home_dir = getenv("HOME")) // compatibility, should be removed after 0.96
Hint: we’re after 0.96 already
Can be removed safely.
Because it is expected that users might want to acsess their savegames or config files. We don’t want them to look for it in some hidden directory.
Recommended by MS or not, most of the games I have store their saves either in home or in the My Documents directory (or its subdirs).
I’d to the top of the list “the directory where the current .exe is placed”. The current working directory often may be different from actual location (eg. when game is run by the start menu). However, the server and client are practically always together.
As for the utf issues, you should assume that all non-wide strings are utf8-encoded.
Use the following calls to make boost::filesystem assume utf8 for non-wide strings (rather than local system encoding):
bfs = boost::filesystem
Updated:
[ul]Global.h
[list]Added platform detection macros.
Included some more bfs includes.[/ul]
CBasicLogConfigurator uses now bfs::path pathes.
VCMIDirs
[ul]Removed utf8 conversion.
Added init() method what creates and updates directories
On Windows and Apple operating systems changed save path to “Saves”
[list]Ofc init() will update these directories.
I don’t know Apple platform, so I used boost to move old saves to new directory.
On Windows I use SHFileOperationW function. This function should show “moving file dialog” and ask user to resolve conflicts (when file exists).
init() removes old directories (only when they are empty)[/ul]
Some functions moved to ‘parent’ class.[/list:u]
Other files
[ul]Update file pathes to use boost::filesystem (part 1)
Minor fixes[/ul][/list:u] Commit
I don’t make boost::locale conversion to UTF-8. I try to do this after updating all file pathes (not VFS) to bfs::path.
I didn’t removed WinAPI headers because:
[ul]Some of them are not available in precompiled headers.
After adding all WinAPI headers compilation will fail due to stupid errors. WinAPI headers defines some stupid macros (min, max, small, ERROR), but this isn’t a problem. Problem is that VCMI defines IFont interface and WinAPI contains this interface too.
So. After 10x recompilation whole VCMI, I gave up.[/ul]
I don’t removed headers from VCMIDirs.h because:
[ul]I don’t want this now. When this headers are missing syntax suggesting don’t work well.
I don’t think this should extend the time of compilation. If these headers are defined in precompiled headers then compiler should omit these includes automatically (precompiled header is always included first).
Before last commit I delete these headers, but I still think that including headers files isn’t bad idea (at least in *.h files).[/ul]
I always expected that application contains own data in %appdata% or My Documents (what I hate, because I don’t want have settings in this folder), but I’m not a normal user.
However (for example) Minecraft contains it’s data in %appdata% folders and people deal with it. I think that average HOMM player is smarter then average Minecraft player, so it isn’t a problem.
The problem is that VCMI data directory can contain VCMI executable. So. It need some more advanced code to properly move content of this directory.
Why not use “My Documents/My Games”? IIRC some games use this path so at least VCMI won’t clobber My Document with one more directory.
Or just redirect users to Launcher which displays user data path and from next release - has a button to open it.
This is possible but unlikely. Still, this code will be needed only for one release to provide a smooth update. At most - add warning in console that VCMI failed to move data to new location.
Another update (commit)
[ul]CConsoleHandler
[list]Now uses VCMI_WINDOWS macro.
WINDOWS: Added coloring for stderr.[/ul]
CLogger
[ul]Removed android/log.h header. (This file is included in Global.h)
Added some functions with r-value references.
Now uses VCMI_ANDROID macro.
Now uses bfs::path paths.
Now uses bfs::ofstream file operations.
Minor fixes.[/ul]
Other files - Some classes are now structures. Updated announcing declarations (it’s proper name?).[/list:u]
*bfs = boost::filesystem.
It’s good idea. I forgot about this folder, because most my games are on Steam, and datas are in cloud. What other developers think about this?
// EDIT:
Fix (commit)
[ul]Updated some names to camel casing.
MSVC++ 11.0 and above compilers shouldn’t show warning C4996 now.[/ul]
We do not want WINAPI in PCH. The platform-specific includes should be preferable as locally isolated as possible.
As for the min/max issue there was some macro that prevented windows.h from overwriting them .We should add it globally, I remember there were once some issues with min/max getting overridden and causing compile errors.
It should work fine, at least in MSVC. You may try also adding the proejct root directory to the project properties include list and see if that helps.
Accesing savegame files is expected use case. We don’t want user accessing hidden directories when performing typical use-cases. By default AppData is not even visible in the Windows Explorer. Even if users are smart, we shouldn’t make things harder for them than necessary.
“Docs/My Games” is acceptable.
CLoggerDomain::CLoggerDomain(const std::string & name) : name(name)
{
if(name.empty())
throw std::runtime_error("Logger domain cannot be empty.");
}
CLoggerDomain::CLoggerDomain(std::string && name) : name(std::move(name))
{
if (this->name.empty())
throw std::runtime_error("Logger domain cannot be empty.");
}
Use this instead:
CLoggerDomain::CLoggerDomain(std::string name) : name(std::move(name))
{
if (this->name.empty())
throw std::runtime_error("Logger domain cannot be empty.");
}
The commonly used name is “forward declaration”. The standardese term is “declaration” as opposed to “definition”.
Legendary Heroes uses My Documents\My Games\Legendary Heroes\Mods for mods, and various other directories under Legendary Heroes for other stuff, so you could use that sort of directory structure if you want (My Documents\My Games\GameName\Directory).
Update #5(commit)
[ul]Global.h
[list]WINDOWS: Added NOMINMAX macro.
Minor fixes[/ul]
CPlayerInterface.cpp, CBasicLogConfigurator.[h/cpp]
[ul]minor fixes[/ul]
Launcher
[ul]StdInc: Added pathToQString function. (converts boost::filesystem::path to QString)
string based paths -> boost::filesystem::path paths[/ul]
VCMIDirs.cpp
[ul]Minor fixes
Added UTF-8 locale.
moveDirIfExists now returns true when succeeded, false otherwise.
moveDirIfExists now updates current path.
Added advancedMoveDirIfExists method
[list]Tries to use standard moveDirIfExist.
When it fails and current executable exists in folder what should be moved:
[list]Starts batch script and closes application.
Batch script should replace directory when application is closed.[/ul][/list:u]Changed userDataPath from %USERPROFILE% to %USERPROFILE%\Documents
minor fixes[/list:u]
Editor.cpp, CGameInterface.cpp, CArchiveLoader.[h/cpp], CInputFileStream.[h/cpp], CFilesystemLoader.[h/cpp], Filesystem.cpp, CLogger.[h/cpp], ERMInterpreter.cpp, CVcmiTestConfig.cpp
[ul]string based paths -> boost::filesystem::path paths
minor fixes (not in all)[/ul][/list:u]
Ok. Done.
Done. Almost. When VCMIDirs system is initiated, logger doesn’t exist. Should I use puts/printf or something else?
Ok. Macro added.
Ok. My bad. Not working when file is located in sub-folder (IntelliSense don’t see file StdInc.h).
I feel kinda like an idiot. I spent much time to find ‘how to mark auto ‘reference’ in C++11’…
Ok. Thanks.
On Windows: My Documents/My Games/vcmi
Other platforms: unchanged.
Done. Will synced with next commit.
TODO:[ul]Log warnings in VCMIDirs.cpp (Should I use puts/printf?) Change all _WIN32, APPLE etc. macros to VCMI_WINDOWS etc. Done.[/ul]I forgot about something?
