Page MenuHomeWildfire Games

Use OsPath, OsString and off_t for Replays
ClosedPublic

Authored by elexis on May 19 2017, 3:01 PM.

Details

Summary

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.

Test Plan

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.

Event Timeline

elexis created this revision.May 19 2017, 3:01 PM
Vulcan added a subscriber: Vulcan.May 19 2017, 4:21 PM

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.

Imarok accepted this revision.May 20 2017, 10:54 PM
This revision is now accepted and ready to land.May 20 2017, 10:54 PM

(maybe remove c_str() for fstreams)

elexis added a comment.EditedMay 20 2017, 11:00 PM

(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/

elexis planned changes to this revision.May 21 2017, 3:01 AM

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)
In D518#21314, @elexis wrote:

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.

Imarok added inline comments.Jun 18 2017, 11:44 PM
source/gui/scripting/ScriptFunctions.cpp
208–1108

Is it just this part, that is failing on win?
Then you could just revert this part and it should work afaics.

elexis updated this revision to Diff 2623.Jun 19 2017, 10:03 PM

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).

This revision is now accepted and ready to land.Jun 19 2017, 10:03 PM

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.

elexis requested review of this revision.Jun 20 2017, 12:08 AM
elexis edited edge metadata.
elexis updated this revision to Diff 2655.Jun 22 2017, 1:05 AM

Fix that string8 TODO with a patch by Philip, thereby fix the debug_printf output for non-ascii characters.

elexis updated the Trac tickets for this revision.Jun 22 2017, 1:08 AM

Do you think the paths print out correctly on mac?

source/ps/VisualReplay.cpp
49

No

In D518#26770, @Imarok wrote:

Do you think the paths print out correctly on mac?

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)

https://en.wikipedia.org/wiki/POSIX#POSIX-certified

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.

Imarok accepted this revision.Jun 23 2017, 12:25 AM

Works on win and linux. (mac should maybe be tested)
(Assuming e change when committing)

This revision is now accepted and ready to land.Jun 23 2017, 12:25 AM
elexis added a subscriber: wraitii.Jun 24 2017, 4:28 PM

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.

In D518#26998, @wraitii wrote:

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.

elexis updated this revision to Diff 2675.Jun 24 2017, 8:41 PM

Not sure why, but wstring_from_utf8(OsPath(VisualReplay::GetDirectoryName() / directoryName).string8()) ends up correctly in the GUI.

Works on win

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

This revision was automatically updated to reflect the committed changes.