Training time at general and training costs for units with a training limit > 1 but < ∞ are not displayed correctly, when multiple training entities of the same type are selected.
Details
Details
- Reviewers
elexis Itms bb - Commits
- rP19538: Improve training tooltip with multiple selection
- Trac Tickets
- #4264
- select 1 barrack: train time is 10; select 2 barracks: train time is 18 (should be 10)
- set hero limit to 7, select two hero train buildings -> the tooltip and button caption shows the costs for only one hero (should be the twice the costs)
Diff Detail
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
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/102/ for more details.
Comment Actions
Rest coming later.
| binaries/data/mods/public/gui/session/input.js | ||
|---|---|---|
| 1252 ↗ | (On Diff #145) | Pollutes the logic change, should be done in a separate commit IMO so that changes are self-contained, easier to audit and easier to bisect. Also not sure if we shouldn't feel offended having to review such a simple change |
| 1466 ↗ | (On Diff #145) | !limits.canBeAddedCount? |
| 1473 ↗ | (On Diff #145) | Personally I prefer x = max(x, 1) |
| binaries/data/mods/public/gui/session/selection_panels.js | ||
| 1030 ↗ | (On Diff #145) | nooo |
Comment Actions
Tested and works!
I don't have anything to add to elexis' review but since he talked about "the rest", you may want to wait for him before fixing the code bits he mentioned.
| binaries/data/mods/public/gui/session/input.js | ||
|---|---|---|
| 1466 ↗ | (On Diff #145) | Nope, as limits.canBeAddedCount can be 0 |
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/193/ for more details.
Comment Actions
- Thanks for committing the renames separately in rP19150, makes reading this diff much more enjoyable.
- De facto broken:
- Typo: It's Math.max, not max
- Shows the time of the first selected building when selecting two buildings with different training speeds at each building (for example CC & barracks, or barracks & fort after researching the barracks train time upgrade)
- De jure broken: No unit test
- Refs: Encountered #4303 again while increasing the hero limit , but that's also a simulation bug, halfway out of scope.
- Couldn't find the original revision that introduced the getEntityCostTooltip bug, it's been there since ages, likely since its introduction
If you want to address the newly identified bug in a new diff, tell me then I can accept this one.
| binaries/data/mods/public/gui/common/tooltips.js | ||
|---|---|---|
| 329 ↗ | (On Diff #268) | Don't like this ternary. If GetBatchTime returns 0, why would we make it 1? |
| binaries/data/mods/public/gui/session/input.js | ||
| 1444 ↗ | (On Diff #268) | depending on the batch train modifier hotkey? |
| binaries/data/mods/public/gui/common/tooltips.js | ||
|---|---|---|
| 329 ↗ | (On Diff #268) | no it would be 1, when !entity |
Comment Actions
Fix max typo and comment. The different training speed bug will not be part of this diff
Comment Actions
- Shows the time of the first selected building when selecting two buildings with different training speeds at each building (for example CC & barracks, or barracks & fort after researching the barracks train time upgrade)
Then the problem arises, which of the two train times we should show: both, the average, the higher one?
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/433/ for more details.