The current replay menu doesn't convert OsPath names using OsString but string8 which is an alias of utf8_from_wstring, which is only suitable to printing in the GUI but not opening files.
While at it use OsPath consistently and the actual type for fileSize returned by CFileInfo->Size(), which is a signed integer type according to posix.
Details
- Reviewers
Imarok - Commits
- rP19824: Fix the replay menu for people with non-latin characters in their username.
- Trac Tickets
- #4320
#4647
Don't apply the patch yet, but change GetDirectoryName to return rêplays instead of replays.
Create that directory and insert some replays.
Notice the thing crashing. Apply the patch and keep the GetDirectoryName replacement. Now everything should be working, including playing a game, using the replay menu, loading the replay metadata, starting a replay from commandline visually and non-visually.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 2371 Build 3957: Vulcan Build (Windows) Jenkins Build 3956: Vulcan Build Jenkins Build 3955: arc lint + arc unit
Event Timeline
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/1251/ for more details.
(You tested it according to the test plan, right?)
Also yes, the c_str can be removed from the ifstream and ofstream ctor:
(22:53:47) Vladislav2: Imarok: probably it's from C++<11, because ifstream has ctor with strings only from C++11
http://www.cplusplus.com/reference/fstream/ifstream/ifstream/
When searching why I used string8 in rP17356, I found this other change:
From 2015-08-30-QuakeNet-#0ad-dev.log:
< Itms> elexis: your OsString change in rP16963 doesn't work on windows
< Itms> apparently it's because Path::String is a wstring
< Itms> and msg is a stringstream
Vladislav could confirm that this patch doesn't compile on windows:
(01:52:42) Vladislav: elexis: I have compilation error, i.e.: couldn't convert const Path::String to std::string
os_path.h has both types (Path::String is a typedef std::wstring String):
static inline const Path::String& OsString(const OsPath& path) static inline std::string OsString(const OsPath& path)
You noticed the if OS_WIN ?
#if OS_WIN static inline const Path::String& OsString(const OsPath& path) #else static inline std::string OsString(const OsPath& path) #endif
So on win OsString converts to wstring and on linux it converts to string.
source/gui/scripting/ScriptFunctions.cpp | ||
---|---|---|
208–1108 | Is it just this part, that is failing on win? |
Always use OsString for ifstream and ofstream, see documentation of the ctors and r9090.
Reverting what Imarok mentioned is correct as OsString.string() always returns a wide string.
Tested positively on unix (replay menu, replay cache, command line visual and nonvisual replay).
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jw:8080/job/phabricator/1587/ for more details.
Executing section Default... Executing section Source... source/gui/scripting/ScriptFunctions.cpp | 711| » return·*(volatile·int*)0; | | [MAJOR] CPPCheckBear (nullPointer): | | Null pointer dereference Executing section JS... Executing section XML GUI... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/211/ for more details.
Fix that string8 TODO with a patch by Philip, thereby fix the debug_printf output for non-ascii characters.
Think so, but can't test.
22:12 < Philip`> Technically the filesystem doesn't matter, we care about the filesystem API
22:13 < Philip`> and we use the POSIX API where paths are always 8-bit values
22:13 < Philip`> (I assume OS X converts to UTF-8 or something)
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jw:8080/job/phabricator/1605/ for more details.
Executing section Default... Executing section Source... source/gui/scripting/ScriptFunctions.cpp | 711| » return·*(volatile·int*)0; | | [MAJOR] CPPCheckBear (nullPointer): | | Null pointer dereference Executing section JS... Executing section XML GUI... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/225/ for more details.
Works on win and linux. (mac should maybe be tested)
(Assuming e change when committing)
ping @wraitii
Could you test this patch on OSX?
Test Procedure:
You need to create a replay directory that starts with a and contains a non-ascii character, like arêplays.
Then the replay menu should work as expected and the terminal output should print that character correctly when starting a game or replay.
Am a bit confused, because replays are now directly in my application support folder instead of inside "replay" ? Is that desired?
Otherwise it works but the path name is not correct in the bottom area, I see this:
It does work beyond that, so I suppose it's an improvement.
Does the console also print out the wrong path when starting and then finishing the game?
What if you change #if !OS_WIN in path.h to #if OS_WIN ?
The directory is correct:
https://trac.wildfiregames.com/wiki/GameDataPaths#OSX
The bad news is that I could reproduce the bug on linux.
The good news is that we can now assume that our assumptions is correct and OSX works like unix.
Interestingly, no matter whether GetReplayDirectoryName returns OsPath.string, OsPath.string8 or utf8_from_wstring(OsPath.string), the output is not correct.
Also, with Alpha 21, if we start a visual replay from command line with a path with a non-ascii character in it, the terminal output is correct, despite not having Philips patch applied.
Hm.
Not sure why, but wstring_from_utf8(OsPath(VisualReplay::GetDirectoryName() / directoryName).string8()) ends up correctly in the GUI.
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jw:8080/job/phabricator/1619/ for more details.
Executing section Default... Executing section Source... source/gui/scripting/ScriptFunctions.cpp | 711| » return·*(volatile·int*)0; | | [MAJOR] CPPCheckBear (nullPointer): | | Null pointer dereference Executing section JS... binaries/data/mods/public/gui/replaymenu/replay_menu.js | 100| » » if·(g_MapNames.indexOf(replay.attribs.settings.Name)·==·-1·&&·replay.attribs.settings.Name·!=·"") | | [NORMAL] JSHintBear: | | Use '!==' to compare with ''. binaries/data/mods/public/gui/replaymenu/replay_menu.js | 181| » if·(attribs.settings.PlayerData.length·&&·attribs.settings.PlayerData[0]·==·null) | | [NORMAL] JSHintBear: | | Use '===' to compare with 'null'. Executing section XML GUI... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/235/ for more details.
Adding the same change to NetClientTurnManager.cpp while at it. The playernames are still broken in that OOS message (and we can't convert a vector of wstrings somehow), it should only send the GUIDs some day.
Thanks for the reviews and everyone helping!
source/main.cpp | ||
---|---|---|
478 | Hope this output was tested on windows with non-latin character too | |
source/ps/VisualReplay.cpp | ||
183 | Notice that this passes the unprintable string, but that is correct as it won't be printed but used for identifying and building OsPaths |