HomeWildfire Games

Create a saveSettingAndWriteToUserConfig() function to absorb some duplication…
Concern RaisedrP21036

Description

Create a saveSettingAndWriteToUserConfig() function to absorb some duplication in the gui

Patch By: ffffffff
Comments By: leper, mimo
Differential Revision: https://code.wildfiregames.com/D1211

Details

Auditors
elexis
Committed
bbJan 27 2018, 9:59 PM
Differential Revision
D1211: Suggest saveSettingAndWriteToUserConfig() function
Parents
rP21035: New spears by Alexandermb
Branches
Unknown
Tags
Unknown
Build Status
Buildable 4695
Build 8155: Post-Commit BuildJenkins

Event Timeline

elexis added a subscriber: elexis.Jan 27 2018, 10:07 PM
elexis added inline comments.
/ps/trunk/binaries/data/mods/public/gui/session/menu.js
353

that String conversion could be absorbed too, not sure if it removes some clarity

bb added inline comments.Jan 27 2018, 10:10 PM
/ps/trunk/binaries/data/mods/public/gui/session/menu.js
353

would look a bit odd, when the value already is a string ~75% of the calls

elexis raised a concern with this commit.Oct 30 2018, 11:52 AM

Remove the concern if you disagree with this being worthy of a concern. While the patch gave the advantage of shortening the code, every line of the patch should be different:

  • Requires public mod: mods/mod/gui/common/terms.js also contains a ConfigDB_CreateValue + ConfigDB_WriteValueToFile call but can't reuse the public/ function.
  • Wrong programming language: The new function has no relation to it's immediate neighbors (sound notifications and civ data) nor to these config functions that relate to a specific setting. If this function was added to JSInterface_ConfigDB.cpp, it would be related to every other neighboring function.
  • Not reusable by design: In the sense of the JSInterface_ConfigDB` and #5046, the function should also be usable with other configuration namespaces. Therefore the name User should be removed from the function name and a namespace argument added.

About the repetition of config/user.cfg in the previous ConfigDB_WriteValueToFile calls, maybe it could be considered a bug, not a feature that one can call the function with different paths but the same namespace. At least it could use the default filepath if no filepath was specified.

So

saveSettingAndWriteToUserConfig("gui.splashscreen.version", Engine.GetFileMTime("gui/splashscreen/splashscreen.txt"));

were

Engine.ConfigDB_CreateAndWriteValueToFile("user", "gui.splashscreen.version", Engine.GetFileMTime("gui/splashscreen/splashscreen.txt"));
This commit now has outstanding concerns.Oct 30 2018, 11:52 AM
elexis added inline comments.Oct 30 2018, 11:56 AM
/ps/trunk/binaries/data/mods/public/gui/session/menu.js
353

(CreateValue always receives a string and config values are stored strings. So the explicit conversion seemed redundant with the implicit conversion. But the implicit conversion complains for booleans (not for numbers).)