Page MenuHomeWildfire Games

Standardize "Territory Influence" in tooltips
Needs ReviewPublic

Authored by Krinkle on Sat, Jun 22, 9:33 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#4259
Summary
  • Remove the hardcoded text for the two buildings that had this already hardcoded in their description.
  • Add parsing for the <TerritoryInfluence> tag to the template loader.
  • Add new tooltip function that can return a "Territory Influence" heading with text similar to the previous text that was hardcoded (made more generic so as to not assume it is a monument).
  • Use the new tooltip function for the tech tree (draw.js), for the construction icon (selection_panels.js), for the existing building (selection_details.js).
  • Use the new tooltip function for the tech tree (draw.js).

Fixes #4259.

Test Plan
  • Compile, Run, and play single player as Iberians.
  • In these places, the old text should be gone, and the new heading present instead.
    1. hover the "Revered Monument" construction icon with units selected.
    2. construct the "Revered Monument", select it, and hover its portrait. (use "gift of the gods" cheat to make this easier).
    3. hover "Revered Monument" in the Structure Tree view.
    4. click for "more information" about "Revered Monument" from any of these places
Screenshots at D2005#83574

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
decay-trac4259
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8045
Build 13094: Vulcan BuildJenkins
Build 13093: arc lint + arc unit

Event Timeline

Krinkle created this revision.Sat, Jun 22, 9:33 PM
Owners added a subscriber: Restricted Owners Package.Sat, Jun 22, 9:33 PM
This comment was removed by Krinkle.

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Templates.js
|  87|  87| 		// If the elements are still strings, split them by space or by '+'
|  88|  88| 		if (typeof sublist == "string")
|  89|  89| 			sublist = sublist.split(/[+\s]+/);
|  90|    |-		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1)
|  91|    |-		                    || (c[0] != "!" && classes.indexOf(c) != -1)))
|    |  90|+		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1) ||
|    |  91|+		                    (c[0] != "!" && classes.indexOf(c) != -1)))
|  92|  92| 			return true;
|  93|  93| 	}
|  94|  94| 
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Templates.js
| 314| 314| 	}
| 315| 315| 
| 316| 316| 	if (template.TerritoryInfluence)
| 317|    |-	{
|    | 317|+	
| 318| 318| 		ret.territoryInfluence = {
| 319| 319| 			// false if undefined
| 320| 320| 			// Should match match logic of CCmpTerritoryInfluence.
| 321| 321| 			"root": template.TerritoryInfluence.Root === "true"
| 322| 322| 		};
| 323|    |-	}
|    | 323|+	
| 324| 324| 
| 325| 325| 	if (template.Heal)
| 326| 326| 		ret.heal = {

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 392| 392| function getRepairTimeTooltip(entState)
| 393| 393| {
| 394| 394| 	return sprintf(translate("%(label)s %(details)s"), {
| 395|    |-			"label": headerFont(translate("Number of repairers:")),
|    | 395|+		"label": headerFont(translate("Number of repairers:")),
| 396| 396| 			"details": entState.repairable.numBuilders
| 397| 397| 		}) + "\n" + (entState.repairable.numBuilders ?
| 398| 398| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 393| 393| {
| 394| 394| 	return sprintf(translate("%(label)s %(details)s"), {
| 395| 395| 			"label": headerFont(translate("Number of repairers:")),
| 396|    |-			"details": entState.repairable.numBuilders
|    | 396|+		"details": entState.repairable.numBuilders
| 397| 397| 		}) + "\n" + (entState.repairable.numBuilders ?
| 398| 398| 		sprintf(translatePlural(
| 399| 399| 			"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/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 394| 394| 	return sprintf(translate("%(label)s %(details)s"), {
| 395| 395| 			"label": headerFont(translate("Number of repairers:")),
| 396| 396| 			"details": entState.repairable.numBuilders
| 397|    |-		}) + "\n" + (entState.repairable.numBuilders ?
|    | 397|+	}) + "\n" + (entState.repairable.numBuilders ?
| 398| 398| 		sprintf(translatePlural(
| 399| 399| 			"Add another worker to speed up the repairs by %(second)s second.",
| 400| 400| 			"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/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 414| 414| function getBuildTimeTooltip(entState)
| 415| 415| {
| 416| 416| 	return sprintf(translate("%(label)s %(details)s"), {
| 417|    |-			"label": headerFont(translate("Number of builders:")),
|    | 417|+		"label": headerFont(translate("Number of builders:")),
| 418| 418| 			"details": entState.foundation.numBuilders
| 419| 419| 		}) + "\n" + (entState.foundation.numBuilders ?
| 420| 420| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 415| 415| {
| 416| 416| 	return sprintf(translate("%(label)s %(details)s"), {
| 417| 417| 			"label": headerFont(translate("Number of builders:")),
| 418|    |-			"details": entState.foundation.numBuilders
|    | 418|+		"details": entState.foundation.numBuilders
| 419| 419| 		}) + "\n" + (entState.foundation.numBuilders ?
| 420| 420| 		sprintf(translatePlural(
| 421| 421| 			"Add another worker to speed up the construction by %(second)s second.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 416| 416| 	return sprintf(translate("%(label)s %(details)s"), {
| 417| 417| 			"label": headerFont(translate("Number of builders:")),
| 418| 418| 			"details": entState.foundation.numBuilders
| 419|    |-		}) + "\n" + (entState.foundation.numBuilders ?
|    | 419|+	}) + "\n" + (entState.foundation.numBuilders ?
| 420| 420| 		sprintf(translatePlural(
| 421| 421| 			"Add another worker to speed up the construction by %(second)s second.",
| 422| 422| 			"Add another worker to speed up the construction by %(second)s seconds.",
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 649| 649| function getTerritoryInfluenceTooltip(template)
| 650| 650| {
| 651| 651| 	let text = "";
| 652|    |-	if (template.territoryInfluence && template.territoryInfluence.root) {
|    | 652|+	if (template.territoryInfluence && template.territoryInfluence.root) 
| 653| 653| 		text += sprintf(translate("%(label)s %(text)s"), {
| 654| 654| 			"label": headerFont(translate("Territory Influence:")),
| 655| 655| 			"text": translate("Other buildings in this building's do not decay.")
| 656| 656| 		});
| 657|    |-	}
|    | 657|+	
| 658| 658| 	return text
| 659| 659| }
| 660| 660| 
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 655| 655| 			"text": translate("Other buildings in this building's do not decay.")
| 656| 656| 		});
| 657| 657| 	}
| 658|    |-	return text
|    | 658|+	return text;
| 659| 659| }
| 660| 660| 
| 661| 661| /**

binaries/data/mods/public/gui/common/tooltips.js
| 652| »   if·(template.territoryInfluence·&&·template.territoryInfluence.root)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/gui/common/tooltips.js
| 658| »   return·text
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

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

Krinkle updated this revision to Diff 8580.Sat, Jun 22, 10:05 PM
Krinkle edited the summary of this revision. (Show Details)
Krinkle edited the test plan for this revision. (Show Details)

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Templates.js
|  87|  87| 		// If the elements are still strings, split them by space or by '+'
|  88|  88| 		if (typeof sublist == "string")
|  89|  89| 			sublist = sublist.split(/[+\s]+/);
|  90|    |-		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1)
|  91|    |-		                    || (c[0] != "!" && classes.indexOf(c) != -1)))
|    |  90|+		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1) ||
|    |  91|+		                    (c[0] != "!" && classes.indexOf(c) != -1)))
|  92|  92| 			return true;
|  93|  93| 	}
|  94|  94| 
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Templates.js
| 314| 314| 	}
| 315| 315| 
| 316| 316| 	if (template.TerritoryInfluence)
| 317|    |-	{
|    | 317|+	
| 318| 318| 		ret.territoryInfluence = {
| 319| 319| 			// false if undefined
| 320| 320| 			// Should match match logic of CCmpTerritoryInfluence.
| 321| 321| 			"root": template.TerritoryInfluence.Root === "true"
| 322| 322| 		};
| 323|    |-	}
|    | 323|+	
| 324| 324| 
| 325| 325| 	if (template.Heal)
| 326| 326| 		ret.heal = {

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 392| 392| function getRepairTimeTooltip(entState)
| 393| 393| {
| 394| 394| 	return sprintf(translate("%(label)s %(details)s"), {
| 395|    |-			"label": headerFont(translate("Number of repairers:")),
|    | 395|+		"label": headerFont(translate("Number of repairers:")),
| 396| 396| 			"details": entState.repairable.numBuilders
| 397| 397| 		}) + "\n" + (entState.repairable.numBuilders ?
| 398| 398| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 393| 393| {
| 394| 394| 	return sprintf(translate("%(label)s %(details)s"), {
| 395| 395| 			"label": headerFont(translate("Number of repairers:")),
| 396|    |-			"details": entState.repairable.numBuilders
|    | 396|+		"details": entState.repairable.numBuilders
| 397| 397| 		}) + "\n" + (entState.repairable.numBuilders ?
| 398| 398| 		sprintf(translatePlural(
| 399| 399| 			"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/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 394| 394| 	return sprintf(translate("%(label)s %(details)s"), {
| 395| 395| 			"label": headerFont(translate("Number of repairers:")),
| 396| 396| 			"details": entState.repairable.numBuilders
| 397|    |-		}) + "\n" + (entState.repairable.numBuilders ?
|    | 397|+	}) + "\n" + (entState.repairable.numBuilders ?
| 398| 398| 		sprintf(translatePlural(
| 399| 399| 			"Add another worker to speed up the repairs by %(second)s second.",
| 400| 400| 			"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/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 414| 414| function getBuildTimeTooltip(entState)
| 415| 415| {
| 416| 416| 	return sprintf(translate("%(label)s %(details)s"), {
| 417|    |-			"label": headerFont(translate("Number of builders:")),
|    | 417|+		"label": headerFont(translate("Number of builders:")),
| 418| 418| 			"details": entState.foundation.numBuilders
| 419| 419| 		}) + "\n" + (entState.foundation.numBuilders ?
| 420| 420| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 415| 415| {
| 416| 416| 	return sprintf(translate("%(label)s %(details)s"), {
| 417| 417| 			"label": headerFont(translate("Number of builders:")),
| 418|    |-			"details": entState.foundation.numBuilders
|    | 418|+		"details": entState.foundation.numBuilders
| 419| 419| 		}) + "\n" + (entState.foundation.numBuilders ?
| 420| 420| 		sprintf(translatePlural(
| 421| 421| 			"Add another worker to speed up the construction by %(second)s second.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 416| 416| 	return sprintf(translate("%(label)s %(details)s"), {
| 417| 417| 			"label": headerFont(translate("Number of builders:")),
| 418| 418| 			"details": entState.foundation.numBuilders
| 419|    |-		}) + "\n" + (entState.foundation.numBuilders ?
|    | 419|+	}) + "\n" + (entState.foundation.numBuilders ?
| 420| 420| 		sprintf(translatePlural(
| 421| 421| 			"Add another worker to speed up the construction by %(second)s second.",
| 422| 422| 			"Add another worker to speed up the construction by %(second)s seconds.",
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 649| 649| function getTerritoryInfluenceTooltip(template)
| 650| 650| {
| 651| 651| 	let text = "";
| 652|    |-	if (template.territoryInfluence && template.territoryInfluence.root) {
|    | 652|+	if (template.territoryInfluence && template.territoryInfluence.root) 
| 653| 653| 		text += sprintf(translate("%(label)s %(text)s"), {
| 654| 654| 			"label": headerFont(translate("Territory:")),
| 655| 655| 			"text": translate("Buildings in this building's territory do not decay.")
| 656| 656| 		});
| 657|    |-	}
|    | 657|+	
| 658| 658| 	return text
| 659| 659| }
| 660| 660| 
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 655| 655| 			"text": translate("Buildings in this building's territory do not decay.")
| 656| 656| 		});
| 657| 657| 	}
| 658|    |-	return text
|    | 658|+	return text;
| 659| 659| }
| 660| 660| 
| 661| 661| /**

binaries/data/mods/public/gui/common/tooltips.js
| 652| »   if·(template.territoryInfluence·&&·template.territoryInfluence.root)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/gui/common/tooltips.js
| 658| »   return·text
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/selection_panels.js
| 453| 453| 				continue;
| 454| 454| 
| 455| 455| 			if (state.pack.progress == 0)
| 456|    |-			{
|    | 456|+			
| 457| 457| 				if (state.pack.packed)
| 458| 458| 					checks.unpackButton = true;
| 459| 459| 				else
| 460| 460| 					checks.packButton = true;
| 461|    |-			}
|    | 461|+			
| 462| 462| 			else if (state.pack.packed)
| 463| 463| 				checks.unpackCancelButton = true;
| 464| 464| 			else
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/selection_panels.js
| 764| 764| 				addResearchToQueue(data.item.researchFacilityId, t);
| 765| 765| 			})(tech);
| 766| 766| 
| 767|    |-			button.onPressRight = (t => function () {
|    | 767|+			button.onPressRight = (t => function() {
| 768| 768| 				showTemplateDetails(
| 769| 769| 					t,
| 770| 770| 					GetTemplateData(data.unitEntStates.find(state => state.id == data.item.researchFacilityId).template).nativeCiv);
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/selection_panels.js
| 942| 942| 			"player": data.player
| 943| 943| 		});
| 944| 944| 
| 945|    |-		let unitIds = data.unitEntStates.map(status => status.id)
|    | 945|+		let unitIds = data.unitEntStates.map(status => status.id);
| 946| 946| 		let [buildingsCountToTrainFullBatch, fullBatchSize, remainderBatch] =
| 947| 947| 			getTrainingStatus(unitIds, data.item, data.playerState);
| 948| 948| 

binaries/data/mods/public/gui/session/selection_panels.js
|  48| »   »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|  59| »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
| 732| »   »   »   »   »   »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
| 945| »   »   let·unitIds·=·data.unitEntStates.map(status·=>·status.id)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

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

BeforeAfter

Note that I choose to not show territory information in tech tree tooltips (it shows only in the construction panel and building details tooltips).

This is for consistency with other primarily textual info such as "Auras" and "Classes". Those are also present in panel/details tooltips, but absent from tech tree tooltips. Instead, they are shown in the body info of the tech tree "Information" dialogs.

The tech tree has a hardcoded relation between what it shown crammed the upper area of "Information" dialogs and in tech tree tooltips. So the choices are:

  1. Have it be in all three kinds of tooltips, and in the upper area of the Information dialog. (this would be inconsistent with what we do for Auras and Classes.)
  2. Have it be in all three kinds of tooltips, and twice in the Information dialog. (Inconsistent, and would be the first time we do that for anything, also Ugh, duplicate info).
  3. Have it be in the tooltip for construction and building details, absent from tech tree tooltip, and in the body of Information dialog. (This is what we do for Auras and Classes).

The commit currently does the latter (Option 3).

Below is what Option 1 would look like:

Option 1 (alternate)
Krinkle updated this revision to Diff 8581.Sat, Jun 22, 10:36 PM
Krinkle edited the summary of this revision. (Show Details)

(resolved TODO)

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 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 392| 392| function getRepairTimeTooltip(entState)
| 393| 393| {
| 394| 394| 	return sprintf(translate("%(label)s %(details)s"), {
| 395|    |-			"label": headerFont(translate("Number of repairers:")),
|    | 395|+		"label": headerFont(translate("Number of repairers:")),
| 396| 396| 			"details": entState.repairable.numBuilders
| 397| 397| 		}) + "\n" + (entState.repairable.numBuilders ?
| 398| 398| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 393| 393| {
| 394| 394| 	return sprintf(translate("%(label)s %(details)s"), {
| 395| 395| 			"label": headerFont(translate("Number of repairers:")),
| 396|    |-			"details": entState.repairable.numBuilders
|    | 396|+		"details": entState.repairable.numBuilders
| 397| 397| 		}) + "\n" + (entState.repairable.numBuilders ?
| 398| 398| 		sprintf(translatePlural(
| 399| 399| 			"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/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 394| 394| 	return sprintf(translate("%(label)s %(details)s"), {
| 395| 395| 			"label": headerFont(translate("Number of repairers:")),
| 396| 396| 			"details": entState.repairable.numBuilders
| 397|    |-		}) + "\n" + (entState.repairable.numBuilders ?
|    | 397|+	}) + "\n" + (entState.repairable.numBuilders ?
| 398| 398| 		sprintf(translatePlural(
| 399| 399| 			"Add another worker to speed up the repairs by %(second)s second.",
| 400| 400| 			"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/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 414| 414| function getBuildTimeTooltip(entState)
| 415| 415| {
| 416| 416| 	return sprintf(translate("%(label)s %(details)s"), {
| 417|    |-			"label": headerFont(translate("Number of builders:")),
|    | 417|+		"label": headerFont(translate("Number of builders:")),
| 418| 418| 			"details": entState.foundation.numBuilders
| 419| 419| 		}) + "\n" + (entState.foundation.numBuilders ?
| 420| 420| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 415| 415| {
| 416| 416| 	return sprintf(translate("%(label)s %(details)s"), {
| 417| 417| 			"label": headerFont(translate("Number of builders:")),
| 418|    |-			"details": entState.foundation.numBuilders
|    | 418|+		"details": entState.foundation.numBuilders
| 419| 419| 		}) + "\n" + (entState.foundation.numBuilders ?
| 420| 420| 		sprintf(translatePlural(
| 421| 421| 			"Add another worker to speed up the construction by %(second)s second.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 416| 416| 	return sprintf(translate("%(label)s %(details)s"), {
| 417| 417| 			"label": headerFont(translate("Number of builders:")),
| 418| 418| 			"details": entState.foundation.numBuilders
| 419|    |-		}) + "\n" + (entState.foundation.numBuilders ?
|    | 419|+	}) + "\n" + (entState.foundation.numBuilders ?
| 420| 420| 		sprintf(translatePlural(
| 421| 421| 			"Add another worker to speed up the construction by %(second)s second.",
| 422| 422| 			"Add another worker to speed up the construction by %(second)s seconds.",
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/selection_panels.js
| 453| 453| 				continue;
| 454| 454| 
| 455| 455| 			if (state.pack.progress == 0)
| 456|    |-			{
|    | 456|+			
| 457| 457| 				if (state.pack.packed)
| 458| 458| 					checks.unpackButton = true;
| 459| 459| 				else
| 460| 460| 					checks.packButton = true;
| 461|    |-			}
|    | 461|+			
| 462| 462| 			else if (state.pack.packed)
| 463| 463| 				checks.unpackCancelButton = true;
| 464| 464| 			else
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/selection_panels.js
| 764| 764| 				addResearchToQueue(data.item.researchFacilityId, t);
| 765| 765| 			})(tech);
| 766| 766| 
| 767|    |-			button.onPressRight = (t => function () {
|    | 767|+			button.onPressRight = (t => function() {
| 768| 768| 				showTemplateDetails(
| 769| 769| 					t,
| 770| 770| 					GetTemplateData(data.unitEntStates.find(state => state.id == data.item.researchFacilityId).template).nativeCiv);
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/selection_panels.js
| 942| 942| 			"player": data.player
| 943| 943| 		});
| 944| 944| 
| 945|    |-		let unitIds = data.unitEntStates.map(status => status.id)
|    | 945|+		let unitIds = data.unitEntStates.map(status => status.id);
| 946| 946| 		let [buildingsCountToTrainFullBatch, fullBatchSize, remainderBatch] =
| 947| 947| 			getTrainingStatus(unitIds, data.item, data.playerState);
| 948| 948| 

binaries/data/mods/public/gui/session/selection_panels.js
|  48| »   »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|  59| »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
| 732| »   »   »   »   »   »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
| 945| »   »   let·unitIds·=·data.unitEntStates.map(status·=>·status.id)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Templates.js
|  87|  87| 		// If the elements are still strings, split them by space or by '+'
|  88|  88| 		if (typeof sublist == "string")
|  89|  89| 			sublist = sublist.split(/[+\s]+/);
|  90|    |-		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1)
|  91|    |-		                    || (c[0] != "!" && classes.indexOf(c) != -1)))
|    |  90|+		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1) ||
|    |  91|+		                    (c[0] != "!" && classes.indexOf(c) != -1)))
|  92|  92| 			return true;
|  93|  93| 	}
|  94|  94| 

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.
Executing section cli...

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

Krinkle added inline comments.Sat, Jun 22, 11:00 PM
binaries/data/mods/public/gui/reference/viewer/viewer.js
21

I swapped these for consistency with selection details and selection panel.

Nescio added a subscriber: Nescio.Sun, Jun 23, 5:59 PM

Although I'm not opposed to the idea per se, I'm not convinced the proposed implementation would really be an improvement.

To start with, the proposed string seems unnecessarily verbose: "Territory: Buildings in this building's territory do not decay." What we're interested in is the fact it has a territory root, which can be conveyed with the much shorter "Territory root: true". And an entity could have territory influence but no root (the majority of structures). If there is a separate territory influence line, then perhaps display radius and weight as well, e.g. "Territory: root: true; radius: 38; weight: 40000."
However, we probably don't want the tooltip to clutter up with unimportant information. That's why we display <CapturePoints>, but not <Capturable/RegenRate> or <Capturable/GarrisonRegenRate>. So the question is whether territory influence is important enough for the typical player to warrant its own tooltip line.
Moreover, the fact a structure has a territory root can be dealt with by two words in the tooltip text (“Territory root.”), or perhaps the addition of a visible class (“TerritoryRoot”).

By the way, the first parts of the monument and pillar <Tooltip> can be removed, because auras now have their own tooltips displayed.
There are several other structures with a territory root (grep -r '<Root>true</Root>') you might want to check.

In D2005#83597, @Nescio wrote:

Although I'm not opposed to the idea per se, I'm not convinced the proposed implementation would really be an improvement.

We currently show this info for two seemingly random buildings only (one civ's monument, and one civ's pillar). For all other buildings with territory root, this information is not discoverable in-game right now. Having that available, as this commit does, I think is a clear improvement. Exactly how to show it, though, is something I'd like help with :)

Aside from making the info available for the first time on many buildings, having it standardised with a bold heading will also aid players to find it more quickly (even for those two random buildings). And it will aid in code maintenance and help translators focus their efforts onto this one single string only.

In D2005#83597, @Nescio wrote:

To start with, the proposed string seems unnecessarily verbose: "Territory: Buildings in this building's territory do not decay." What we're interested in is the fact it has a territory root, which can be conveyed with the much shorter "Territory root: true".

As starting point, I used the same text we already have today for the monument and pilar. Almost verbatim, but I made it slightly shorter. I'd like shorten it even more, but not sure how best to phrase it. "Territory root: true", I think would be too cryptic for the primary audience of this information (new players). The meaning of "root" is not clear (and difficult to translate). Use of "true" is a bit software-centric, I think.

Something like "Territory: Has territory root." could be a minimal version, but still has the "Root" problem. If we do go with something that short, I would propose that we display a different string in the Information dialog. I don't know if that would be controversial, as right now we always have the same text in both (for bits that are included in both).

In D2005#83597, @Nescio wrote:

If there is a separate territory influence line, then perhaps display radius and weight as well, e.g. "Territory: root: true; radius: 38; weight: 40000."

Yeah, this might be much for the tooltip, but could definitely work in the Information dialog. It would also provide space to use a little extra sentence to give it meaningful context.

If someone approves adding the other influence parameters as well now, I'd be happy to work on that. If not (yet), perhaps continue that on Trac or in the forums first?

The first parts of the monument and pillar <Tooltip> can be removed, because auras now have their own tooltips displayed.

I've removed the text about territory root from the descriptions in this commit. I'm not very familiar with auras yet, can you elaborate on that? Is there a Trac task for that?

We currently show this info for two seemingly random buildings only (one civ's monument, and one civ's pillar). For all other buildings with territory root, this information is not discoverable in-game right now. Having that available, as this commit does, I think is a clear improvement.

Yes, I fully agree with that, I'm just not convinced it should be on a separate line.
Wouldn't it be better to simply add "Territory root." at the beginning or end of the tooltip text string?
Have a look at the farmstead, where the tooltip informs us it's a dropsite for food, important information.

You're right “Territory root” might be unclear when encountered the first time, but I believe people can figure it out. “Buildings in the territory of the monument do not decay.” is also problematic (what is the “territory of the monument”? what does “decay” mean?).

I'm not very familiar with auras yet, can you elaborate on that? Is there a Trac task for that?

Have a look at the screenshots you posted. As you can see all relevant information of the aura is already displayed automatically (the "Religious Fervor" line). Which means the tooltips of iber_monument.xml, maur_pillar_ashoka.xml, and pers_ishtar_gate.xml can now be purged.

That's why we display <CapturePoints>

Actually it isn't, I thought it was. It probably should, perhaps on the same line as health. Something for a future patch.

Krinkle added a comment.EditedMon, Jun 24, 11:25 PM
In D2005#83640, @Nescio wrote:
@Krinkle wrote:

I'm not very familiar with auras yet, can you elaborate on that? Is there a Trac task for that?

Have a look at the screenshots you posted. As you can see all relevant information of the aura is already displayed automatically (the "Religious Fervor" line). Which means the tooltips of iber_monument.xml, maur_pillar_ashoka.xml, and pers_ishtar_gate.xml can now be purged.

Ah, right. Yeah, that seems redundant indeed. I've filed #5466. I'll work on that in a separate patch alongside this one.

Krinkle edited the test plan for this revision. (Show Details)Sat, Jun 29, 1:25 AM
wraitii added a subscriber: wraitii.EditedSat, Jun 29, 12:19 PM

I agree that Territory root isn't immediately obvious, but we should have consistency in naming things so probably best to keep it: https://trac.wildfiregames.com/wiki/EnglishStyleGuide

It seems like having some data on influence could also be useful, but tooltip wise I think it makes sense to have them on separate lines.
If we go with just "Territory root" in bold white, players will ultimately understand what it means and that's quite enough imo.

We probably should have a glossary or something like the Civilopedia someday so that new players can understand such words though.

binaries/data/mods/public/globalscripts/Templates.js
320

By convention we use !!template.TerritoryInfluence.Root

Something similar is that <BuildRestrictions/Territory> (own, neutral, enemy, ally) is sometimes mentioned in a tooltip text (e.g. outpost), but certainly not always (e.g. civic centre). Perhaps we should move that information in this patch as well and display it on the same line as territory root, e.g.:
Territory: own, neutral; root: true
On the other hand, when you're attempting to place a structure in invalid territory, the previewed structure turns red and the tooltip informs you already why it can't be placed.
It's just something to contemplate, possibly though not necessarily a good idea.

Krinkle marked an inline comment as done.Sat, Jun 29, 11:41 PM
Krinkle added inline comments.
binaries/data/mods/public/globalscripts/Templates.js
320

This is not a boolean comparison, it's a string comparison :)

Krinkle marked an inline comment as done.Sun, Jun 30, 12:44 AM

A few options mentioned so far:

Buildings in the territory of the monument do not decay.(trunk)
Territory: Buildings in this building's territory do not decay.(current diff)
Territory: root = true.
Territory: Has territory root.
Territory: This building provides territory root.
Territory root: true.

The current diff is consistent with the old text, but is also not very good. Also, upon closer inspection, I find it's actually quite inconsistent with how other buildings are written. I believe, we tend to omit the context/subject from descriptions and tooltips. For example:

  • Barracks: Train citizen solders.
  • Temple / Medial Treatment: Heals nearby units.

A few more ideas with this taken into account:

Territory root: Territory of nearby buildings does not decay.
Territory root: Supporting territory for other buildings.
Territory root: Support the territory of nearby buildings.
Territory root: Strong territory that claims loyalty of nearby buildings.

It seems like having some data on influence could also be useful, [..]

Yeah, we should definitely add this data on the same line as well. The Trac ticket asked for this as well. Right now it is entirely undiscoverable in-game. If it's too much for the tooltip, it could also go to the Information overlay. Reducing the amount of text in the tooltip to only critical concepts, I think, helps players to identify and learn these fundamentals. Too much information overload in the tooltip can be discouraging and/or lead to "banner fatigue" where the tooltip is ignored entirely (I already see this problem today, something deserving of its own design improvements).

If it doesn't require too much extra code changes to expose, I can work that into this commit as well, but perhaps better to restrict to the Information overlay for now to keep the difference minimal. Then in a separate change we can work on finding a brief way to fit it in the tooltip, and decide whether it is worth the cost/overload to be in the tooltip. I'd like to leave that out for this change, which mainly focusses on standardising the existing information first, and making it available for many more buildings.

If we go with just "Territory root" in bold white, players will ultimately understand what it means and that's quite enough imo.
[..]
We probably should have a glossary or something like the Civilopedia someday so that new players can understand such words though.

I think once a concept is understood, we have quite a bit of liberty around what label to use to refer to it. "Territory root" is certainly good enough for that. But on its own, I don't think it help a player learn the concept itself. I don't think it's reasonable to expect that when a player needs to expand their territory, that they are first going to look through all possible buildings they can construct, find that none of them says it does what they find, collect all the unknown terms they encountered in the process (including "territory root"), and look them up in a terminology index to see if one of the missed terms does what they want. That's quite a lot of steps. I think this kind of workflow is good to allow, but better suited for something already less direct in nature. For example, perhaps if there was a special Healer that can defeat an Aura, it can be described "Disable the aura of nearby units". A player would only need this if they know what an aura is, so it doesn't need to explain it. Or if they're curious anyway, can look it up in such terminology page.

I think we can find a good middle ground by giving its own tooltip line (combined with the strength and radius of the territory root, but separate from what territory kind the building can be placed in), in which we give a brief definition of the concept that would help players understand it.

That gives us the best of both: We have a term that we can refer to elsewhere (so "Territory root", not "Territory"), and yet also explained in-context in-game.

Do you like one of the above ideas? Or perhaps have additional suggestions for the phrasing?

A few options mentioned so far:

  • Automatically insert “Territory root.” at the beginning of the tooltip text line
  • Automatically insert “Territory root.” at the end of the tooltip text line
  • have a separate territory root line (current diff), e.g. “Territory root: true”
  • have a separate territory influence line, e.g. “Territory: root: true; radius: 38; weight: 40000.”
  • have a combined <BuildRestrictions/Territory> and <Territory/Root> line, e.g. “Territory: own, neutral; root: true”