- 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
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 9795 Build 16481: Vulcan Build Jenkins Build 16480: Vulcan Build (Windows) Jenkins Build 16479: arc lint + arc unit
Event Timeline
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 | ||
---|---|---|
467–471 | why do you negate negation to be false? Is not better to ask if it is equal to false? :) |
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 | ||
---|---|---|
376 | (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 | ||
467–471 | I don't remember pointing that out, but the double negation is indeed unnecessary indirection. |
Negeted negation.
I have not yet found a better name, so if anyone has an idea, please share ;)
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 | ||
---|---|---|
376 | -control? |
binaries/data/config/default.cfg | ||
---|---|---|
376 | Abb. [sic] but I'm okay with that. | |
binaries/data/mods/public/gui/session/selection.js | ||
467–471 |
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 | ||
---|---|---|
467–471 |
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 | ||
---|---|---|
467–471 |
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 | ||
---|---|---|
467–471 | I guess :) Adding a test would make sure of it :) |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/19/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/528/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/442/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/957/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/443/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/958/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/466/display/redirect
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 |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/467/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/982/display/redirect
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
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
(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.