Page MenuHomeWildfire Games

Suggest saveSettingAndWriteToUserConfig() function
ClosedPublic

Authored by ffffffff on Jan 9 2018, 1:47 PM.

Details

Summary

Proposing function saveSettingAndWriteToUserConfig(setting, value)

(Coming up in D1113)

There is also one ConfigDB_CreateValue and ConfigDB_WriteFile engine call in modmod.js but im not sure how to generalize also here. Maybe must be put in a new or existing file in under mods/mod/gui/common like guitags.js.

Test Plan

may test changed positions in files (guis)

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

ffffffff edited the summary of this revision. (Show Details)Jan 9 2018, 1:49 PM
bb added a subscriber: leper.Jan 9 2018, 10:51 PM
bb added inline comments.
binaries/data/mods/public/gui/common/functions_utility.js
11

Write setting with its value in the user config file.

missing space before *

12

also space

13

Maybe name writeSettingToUserConfig

binaries/data/mods/public/gui/gamesetup/gamesetup.js
1018–1021

nice finds

1019–1020

@leper do you know why we call both functions, isn't the second one implying the first? (with seeing the SetStringValue call in ConfigDB L451)
Also for all cases below

1020

unneeded braces

1216

inlinable

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
284

looks like redundant with the proposed name

287

same

binaries/data/mods/public/gui/session/menu.js
344

redundant

345

inlinable, while on it nuke the foo.toString to String(foo)

ffffffff added inline comments.Jan 9 2018, 10:56 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1019–1020

(thought the same want to ask later :( ) also ther is Engine.ConfigDB_WriteFile function

leper added inline comments.
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1019–1020

Yes. No.

bb added inline comments.Jan 9 2018, 11:12 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1019–1020

Why do we call both functions?, what part of the first is not implied in the second?

ffffffff added inline comments.Jan 9 2018, 11:17 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1019–1020

Im thinking it sets internal var and writes it into file

bb added inline comments.Jan 9 2018, 11:35 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1019–1020

ah got it, my first test (chat extension) case simply failed (due to chat window is inside the session and stores the value there), however instincts on IRC were right:
(22:15:27) bb: the first sets it temporarily, the second for permanent in the file
(22:26:09) bb: WriteValueToFile writes to file (and not to memory), CreateValue writes to memory (and not file)
confirmed with using the STUN option in gamesetup_mp

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
275

maybe nuke the whole function

ffffffff updated this revision to Diff 5213.Jan 10 2018, 3:44 AM
ffffffff retitled this revision from Suggest setConfigDb() function to Suggest writeSettingToUserConfig() function.
ffffffff edited the summary of this revision. (Show Details)
ffffffff added inline comments.Jan 11 2018, 2:37 PM
binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
275

Put into confirmSetup() where it is called?

bb added inline comments.Jan 11 2018, 6:11 PM
binaries/data/mods/public/gui/common/functions_utility.js
13

slightly worried about the name now, seeing the above observation, don't only write it to the file but also change it for the current instance. Maybe we need to go for saveSettingAndWriteToUserConfig (sorry for not mentioning earlier)

Same goes for the comment ofc

ffffffff added inline comments.Jan 12 2018, 6:20 PM
binaries/data/mods/public/gui/common/functions_utility.js
13

As long as we only use one function in general that is often used for setting a user config setting, why not giving that an easy general name f.e. writeToUserConfig or writeToConfig(name, value) use this generaly and explain in js doc that it is updating config in cache and write it into file?

Same would be for D1216 an easy general name readConfigSetting or readFromConfig or getConfig(name).

Not seeing why we need big name for an easy function.

Shall make things easier not more complicating in big names.

ffffffff added inline comments.
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1019–1020

it seems, that a simple

Engine.ConfigDB_CreateValue("user", setting, value);
Engine.ConfigDB_WriteFile("user", "config/user.cfg");

is sufficient instead of

Engine.ConfigDB_CreateValue("user", setting, value);
Engine.ConfigDB_WriteValueToFile("user", setting, value, "config/user.cfg");

correct @leper ?

leper added inline comments.
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1019–1020

No.

ffffffff added inline comments.Jan 13 2018, 1:05 AM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1019–1020

Why its working for me

ffffffff added inline comments.Jan 13 2018, 1:45 AM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1019–1020

using it here D210 lobby.js L717. seems work

mimo added a subscriber: mimo.Jan 13 2018, 11:20 AM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1019–1020

Both functions have a different purpose:
Engine.ConfigDB_WriteValueToFile adds the given value to the file
Engine.ConfigDB_WriteFile writes all current values in memory to the file. That's something we usually don't want to do (people may have set some options only for the current session and do not want to have them saved in their config file behind their back). So it is also certainly wrong to use it in D210 (although i've not looked at the code). Engine.ConfigDB_WriteFile should in principle only be used when clicking the save button while you are in the option panel.

ffffffff added inline comments.Jan 13 2018, 2:08 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1019–1020

Tx!:)

ffffffff updated this revision to Diff 5440.Jan 23 2018, 10:26 AM
ffffffff retitled this revision from Suggest writeSettingToUserConfig() function to Suggest saveSettingAndWriteToUserConfig() function.
ffffffff edited the summary of this revision. (Show Details)

Name like bb suggested see now its true right.

bb accepted this revision.Jan 27 2018, 9:47 PM

Patch looks correct and complete, thanks for the cleanup

This revision is now accepted and ready to land.Jan 27 2018, 9:47 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Jan 27 2018, 9:59 PM