- Start a game.
- Group some units.
- Make a second group with a part of the first group.
- Verify that the part is not in the first group anymore.
- Flip the switch in the options menu.
- Try again and verify that units can be in multiple control groups.
- Check the replay and verify that the states are equal even when switching the option between successive replays.
Details
Details
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 8846 Build 14509: Vulcan Build Jenkins Build 14508: arc lint + arc unit
Event Timeline
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues:
Executing section Source...
Executing section JS...
binaries/data/mods/public/gui/session/selection.js
| 462| » if·(!(Engine.ConfigDB_GetValue("user",·"gui.session.entitiesinmultiplecontrolgroups")·!=·"false"))
| | [NORMAL] JSHintBear:
| | Confusing use of '!'.
Executing section cli...Link to build: https://jenkins.wildfiregames.com/job/docker-differential/374/display/redirect
| binaries/data/mods/public/gui/session/selection.js | ||
|---|---|---|
| 462 | why do you negate negation to be false? Is not better to ask if it is equal to false? :) | |
Comment Actions
The tooltip and name must be as transparent as possible, it's probably okay. I think I have to accept this unless you want to change it.
| binaries/data/config/default.cfg | ||
|---|---|---|
| 377 | (That's the reason why aligning with spaces is discouraged) (Perhaps we can find a name that is 6 characters shorter without restoring to an abbreviation.) | |
| binaries/data/mods/public/gui/session/selection.js | ||
| 462 | I don't remember pointing that out, but the double negation is indeed unnecessary indirection. | |
Comment Actions
Negeted negation.
I have not yet found a better name, so if anyone has an idea, please share ;)
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/402/display/redirect
| binaries/data/config/default.cfg | ||
|---|---|---|
| 377 | -control? | |
| binaries/data/config/default.cfg | ||
|---|---|---|
| 377 | Abb. [sic] but I'm okay with that. | |
| binaries/data/mods/public/gui/session/selection.js | ||
| 462 |
In the previous situation that would indeed return only one group, however, when changing the option during a game, it could return an array now.
That would indeed be a problem when you'd remove the last entity from a group, since the group would then be removed. Apparently there has been no issues with this before. | |
| binaries/data/mods/public/gui/session/selection.js | ||
|---|---|---|
| 462 |
Well that's why I suggested filter, not find :)
I would say out of scope to change here? Yeah totally, just thought it was worth mentionning in case it could actually fail. | |
| binaries/data/mods/public/gui/session/selection.js | ||
|---|---|---|
| 462 |
Like this? usefulGroups = this.groups.filter(group => ent in group.ent);
for (let group in usefulGroups)
group.remove(ent);This would also remove the problem with removing groups whilst iterating. | |
| binaries/data/mods/public/gui/session/selection.js | ||
|---|---|---|
| 462 | I guess :) Adding a test would make sure of it :) | |
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/19/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/528/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/442/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/957/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/443/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/958/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/466/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/981/display/redirect
| binaries/data/mods/public/gui/options/options.json | ||
|---|---|---|
| 546 | Control Groups Behaviour | |
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/467/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/982/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... Executing section JS... | | [NORMAL] ESLintBear (semi): | | Missing semicolon. |----| | /zpool0/trunk/binaries/data/mods/public/gui/session/selection.js | |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection.js | 443| 443| this.addList([entityID]); | 444| 444| | 445| 445| Engine.CameraMoveTo(entState.position.x, entState.position.z); | 446| |-} | | 446|+}; | 447| 447| | 448| 448| /** | 449| 449| * Cache some quantities which depends only on selection binaries/data/mods/public/gui/session/selection.js | 446| } | | [NORMAL] JSHintBear: | | Missing semicolon. Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1609/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... Executing section JS... | | [NORMAL] ESLintBear (semi): | | Missing semicolon. |----| | /zpool0/trunk/binaries/data/mods/public/gui/session/selection.js | |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection.js | 443| 443| this.addList([entityID]); | 444| 444| | 445| 445| Engine.CameraMoveTo(entState.position.x, entState.position.z); | 446| |-} | | 446|+}; | 447| 447| | 448| 448| /** | 449| 449| * Cache some quantities which depends only on selection binaries/data/mods/public/gui/session/selection.js | 446| } | | [NORMAL] JSHintBear: | | Missing semicolon. Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1699/display/redirect
Comment Actions
(Following discussion on http://irclogs.wildfiregames.com/2020-02/2020-02-02-QuakeNet-%230ad-dev.log and before (on days when a new patch was uploaded))
- There are significant numbers of user groups for each of the selectable values, thus meeting the objective of an option.
- Implementation works as advertized.
- Tooltips / value descriptions and labels thoroughly evaluated and reevaluated over several days and several iterations, making the description as simple to understand as possible while maximizing its accuracy.
- Strings are compliant with EnglishStyleGuide
- Thanks for the patch.