Page MenuHomeWildfire Games
Differential D1623

Fix button caption and enable in modmod
ClosedPublic
Actions

Authored by Imarok on Sep 1 2018, 11:39 AM.

Details

Reviewers
None
Commits
rP23506: Fix button caption and enable in modmod
Trac Tickets
#5155
Summary

Some buttons are enabled when they shouldn't and show the wrong caption when no mod selected.

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 7299
Build 11886: Vulcan BuildJenkins
Build 11885: 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

Harbormaster failed remote builds in B6352: Diff 6877!Sep 1 2018, 11:40 AM
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

Harbormaster completed remote builds in B6352: Diff 6877.Sep 2 2018, 8:48 PM
Imarok added a comment.Nov 10 2018, 10:51 PM
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.

Vulcan added a comment.Nov 10 2018, 11:03 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
| 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/

Harbormaster completed remote builds in B6449: Diff 6984.Nov 10 2018, 11:03 PM
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.Nov 12 2018, 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.Nov 12 2018, 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.Nov 12 2018, 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.Nov 17 2018, 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?

elexis mentioned this in rP21960: Fix some untranslated strings, fixes #5155..Dec 28 2018, 2:21 PM
Itms added a subscriber: Itms.Dec 28 2018, 2:28 PM

Ah sorry for missing this patch, I committed the simplest fix for #5155, which was just adding the translate call.

So this patch needs a rebase and a new description if the logic change for this button is agreed upon.

Imarok updated this revision to Diff 7854.Apr 25 2019, 7:01 PM

Rebase. Disable Website button if selected mod has no url.

Vulcan added a comment.Apr 25 2019, 7:02 PM

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

Linter detected issues:
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| }
Executing section cli...

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

Harbormaster completed remote builds in B7298: Diff 7854.Apr 25 2019, 7:02 PM
Imarok retitled this revision from Fix missing translate call in modmod to Fix button caption and enable in modmod.Apr 25 2019, 7:03 PM
Imarok edited the summary of this revision. (Show Details)
Stan added a subscriber: Stan.Apr 25 2019, 7:08 PM
Stan added inline comments.
binaries/data/mods/mod/gui/modmod/modmod.js
354

Could fix the warning here while at it :)

Imarok updated this revision to Diff 7855.Apr 25 2019, 7:12 PM
Imarok marked an inline comment as done.

Fix indent spotted by Stan

Vulcan added a comment.Apr 25 2019, 7:13 PM

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

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

Harbormaster completed remote builds in B7299: Diff 7855.Apr 25 2019, 7:13 PM
Imarok updated this revision to Diff 11371.Feb 16 2020, 3:52 PM

Rebase, Only two states for Enable/Disable button, delete caption of button in xml file

Imarok added a subscriber: Silier.Feb 16 2020, 3:54 PM
Imarok added inline comments.
binaries/data/mods/mod/gui/modmod/modmod.js
354

Good catch.

Vulcan added a comment.Feb 16 2020, 3:59 PM

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1774/display/redirect

Harbormaster completed remote builds in B11145: Diff 11371.Feb 16 2020, 3:59 PM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 17 2020, 7:23 PM
Closed by commit rP23506: Fix button caption and enable in modmod (authored by Imarok). · Explain Why
This revision was automatically updated to reflect the committed changes.
Imarok added a commit: rP23506: Fix button caption and enable in modmod.

Revision Contents
Changeset List

PathSize
binaries/
data/
mods/
mod/
gui/
modmod/
35 lines

Diff 7855

View Options

binaries/data/mods/mod/gui/modmod/modmod.js

Loading...
Wildfire Games · Phabricator