Page MenuHomeWildfire Games

Adds list_data property for dropdown option
ClosedPublic

Authored by Polakrity on May 1 2017, 3:22 PM.

Details

Summary

The "list_data" property for dropdowns in options isn't currently implemented.
ATM the actual values used and written in user.cfg are the indexs of array.

  • Allow to define a specific value to each element of the list (separated label and value).
  • Change the phase notif. with new values (none, completed, all).

Proposals style options.json :

"list": [  { "value": "none", "label": "Disable" },
            { "value": "completed", "label": "Completed" },
            { "value": "all", "label": "All displayed" } ]

or

"list": [ "Disable", "Completed", "All displayed" ], "list_data": [ "none", "completed", "all" ]
Test Plan

Check the value written in user.cfg
Verify if the patch doesn't break the actual phase notif.

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

Polakrity created this revision.May 1 2017, 3:22 PM
Polakrity edited the test plan for this revision. (Show Details)May 1 2017, 3:24 PM
elexis added a subscriber: elexis.May 1 2017, 3:54 PM

Looks like it might work, would improve IMO

binaries/data/config/default.cfg
348 ↗(On Diff #1583)

quotes

Could it be that it will bug for people who have saved a numerical value already? Running into a similar issue with the late observer setting in #4528 :-/

binaries/data/mods/public/gui/options/options.json
286 ↗(On Diff #1583)

[
\t{ "value": ..., "label": ... },
\t{ "value": ..., "label": ... },
\t...
]

messages.json will need an entry so that the string extractor knows that it should extract all labels... Oh well actually it looks like it already does (since we already have a label property). Gotta check

Vulcan added a subscriber: Vulcan.May 1 2017, 6:43 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/956/ for more details.

Polakrity updated this revision to Diff 1794.May 9 2017, 5:40 PM
Polakrity retitled this revision from Better lists for dropdown option to Adds list_data property for dropdown option.
Polakrity edited the summary of this revision. (Show Details)
Polakrity edited the test plan for this revision. (Show Details)

The patch doesn't break the phase notification.
With actual numerical values (0, 1 and 2) only the completed phase is displayed.

If anyone can check if the string extractor works.

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/1107/ for more details.

elexis accepted this revision.May 16 2017, 5:44 PM

Works, complete, translated, backwards-compatible, tested, thanks.

binaries/data/config/default.cfg
350 ↗(On Diff #1794)

Should mention the possible values

binaries/data/mods/public/gui/options/options.js
242 ↗(On Diff #1794)

This let is a bit ugly, since it's still available in the entire function scope,
switch statements should have scope braces most of the time in the individual cases to prevent such "leaks" into the other cases.

binaries/data/mods/public/gui/options/options.json
298 ↗(On Diff #1794)

config and list on a separate line each.
the 3 values on a separate line each as well.

binaries/data/mods/public/l10n/messages.json
313 ↗(On Diff #1794)

correct, since we dont want to translate the entire object recursively (if that is even implemented)

This revision is now accepted and ready to land.May 16 2017, 5:44 PM
elexis added inline comments.May 16 2017, 5:48 PM
binaries/data/mods/public/gui/options/options.js
242 ↗(On Diff #1794)

config would fit better, since selected is mostly (including previously) used for the index

This revision was automatically updated to reflect the committed changes.