Page MenuHomeWildfire Games

Fix missing translate call in modmod
Needs ReviewPublic

Authored by Imarok on Sep 1 2018, 11:39 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Trac Tickets
#5155
Summary
Test Plan

agree on the choosen context

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6449
Build 10683: Vulcan BuildJenkins
Build 10682: arc lint + arc unit

Event Timeline

Imarok created this revision.Sep 1 2018, 11:39 AM
Vulcan added a subscriber: Vulcan.Sep 1 2018, 11:40 AM

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
| 367| 367| 
| 368| 368| 	g_ModsEnabled.sort((folder1, folder2) =>
| 369| 369| 		dependencies[folder1].indexOf(g_Mods[folder2].name) != -1 ? 1 :
| 370|    |-		dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
|    | 370|+			dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
| 371| 371| 
| 372| 372| 	displayModList("modsEnabledList", g_ModsEnabled);
| 373| 373| }

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

elexis updated the Trac tickets for this revision.Sep 1 2018, 3:16 PM
elexis added a subscriber: elexis.Sep 1 2018, 3:20 PM
elexis added inline comments.
binaries/data/mods/mod/gui/modmod/modmod.js
372

Since there are likely no other Enable/Disable strings in the modselecton screen and since the modselection screen is the only page in the transifex resource, it might work without the context too. But not wrong, good to add it in advance.

elexis added a comment.Sep 1 2018, 3:27 PM

While proofreading the modmod files for completeness, I found that the default caption of this button is inconsistent. It is shown when the button is disabled by default, but the caption remains Enabled if there is no mod selected. It would be more consistent to always have the same state if there is no mod selected.
I don't see any other i18n violation in these two files.

Vulcan added a comment.Sep 2 2018, 8:48 PM

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
| 367| 367| 
| 368| 368| 	g_ModsEnabled.sort((folder1, folder2) =>
| 369| 369| 		dependencies[folder1].indexOf(g_Mods[folder2].name) != -1 ? 1 :
| 370|    |-		dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
|    | 370|+			dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
| 371| 371| 
| 372| 372| 	displayModList("modsEnabledList", g_ModsEnabled);
| 373| 373| }

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

In D1623#64723, @elexis wrote:

While proofreading the modmod files for completeness, I found that the default caption of this button is inconsistent. It is shown when the button is disabled by default, but the caption remains Enabled if there is no mod selected. It would be more consistent to always have the same state if there is no mod selected.

Good catch!

Imarok updated this revision to Diff 6984.Nov 10 2018, 10:58 PM

Fix the inconsistency noticed by elexis for the Enable/Disable button, the website button and the arrows.

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
| 351| 351| 
| 352| 352| 	g_ModsEnabled.sort((folder1, folder2) =>
| 353| 353| 		dependencies[folder1].indexOf(g_Mods[folder2].name) != -1 ? 1 :
| 354|    |-		dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
|    | 354|+			dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
| 355| 355| 
| 356| 356| 	displayModList("modsEnabledList", g_ModsEnabled);
| 357| 357| }

Link to build: https://jenkins.wildfiregames.com/job/differential/782/

elexis added inline comments.Nov 11 2018, 2:22 AM
binaries/data/mods/mod/gui/modmod/modmod.js
379
  • What is the point of the third string? If it's to inform the user that this button has dual use, then it seems wrong to not inform the user of the dual-use once an item is selected. To me the simpler solution seems favorable.
  • If this string should be used, the copy in the XML should be removed, so that it isn't a surprise anymore that there are strings in different files for the same UI object. In particular if someone gets the idea to change one of the two copies of this string, the other copy will remain unedited.
Imarok marked an inline comment as done.Nov 11 2018, 8:57 AM
Imarok added inline comments.
binaries/data/mods/mod/gui/modmod/modmod.js
379

What else should the caption of the button be if no mod has been selected?

elexis added inline comments.Mon, Nov 12, 4:30 PM
binaries/data/mods/mod/gui/modmod/modmod.js
379

Why not just KISS and use "Enable" or "Disable" with the button being disabled if nothing is selected? It's accurate because the user cannot enable or disable if nothing is selected. It's the same behavior with all our other buttons that are disabled conditionally, no?

Imarok marked an inline comment as done.Mon, Nov 12, 7:25 PM
Imarok added inline comments.
binaries/data/mods/mod/gui/modmod/modmod.js
379

It's the same behavior with all our other buttons that are disabled conditionally, no?

I don't think we have much buttons that show different captions and are sometimes disabled...

elexis added inline comments.Mon, Nov 12, 9:10 PM
binaries/data/mods/mod/gui/modmod/modmod.js
379

It would be the first button that shows a different caption depending on it's enabled property.

Imarok marked an inline comment as done.Sat, Nov 17, 7:35 PM
Imarok added inline comments.
binaries/data/mods/mod/gui/modmod/modmod.js
379

I think it's nicer with the third string, and I mean it doesn't really hurt doing it, right?