Page MenuHomeWildfire Games
Differential D573

Add a repair time tooltip
ClosedPublic
Actions

Authored by temple on May 29 2017, 6:13 AM.

Details

Reviewers
bb
Commits
rP21121: Add a repair time tooltip
Summary

We have a tooltip that shows the time to finish constructing a building. It makes sense that we should also have one showing the time to finish repairing a building or unit. See the screenshot below.

The code is basically copied from D522 (and D572). So if further changes are made to that one then the same changes should be made here.

An extra thing in the repair code is repairTimeRatio, which accounts for buildings taking longer to repair than they took to construct.

I think this tooltip would be a useful addition, but let me know what you think.

Test Plan

Test that the tooltip time matches the actual repair time.

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

temple created this revision.May 29 2017, 6:13 AM
temple mentioned this in D522: Fixing the build time tooltip.May 29 2017, 7:03 AM
temple updated this revision to Diff 2322.May 30 2017, 9:57 PM
temple added a reviewer: fatherbushido.

Updated to mirror my changes in D522.

temple updated this revision to Diff 2328.May 31 2017, 2:05 AM

Moved the times to extended entity state.

temple updated this revision to Diff 2330.May 31 2017, 5:19 PM

Fixed test.

fatherbushido resigned from this revision.Jul 29 2017, 11:28 AM
fatherbushido removed a reviewer: fatherbushido.Oct 23 2017, 5:11 PM

(It will be more efficient if bb_ reviews it as he commited the other one.)

ffffffff added a subscriber: ffffffff.Jan 5 2018, 3:43 PM

The tooltip is very good. For me the number of workers can be shown in tooltip too, as the time to complete repair (from D572) is much more important.

ffffffff awarded a token.Jan 5 2018, 3:43 PM
ffffffff mentioned this in D572: Combine the "time to completion" and "time speed-up" tooltips.Jan 21 2018, 9:01 PM
temple updated this revision to Diff 5453.Jan 23 2018, 7:30 PM

Updated to mirror D572.

Harbormaster failed remote builds in B4639: Diff 5453!Jan 23 2018, 7:30 PM
Imarok added a subscriber: Imarok.Jan 28 2018, 11:00 PM
In D573#50633, @temple wrote:

Updated to mirror D572.

getRepairTimeTooltip here and getBuildTimeTooltipin D572 look quite similar. Already thought about merging both into one generic function? (I know, there are many strings in it...)

temple updated this revision to Diff 5581.Jan 30 2018, 1:18 AM

rebase, remove the repair rate tooltip

temple added a comment.Jan 30 2018, 1:27 AM
In D573#51352, @Imarok wrote:

getRepairTimeTooltip here and getBuildTimeTooltipin D572 look quite similar. Already thought about merging both into one generic function? (I know, there are many strings in it...)

I guess I thought "construction" and "repair" were different enough to warrant different words.
Maybe in the future the foundation and repairable components could be combined (or something) to reduce the amount of duplicated code.

binaries/data/mods/public/gui/session/selection_details.js
253 ↗(On Diff #5581)

don't need this check, since it's in the else if

Vulcan added a subscriber: Vulcan.Jan 30 2018, 4:09 AM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Jan 30 2018, 4:10 AM
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Mirage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Mirage.js
|  70|  70| 	this.classesList = cmpIdentity.GetClassesList();
|  71|  71| };
|  72|  72| 
|  73|    |-Mirage.prototype.GetClassesList = function() { return this.classesList };
|    |  73|+Mirage.prototype.GetClassesList = function() { return this.classesList; };
|  74|  74| 
|  75|  75| // Foundation data
|  76|  76| 

binaries/data/mods/public/simulation/components/Mirage.js
|  73| Mirage.prototype.GetClassesList·=·function()·{·return·this.classesList·};
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
| 364| 364| 		});
| 365| 365| 
| 366| 366| 	return sprintf(translate("%(label)s %(details)s"), {
| 367|    |-			"label": headerFont(translate("Number of repairers:")),
|    | 367|+		"label": headerFont(translate("Number of repairers:")),
| 368| 368| 			"details": entState.repairable.numBuilders
| 369| 369| 		}) + "\n" +
| 370| 370| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
| 365| 365| 
| 366| 366| 	return sprintf(translate("%(label)s %(details)s"), {
| 367| 367| 			"label": headerFont(translate("Number of repairers:")),
| 368|    |-			"details": entState.repairable.numBuilders
|    | 368|+		"details": entState.repairable.numBuilders
| 369| 369| 		}) + "\n" +
| 370| 370| 		sprintf(translatePlural(
| 371| 371| 			"Add another worker to speed up the repairs by %(second)s second.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
| 366| 366| 	return sprintf(translate("%(label)s %(details)s"), {
| 367| 367| 			"label": headerFont(translate("Number of repairers:")),
| 368| 368| 			"details": entState.repairable.numBuilders
| 369|    |-		}) + "\n" +
|    | 369|+	}) + "\n" +
| 370| 370| 		sprintf(translatePlural(
| 371| 371| 			"Add another worker to speed up the repairs by %(second)s second.",
| 372| 372| 			"Add another worker to speed up the repairs by %(second)s seconds.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
| 388| 388| 		});
| 389| 389| 
| 390| 390| 	return sprintf(translate("%(label)s %(details)s"), {
| 391|    |-			"label": headerFont(translate("Number of builders:")),
|    | 391|+		"label": headerFont(translate("Number of builders:")),
| 392| 392| 			"details": entState.foundation.numBuilders
| 393| 393| 		}) + "\n" +
| 394| 394| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public
Harbormaster completed remote builds in B4745: Diff 5581.Jan 30 2018, 4:11 AM
bb added a subscriber: bb.Jan 30 2018, 8:20 PM
bb added inline comments.
binaries/data/mods/public/gui/common/tooltips.js
359–364 ↗(On Diff #5581)

where can I see this tooltip ingame?

367–373 ↗(On Diff #5581)

vulcan is yelling about the indentation, but actually code is correct

ffffffff added a comment.Jan 30 2018, 8:45 PM

Why not shown same as in build as time directly in panel?

temple added inline comments.Jan 31 2018, 1:39 AM
binaries/data/mods/public/gui/common/tooltips.js
359–364 ↗(On Diff #5581)

Nowhere, after it was removed from the health tooltip. So why don't we show the icon when a building or unit is repairable (and then the tooltip will show this message if there's no repairers yet)? There's an entState.needsRepair already.

temple updated this revision to Diff 5598.Jan 31 2018, 1:40 AM
Vulcan added a comment.Jan 31 2018, 2:47 AM
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Mirage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Mirage.js
|  70|  70| 	this.classesList = cmpIdentity.GetClassesList();
|  71|  71| };
|  72|  72| 
|  73|    |-Mirage.prototype.GetClassesList = function() { return this.classesList };
|    |  73|+Mirage.prototype.GetClassesList = function() { return this.classesList; };
|  74|  74| 
|  75|  75| // Foundation data
|  76|  76| 

binaries/data/mods/public/simulation/components/Mirage.js
|  73| Mirage.prototype.GetClassesList·=·function()·{·return·this.classesList·};
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
| 364| 364| 		});
| 365| 365| 
| 366| 366| 	return sprintf(translate("%(label)s %(details)s"), {
| 367|    |-			"label": headerFont(translate("Number of repairers:")),
|    | 367|+		"label": headerFont(translate("Number of repairers:")),
| 368| 368| 			"details": entState.repairable.numBuilders
| 369| 369| 		}) + "\n" +
| 370| 370| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
| 365| 365| 
| 366| 366| 	return sprintf(translate("%(label)s %(details)s"), {
| 367| 367| 			"label": headerFont(translate("Number of repairers:")),
| 368|    |-			"details": entState.repairable.numBuilders
|    | 368|+		"details": entState.repairable.numBuilders
| 369| 369| 		}) + "\n" +
| 370| 370| 		sprintf(translatePlural(
| 371| 371| 			"Add another worker to speed up the repairs by %(second)s second.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
| 366| 366| 	return sprintf(translate("%(label)s %(details)s"), {
| 367| 367| 			"label": headerFont(translate("Number of repairers:")),
| 368| 368| 			"details": entState.repairable.numBuilders
| 369|    |-		}) + "\n" +
|    | 369|+	}) + "\n" +
| 370| 370| 		sprintf(translatePlural(
| 371| 371| 			"Add another worker to speed up the repairs by %(second)s second.",
| 372| 372| 			"Add another worker to speed up the repairs by %(second)s seconds.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
| 388| 388| 		});
| 389| 389| 
| 390| 390| 	return sprintf(translate("%(label)s %(details)s"), {
| 391|    |-			"label": headerFont(translate("Number of builders:")),
|    | 391|+		"label": headerFont(translate("Number of builders:")),
| 392| 392| 			"details": entState.foundation.numBuilders
| 393| 393| 		}) + "\n" +
| 394| 394| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public
Vulcan added a comment.Jan 31 2018, 2:50 AM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Harbormaster completed remote builds in B4763: Diff 5598.Jan 31 2018, 2:51 AM
bb accepted this revision.Feb 6 2018, 5:54 PM

Looks about committable in my opinion

2 comments can be done when commiting => accept (rather commit this soonish so translators have as much time as possible to change their strings)

binaries/data/mods/public/gui/common/tooltips.js
359–361 ↗(On Diff #5598)

Maybe add a "No repairers" header
(That would just be adding headerFont(translate("No repairers assigned:")) + "\n" + in front, I guess)

binaries/data/mods/public/simulation/components/Repairable.js
17 ↗(On Diff #5598)

(at some moment this should perhaps become some template setting, but ok for now)

79 ↗(On Diff #5598)

(no period required)

This revision is now accepted and ready to land.Feb 6 2018, 5:54 PM
bb added inline comments.Feb 6 2018, 6:35 PM
binaries/data/mods/public/gui/common/tooltips.js
359–361 ↗(On Diff #5598)

just noticed that the build tooltip doesn't do this, so maybe meh

temple updated this revision to Diff 5685.Feb 6 2018, 8:21 PM

added "number of repairers/builders: 0" when no repairers/builders

Vulcan added a comment.Feb 6 2018, 8:26 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Feb 6 2018, 8:27 PM
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Mirage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Mirage.js
|  70|  70| 	this.classesList = cmpIdentity.GetClassesList();
|  71|  71| };
|  72|  72| 
|  73|    |-Mirage.prototype.GetClassesList = function() { return this.classesList };
|    |  73|+Mirage.prototype.GetClassesList = function() { return this.classesList; };
|  74|  74| 
|  75|  75| // Foundation data
|  76|  76| 

binaries/data/mods/public/simulation/components/Mirage.js
|  73| Mirage.prototype.GetClassesList·=·function()·{·return·this.classesList·};
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
| 355| 355| function getRepairTimeTooltip(entState)
| 356| 356| {
| 357| 357| 	return sprintf(translate("%(label)s %(details)s"), {
| 358|    |-			"label": headerFont(translate("Number of repairers:")),
|    | 358|+		"label": headerFont(translate("Number of repairers:")),
| 359| 359| 			"details": entState.repairable.numBuilders
| 360| 360| 		}) + "\n" + (entState.repairable.numBuilders ?
| 361| 361| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
| 356| 356| {
| 357| 357| 	return sprintf(translate("%(label)s %(details)s"), {
| 358| 358| 			"label": headerFont(translate("Number of repairers:")),
| 359|    |-			"details": entState.repairable.numBuilders
|    | 359|+		"details": entState.repairable.numBuilders
| 360| 360| 		}) + "\n" + (entState.repairable.numBuilders ?
| 361| 361| 		sprintf(translatePlural(
| 362| 362| 			"Add another worker to speed up the repairs by %(second)s second.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
| 357| 357| 	return sprintf(translate("%(label)s %(details)s"), {
| 358| 358| 			"label": headerFont(translate("Number of repairers:")),
| 359| 359| 			"details": entState.repairable.numBuilders
| 360|    |-		}) + "\n" + (entState.repairable.numBuilders ?
|    | 360|+	}) + "\n" + (entState.repairable.numBuilders ?
| 361| 361| 		sprintf(translatePlural(
| 362| 362| 			"Add another worker to speed up the repairs by %(second)s second.",
| 363| 363| 			"Add another worker to speed up the repairs by %(second)s seconds.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/tooltips.js
| 377| 377| function getBuildTimeTooltip(entState)
| 378| 378| {
| 379| 379| 	return sprintf(translate("%(label)s %(details)s"), {
| 380|    |-			"label": headerFont(translate("Number of builders:")),
|    | 380|+		"label": headerFont(translate("Number of builders:")),
| 381| 381| 			"details": entState.foundation.numBuilders
| 382| 382| 		}) + "\n" + (entState.foundation.numBuilders ?
| 383| 383| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected in
Harbormaster completed remote builds in B4826: Diff 5685.Feb 6 2018, 8:27 PM
Closed by commit rP21121: Add a repair time tooltip (authored by temple). · Explain WhyFeb 6 2018, 8:32 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Feb 6 2018, 8:32 PM

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
gui/
common/
50 lines
session/
8 lines
simulation/
components/
10 lines
3 lines
68 lines

Diff 5686

View Options

ps/trunk/binaries/data/mods/public/gui/common/tooltips.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/session/selection_details.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/components/GuiInterface.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/components/Mirage.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/components/Repairable.js

Loading...
Wildfire Games · Phabricator