Separate ModIo JSInterface from Mod
ClosedPublic
Actions

Authored by elexis on Apr 23 2018, 4:54 PM.

Details

Reviewers

Itms

Commits

rP21778: Move ModIo JS interface functions to a separate namespace, refs rP21759.

Summary

These two sets of functions are not logically coupled, so it makes things a bit easier to comprehend, maintain or replace if they remain in separate interfaces, refs rP21759.
I'm late to the C++ party.

Test Plan

Make sure that it still compiles.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Apr 23 2018, 4:54 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/431/display/redirect

Harbormaster completed remote builds in B5952: Diff 6461.Apr 23 2018, 6:02 PM

vladislavbelov added a subscriber: vladislavbelov.Apr 23 2018, 11:09 PM

vladislavbelov added inline comments.

source/ps/scripting/JSInterface_ModIo.h
39 ↗	(On Diff #6461)	Add a comment: https://trac.wildfiregames.com/wiki/Coding_Conventions#Creatingnewfiles.

This looks fine to me iff you add the comment Vlad pointed out and move the strings back next to where they are used.

source/ps/scripting/JSInterface_ModIo.cpp
26 ↗	(On Diff #6461)	I would keep this next to `GetDownloadProgress`.

This revision is now accepted and ready to land.Apr 24 2018, 11:34 AM

elexis added inline comments.Apr 24 2018, 11:53 AM

source/ps/scripting/JSInterface_Mod.h
36 ↗	(On Diff #6461)	(Guess I can add it here too while at it)
source/ps/scripting/JSInterface_ModIo.cpp
26 ↗	(On Diff #6461)	The idea was that the reader isn't surprised by the sudden occurrence of data after reading many functions. For JS and C++ the diffs I had accepted or committed had moved/introduced all globals at the top of the file throughout the last years, so that everything holding the state and data comes first, then everything below are functions transitioning but not holding the state. (I thought it's already in the CC, but apparently not.) So these two from `main.cpp` should have been moved from the top to `Frame()` and `RunGameOrAtlas()` then? extern bool g_GameRestarted; extern CStrW g_UniqueLogPostfix;

Itms added inline comments.Apr 24 2018, 12:19 PM

source/ps/scripting/JSInterface_ModIo.cpp
26 ↗	(On Diff #6461)	Those are globals, used across the engine, I think it's nice that they are on top of the file. Whereas those status strings are just a constant, only used in this file, and actually tightly bound to `GetDownloadProgress`, since the function is about making the conversion between engine values and JS values, and that structure is the dictionary for that conversion. If I could I would have put this map inside the function, but that would allocate it everytime it is called, so I made a const.

elexis added inline comments.Apr 24 2018, 12:32 PM

source/ps/scripting/JSInterface_ModIo.cpp
26 ↗	(On Diff #6461)	And the three ones in `VisualReplay.cpp`?

Also do you want to commit this diff? I didn't really earn my name in the svn blame

Itms added inline comments.Apr 24 2018, 2:07 PM

source/ps/scripting/JSInterface_ModIo.cpp
26 ↗	(On Diff #6461)	Well those three ones look like they could have to be easily tweaked by devs in the future. Here the map should be changed only if `DownloadProgressData` is modified, in which case `GetDownloadProgress` would be changed accordingly. Frankly this is not a big issue, I just feel like this map should be strongly associated with `GetDownloadProgress`.