Page MenuHomeWildfire Games

Replace Options page Renderer functions with generic Function
ClosedPublic

Authored by elexis on Aug 27 2017, 5:57 PM.

Details

Summary

Many of the settings in the option page call an Engine function.
The booleans only support calling functions that have Renderer in their name,
while the other ones support calling arbitrarily named functions.

So it would be more consistent to unify these two flavors,
remove some code (and later we can make that abstract to unify even more, refs D510, D805)

Test Plan

Make sure reverting and saving of options of every type still works as advertized.

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.Aug 27 2017, 5:57 PM
bb accepted this revision.Aug 28 2017, 1:59 PM
bb added a subscriber: bb.

Patch works as advertised, it seems this doesn't break anything in the related revisions, so i guess this can go before.

This revision is now accepted and ready to land.Aug 28 2017, 1:59 PM

The Renderer function was introduced to get rid of eval.
It was discussed the last time in the context of #3737 (only lines matching eval):

2016-01-10-QuakeNet-#0ad-dev.log:19:00 <@leper> that eval code used there is still ugly, probably not that bad, but still
2016-01-11-QuakeNet-#0ad-dev.log:23:12 <@leper> and I still do not like that much eval usage in there
2016-01-11-QuakeNet-#0ad-dev.log:23:13 < mimo> what can we use as a replacement to eval to edit functions on the fly ? 
2016-01-17-QuakeNet-#0ad-dev.log:17:35 < elexis> eval?
2016-01-17-QuakeNet-#0ad-dev.log:17:36 <@leper> yes eval
2016-01-17-QuakeNet-#0ad-dev.log:17:36 < elexis> eval not nice
2016-01-17-QuakeNet-#0ad-dev.log:17:37 <@leper> not that nice, but removes the issues an eval could cause
2016-01-17-QuakeNet-#0ad-dev.log:17:38 < elexis> mimo: the "function" argument is only needed for sound, so maybe look for a better solution than calling eval there
2016-01-17-QuakeNet-#0ad-dev.log:17:39 < elexis> in that case you can remove eval without having to lose the ability to save it in a *.json file
2016-01-17-QuakeNet-#0ad-dev.log:17:40 < mimo> yes, but that code was supposed to be more generic, i.e. to allow more options to call functions.  And as the eval were already in the previous code, cleaning them could be an next ticket
2016-01-17-QuakeNet-#0ad-dev.log:17:47 < elexis> the renderer code also has some eval()s... maybe a lookup table would fit
2016-01-17-QuakeNet-#0ad-dev.log:21:14 <@leper> mimo: checked = eval("Engine.Renderer_Get" + keyRenderer + "Enabled()"); -> checked = Engine["Renderer_Get"+keyRenderer+"Enabled"]();

That goal is still achieved, so we're not removing a good workaround but just unify the workaround. If we ever need more functionality (such as executing two statements or one statement with arbitrary arguments), then we can still add said lookup table to options_somethingelse.js or by converting options.json entirely to a JS file, while keeping the contents as is + adding one function property.

From yesterday:

16:21 < Vladislav> elexis: I agree, I think it's better than the previous hardcoding. Also hardcoding is less flexible. Security has the same level, because if someone will change the json file, he will be able to change something else too.

This revision was automatically updated to reflect the committed changes.