Page MenuHomeWildfire Games

Add an in-game Template Details Viewer
ClosedPublic

Authored by s0600204 on Apr 6 2017, 5:28 PM.

Details

Summary

Right-click on icons of units, structures, technologies, flora, fauna, etc. and view details about the underlying template. (ie. http://trac.wildfiregames.com/ticket/3212#comment:4)

Works in the Structure Tree and the in-game session gui.

Known Issues:

  • Within the session gui, if a button for selecting which unit/structure/technology to train/build/research is disabled (for instance if the player has not reached the appropriate phase) then right-clicking to see the details will not work due to the button being disabled.
    • Right-click on the production icons also doesn't work for observers. 😢
  • Animals display a cost of 1 time. (Except trainable animals such as sheep and goats.)
    • This is inherited from template_unit.
    • template_unit_fauna modifies the component settings by removing the population cost, but does not remove the time cost.

Translation Warning: This revision turns on translation for the history attribute of units and structures, and the description attribute of technologies. Committing this will suddenly give translators a lot of work to do. History strings have been purged from all templates. This is no longer a concern.

Depends on D295, D1192

Test Plan
  • Apply patch.
  • Start 0AD
  • Within the Structure Tree:
    • Right-click on some of the icons of structures/units/technologies
  • Within a game session:
    • Select a unit or structure.
    • Right-click on the smaller icons that make up the build/train/research lists.
    • Select any selectable in-game entity.
    • Right-click on the big icon that is part of the single-select gui at the bottom centre of the screen.

Check linting/code sanity.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

Alterations in response to elexis' comments.

Revision now requires/depends on D1192

In D297#50795, @elexis wrote:

Foremoslty (that is a word apparently),

Foremostly* (indeed it is 😉)

When testing I noticed that the tooltip of the structree should become hidden when opening the dialog. But I suspect that won't be a cakewalk.

Probably not, no.

I recall there was some decision not to have D846 or to have it differently? It seems like a nice feature, but I guess not for now.

(A bit off-topic for this revision...) I'm not totally against having a button, it was the merging of the two pages (which was the original intent of that revision) I was against.

On a related note, I think that the civinfo page should be (a) rewritten to load the information it needs from entity templates where possible (rather than the the {civ}.json files), and (b) redesigned to bring it in-line visually with the rest of the gui (at which point a button leading the structree could be added).

binaries/data/mods/public/gui/reference/common/core.js
175 ↗(On Diff #5377)

This revision now requires D1192

binaries/data/mods/public/gui/reference/common/load.js
45 ↗(On Diff #5377)

How so? (It is possible I'm missing something)

If the first argument is removed, the following warning will display for sheep and palisade walls.

If the second argument is removed, the following warning will display on viewer page init before g_SelectedCiv has had a chance to be set to anything but its default.

If the third argument is removed, we get a warning for every single template loaded.

For any of these cases, we shouldn't give a warning.

binaries/data/mods/public/gui/reference/viewer/viewer.js
124 ↗(On Diff #5377)

No part of this code block sets the font, it only reads it so it can determine how tall each line should be.

But yes, most of this block can be replaced by a call to the function implemented/proposed in D844, which would satisfy the @todo.

142 ↗(On Diff #5377)

(Nor am I, which is partly why this function is in this file, and not in one of the common js files.)

313 ↗(On Diff #5377)

Uh, the return is a string. Might be an empty string on some occasions, but it still is a string.

I realise the name of the function may be misleading, so I've renamed this and the other applicable functions.

elexis added inline comments.Jan 30 2018, 4:55 PM
binaries/data/mods/public/gui/reference/common/core.js
185 ↗(On Diff #5508)

I don't see the SelectionGroupName dictating any template name:

"<element name='SelectionGroupName' a:help='Name used to group ranked entities.'>" +

So we may not rely on selection group names being template names or paths.
By definition _x0222 is a unique identifier too.

Ranks were only the first use case, but (1) the name of the property SelectionGroupName, (2) all code that we have for it (grouping of
GarrisonHolder unloading and session unit selection list) and (3) that we (me after fatherbushidos idea) gave relics the same SelectionGroupName despite them not having ranks indicate against this parsing.

binaries/data/mods/public/gui/reference/viewer/viewer.js
188 ↗(On Diff #5508)

let templateFoo = templateName.split("|").pop() maybe?

elexis added inline comments.Feb 1 2018, 12:55 AM
binaries/data/mods/public/gui/reference/viewer/viewer.js
17 ↗(On Diff #5377)

Moved all treasure to gaia/treasure/ and all ruins to gaia/ruins/, D989

s0600204 updated this revision to Diff 5622.Feb 1 2018, 5:34 PM
s0600204 marked 5 inline comments as done.

Take into account moved treasures and the renamed rank icons, and rewrite the getBaseTemplateName function to not use Identity/SelectionGroupName.

binaries/data/mods/public/gui/reference/common/core.js
185 ↗(On Diff #5508)

Rewritten function to not use SelectionGroupName at all. See if you prefer this new version.

Vulcan added a comment.Feb 1 2018, 6:28 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 1 2018, 6:29 PM
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'while' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/globalscripts/utility.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/globalscripts/utility.js
|  30|  30| 
|  31|  31| 	let i = 0;
|  32|  32| 	while (i < array.length)
|  33|    |-	{
|    |  33|+	
|  34|  34| 		if (c[i] < i)
|  35|  35| 		{
|  36|  36| 			let swapIndex = i % 2 ? c[i] : 0;
|  48|  48| 			c[i] = 0;
|  49|  49| 			++i;
|  50|  50| 		}
|  51|    |-	}
|    |  51|+	
|  52|  52| }
|  53|  53| 
|  54|  54| /**
|    | [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
| 414| 414| 		});
| 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" +
| 420| 420| 		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
| 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" +
| 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/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
| 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" +
|    | 419|+	}) + "\n" +
| 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/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels.js
| 481| 481| 				continue;
| 482| 482| 
| 483| 483| 			if (state.pack.progress == 0)
| 484|    |-			{
|    | 484|+			
| 485| 485| 				if (state.pack.packed)
| 486| 486| 					checks.unpackButton = true;
| 487| 487| 				else
| 488| 488| 					checks.packButton = true;
| 489|    |-			}
|    | 489|+			
| 490| 490| 			else if (state.pack.packed)
| 491| 491| 				checks.unpackCancelButton = true;
| 492| 492| 			else
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels.js
| 796| 796| 				addResearchToQueue(data.item.r

About SelectionGroupName, it can be used in and only in accordance with it's definition, which is grouping some units in a GUI selection. So you could use it if you do some kind of grouping in the GUI. Looks like you need a template name, so using the parent template name sounds more correct.
Thanks for the update.

binaries/data/mods/public/globalscripts/utility.js
73 ↗(On Diff #5622)

Correct to catch index -1 (a negative second argument for slice would move backwards from the end, not what we want for index == -1).

Maybe path.split("/").pop().join("/")?

binaries/data/mods/public/gui/common/tooltips.js
126 ↗(On Diff #5622)

Renaming this to getIdentityTooltip would transport the same message with 2 lines less

143 ↗(On Diff #5622)

Auras, technologies and entity components have different features, so making a function that returns something for more than one type means that the reader of the call to this function has to lookup this function to see what it supports. GetAuraDescription(Tooltip), GetTechnologyDescription and GetIdentityDescription seems better distinguishable and more conform with the rest to me. Works for now though and saves some redundancy as the description property coincidentally has the same name in all 3 cases. But IMO it's an ambiguity which should be rectified.
Notice there might be a different entity component with a Description property however, then we certainly need to distinguish.

Function comments should be condensed into 1-2 lines if possible.
The amount of different sources for this function seems too complex.

818 ↗(On Diff #5622)

Should end with Tooltip instead of message, as that's the norm of this file. templateViewerTooltip?

binaries/data/mods/public/gui/reference/common/core.js
185 ↗(On Diff #5622)

Should define in one sentence what a template base is, since it's a new term. (Better would be to avoid it however).

I didn't understand the function. How can templateName be a mirage, by "mirage|templatename"?

binaries/data/mods/public/gui/reference/viewer/viewer.js
20 ↗(On Diff #5622)

Condense to 1 line. List is not well defined (array), but that it's an array is self evident, no need to mention that, just consumes time to read when one could have read [ already.

22 ↗(On Diff #5622)

s/Info/Tooltip

32 ↗(On Diff #5622)

The functions here should all do the same thing, return the entire string, not one to return the caption and the rest the rest of the previous function.

I suspect you might want 3 arrays, one for auras, one for techs and one for entities.

74 ↗(On Diff #5622)

(It's the tech requirements defining which civ can research it, but deriving it from the researcher is ok if that's shorter)

87 ↗(On Diff #5622)

Should always or never pass the civ per parameter (single source of truth, minimization of possible code paths i.e. https://en.wikipedia.org/wiki/Cyclomatic_complexity).

If I recall correctly it was only about the palisade wallset? If so I will try to assassinate that.

114 ↗(On Diff #5622)

let entityStats = Engine.GetGUIObjectByName("entityStats");, easier to track if GUI objects names are unique

193 ↗(On Diff #5622)

We shouldn't figure out after the effect what kind of file we're dealing with, but it should be a choice by the template viewer to first load all aura templates into an aura variable, then all techs into a tech variable and then all entity templates into a templates or entitytemplates variable.

Whether an entity is a Unit or Structure should be derived from the template (https://en.wikipedia.org/wiki/Single_source_of_truth pattern).
This way by design there is no management of secondary data that needs to be kept in sync (and we don't even have to start philosophing about other/)

I suspect we don't need the Unit / Structure differentiation if the getBuilderTooltip and getResearcherTooltip functions just return emptystring in case.
(Also suspecting that this iteration of the code implies that units can't research techs?)

253 ↗(On Diff #5622)

(Can't tell right now, but might be nicer as 3 functions too?)

binaries/data/mods/public/gui/reference/viewer/viewer.xml
6 ↗(On Diff #5377)

thx

binaries/data/mods/public/gui/session/selection_panels.js
273 ↗(On Diff #5622)

The push breaks the symmetry here and in more than one place below. Suggesting
templateViewerRightclickTooltip
templateViewerLeftclickTooltip

binaries/data/mods/public/l10n/messages.json
405 ↗(On Diff #5622)

History entries are correct and it wouldn't hurt to commit them already so that the stakeholders are informed separately.

Can nuke this new one while committing if you want, afaik empty strings are illegal even:
units/athen_champion_ranged_gastraphetes.xml: <History></History>

526 ↗(On Diff #5622)

(Could think about adding History here too, but then we would need GUI support too)

elexis added inline comments.Feb 1 2018, 11:33 PM
binaries/data/mods/public/globalscripts/utility.js
73 ↗(On Diff #5622)

You can commit this function (with or without popping) separately already if you want, because we can use it in randombiome.js which has the same code already.

Also we have two occurrences of this pattern templateName.substr(templateName.lastIndexOf("|") + 1), so could refactor that too.

s0600204 updated this revision to Diff 5777.Feb 13 2018, 6:56 AM
s0600204 marked 6 inline comments as done.

Update to take into account rP21174, and address some comments from @elexis.

binaries/data/mods/public/gui/common/tooltips.js
126 ↗(On Diff #5622)
  1. Other tooltip functions have names that reflect what they return. getCurrentHealthTooltip returns the current health, getLootTooltip builds and returns a suitable string displaying loot. We're not returning the Identity here.
  1. Furthermore, in entity templates "History" is also stored in the Identity component, so by calling it getIdentityTooltip, which of the two, or indeed any of the other values in the Identity component, are we returning?
  1. Technologies have a tooltip attribute that
    1. isn't stored in any Identity component or object, and
    2. is also formatted and returned here.
  1. Calling it getEntityTooltip matches the other getEntity* functions that also apply to techs and auras as well as xml-defined entities.

So no, I'm not renaming the function to that.

143 ↗(On Diff #5622)

If we were to have GetAuraDescription() and GetTechnologyDescription(), the logic of the two functions would be identical and you'd be asking me to remove the duplication.

We also don't have GetTechnologyNames or GetAuraName - they all use GetEntityNames, so why would we need separate functions to return the same attribute from techs, auras and entities just because they come from different sources? The approach used here is consistent with the rest of the file.

binaries/data/mods/public/gui/reference/viewer/viewer.js
22 ↗(On Diff #5622)

They're not being used in/as a tooltip, so no.

87 ↗(On Diff #5622)

And modders. Please don't assassinate the modders.

193 ↗(On Diff #5622)

I suspect we don't need the Unit / Structure differentiation if the getBuilderTooltip and getResearcherTooltip functions just return emptystring in case.

If you mean the functions I think you're referring to, they already do.

(Also suspecting that this iteration of the code implies that units can't research techs?)

Where are you getting that from? The structree/reference-suite code has supported units being able to research techs since rP18218.

253 ↗(On Diff #5622)

Tried it. Now tell me if you like the duplicated logic...

elexis accepted this revision.Feb 13 2018, 2:55 PM
  • About the tooltip of the previous page being still shown after the click, maybe we could replace that tooltip with empty string just before the page opening.
  • The selection panel unit portrait doesn'T do anything on left-click, so that could open on left instead of right click
  • The selection panel tooltip icon could open on leftclick too maybe

Tested, all works well.
Code read from A to Z again, all reads well.
Correctness forces me to accept and salute!

Thanks for all your work on the structure tree and the template viewer s0600204!

Perhaps you can paste the code on jshint.com to find some linting issues we didn't notice yet (Vulcan seems to be quiet).

binaries/data/mods/public/globalscripts/Templates.js
393 ↗(On Diff #5777)

just civ?

binaries/data/mods/public/gui/common/tooltips.js
126 ↗(On Diff #5622)

Ok to keep the name now as it's not this patch introducing it, but getEntityFoo being used for techs and aruas is misleading.

143 ↗(On Diff #5622)

The function introduced here is okay
I'm just not happy about that mapping auraDescription -> description, either we should have different functions parsing entity.description or aura.auraDescription or (likely better) the aura using description instead of auraDescription. The preknowledge about that mapping shouldn't be required or explained if we can get away without that additional complexity

binaries/data/mods/public/gui/reference/common/core.js
193 ↗(On Diff #5777)

for example foo returns bar

216 ↗(On Diff #5777)

(> 0 could be omitted I suppose?)

binaries/data/mods/public/gui/reference/common/load.js
107 ↗(On Diff #5777)

bad space

binaries/data/mods/public/gui/reference/viewer/viewer.js
22 ↗(On Diff #5622)

I mostly recommended that because about every of the items is a Tooltip function.
Then eventually the right thing would be renaming tooltips.js and the functions to something like description.
g_LabelFunctions? g_DescriptonFunctions? Everything is an Information, no?

87 ↗(On Diff #5622)

But we don't want to encourage mods using templates as wrong as the palisades however?

253 ↗(On Diff #5622)

Barely anything left where we can speak of duplication. The function looks much cleaner to me then the previous getProducedByCaption which would mean I should say thank you :-)
In case the shared code becomes more than 2 lines and we wanting to unify that, we could still add a helper function.

2 ↗(On Diff #5777)

.

15 ↗(On Diff #5777)

modification of variables should be done in a function, can do it on init or a function called on init

95 ↗(On Diff #5777)

Can also do throw new Error, but I guess this one is more comfortable for debugging

110 ↗(On Diff #5777)

move one line lower

172 ↗(On Diff #5777)

The internal property seems like a workaround. That property should be accessible to all places using the globalscripts parsing or none.

Could also be done by changing the structure to g_BuildList[civ][templateName], g_BuildList[civ] = { "templateName": ..., "template": ... } or g_BuildList[civ] = [templateName1, ...]` and parsing JIT.

But it seems legit that one might want to get the templateName from a parsed template, so why not add it to GetTemplateDataHelper? (Can be done afterwards, since it's present already).

At last, I don't know where the tech/ actually comes from, can't find it in templates files nor directories. I had assumed we might be able to use basename

218 ↗(On Diff #5777)

prod -> tech?

220 ↗(On Diff #5777)

research -> techTemplate or technologyTemplate?

242 ↗(On Diff #5777)

could be inlined if you like

266 ↗(On Diff #5777)

The ones who like that could use
"listOfNames": template.upgrades.map(upgrade => getEntityNames(upgrade.name ? upgrade : loadEntityTemplate(upgrade.entity))).join(translate(", "))

binaries/data/mods/public/gui/session/selection_panels.js
1025 ↗(On Diff #5777)

it doesn't consume an argument, but we could stil add that to the array above. not so relevant here because we push custom things below too.

1173 ↗(On Diff #5777)

If technologies don't have a civ, then code parsing techs requiring a civ would be technically wrong, no?

binaries/data/mods/public/l10n/messages.json
405 ↗(On Diff #5777)

I suspect we want to ask Sundiata if he wants to write something nice for Kushites

478 ↗(On Diff #5777)

This file is still correct.

This revision is now accepted and ready to land.Feb 13 2018, 2:55 PM

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/reference/common/load.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/reference/common/load.js
| 104| 104|  *
| 105| 105|  * @return {(object|null)} Sanitized object about the requested template or null if entity template doesn't exist.
| 106| 106|  */
| 107|    |-function loadEntityTemplate (templateName)
|    | 107|+function loadEntityTemplate(templateName)
| 108| 108| {
| 109| 109| 	if (!Engine.TemplateExists(templateName))
| 110| 110| 		return null;
|    | [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 (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /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
| 226| 226| 		{
| 227| 227| 			let aura = auraTemplates[auraID];
| 228| 228| 			ret.auras[auraID] = {
| 229|    |-					"name": aura.auraName,
|    | 229|+				"name": aura.auraName,
| 230| 230| 					"description": aura.auraDescription || null,
| 231| 231| 					"radius": aura.radius || null
| 232| 232| 				};
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /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
| 227| 227| 			let aura = auraTemplates[auraID];
| 228| 228| 			ret.auras[auraID] = {
| 229| 229| 					"name": aura.auraName,
| 230|    |-					"description": aura.auraDescription || null,
|    | 230|+				"description": aura.auraDescription || null,
| 231| 231| 					"radius": aura.radius || null
| 232| 232| 				};
| 233| 233| 		}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /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
| 228| 228| 			ret.auras[auraID] = {
| 229| 229| 					"name": aura.auraName,
| 230| 230| 					"description": aura.auraDescription || null,
| 231|    |-					"radius": aura.radius || null
|    | 231|+				"radius": aura.radius || null
| 232| 232| 				};
| 233| 233| 		}
| 234| 234| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 4.
|----|    | /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
| 229| 229| 					"name": aura.auraName,
| 230| 230| 					"description": aura.auraDescription || null,
| 231| 231| 					"radius": aura.radius || null
| 232|    |-				};
|    | 232|+			};
| 233| 233| 		}
| 234| 234| 	}
| 235| 235| 

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
| 393| 393| function getRepairTimeTooltip(entState)
| 394| 394| {
| 395| 395| 	return sprintf(translate("%(label)s %(details)s"), {
| 396|    |-			"label": headerFont(translate("Number of repairers:")),
|    | 396|+		"label": headerFont(translate("Number of repairers:")),
| 397| 397| 			"details": entState.repairable.numBuilders
| 398| 398| 		}) + "\n" + (entState.repairable.numBuilders ?
| 399| 399| 		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
| 394| 394| {
| 395| 395| 	return sprintf(translate("%(label)s %(details)s"), {
| 396| 396| 			"label": headerFont(translate("Number of repairers:")),
| 397|    |-			"details": entState.repairable.numBuilders
|    | 397|+		"details": entState.repairable.numBuilders
| 398| 398| 		}) + "\n" + (entState.repairable.numBuilders ?
| 399| 399| 		sprintf(translatePlural(
| 400| 400| 			"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
| 395| 395| 	return sprintf(translate("%(label)s %(details)s"), {
| 396| 396| 			"label": headerFont(translate("Number of repairers:")),
| 397| 397| 			"details": entState.repairable.numBuilders
| 398|    |-		}) + "\n" + (entState.repairable.numBuilders ?
|    | 398|+	}) + "\n" + (entState.repairable.numBuilders ?
| 399| 399| 		sprintf(translatePlural(
| 400| 400| 			"Add another worker to speed up the repairs by %(second)s second.",
| 401| 401| 			"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
| 415| 415| function getBuildTimeTooltip(entState)
| 416| 416| {
| 417| 417| 	return sprintf(translate("%(label)s %(details)s"), {
| 418|    |-			"label": headerFont(translate("Number of builders:")),
|    | 418|+		"label": headerFont(translate("Number of builders:")),
| 419| 419| 			"details": entState.foundation.numBuilders
| 420| 420| 		}) + "\n" + (entState.foundation.numBuilders ?
| 421| 421| 		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
| 416| 416| {
| 417| 417| 	return sprintf(translate("%(label)s %(details)s"), {
| 418| 418| 			"label": headerFont(translate("Number of builders:")),
| 419|    |-			"details": entState.foundation.numBuilders
|    | 419|+		"details": entState.foundation.numBuilders
| 420| 420| 		}) + "\n" + (entState.foundation.numBuilders ?
| 421| 421| 		sprintf(translatePlural(
| 422| 422| 			"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
| 417| 417| 	return sprintf(translate("%(label)s %(details)s"), {
| 418| 418| 			"label": headerFont(translate("Number of builders:")),
| 419| 419| 			"details": entState.foundation.numBuilders
| 420|    |-		}) + "\n" + (entState.foundation.numBuilders ?
|    | 420|+	}) + "\n" + (entState.foundation.numBuilders ?
| 421| 421| 		sprintf(translatePlural(
| 422| 422| 			"Add another worker to speed up the construction by %(second)s second.",
| 423| 423| 			"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
| 481| 481| 				continue;
| 482| 482| 
| 483| 483| 			if (state.pack.progress == 0)
| 484|    |-			{
|    | 484|+			
| 485| 485| 				if (state.pack.packed)
| 486| 486| 					checks.unpackButton = true;
| 487| 487| 				else
| 488| 488| 					checks.packButton = true;
| 489|    |-			}
|    | 489|+			
| 490| 490| 			else if (state.pack.packed)
| 491| 491| 				checks.unpackCancelButton = true;
| 492| 492| 			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
| 796| 796| 				addResearchToQueue(data.item.researchFacilityId, tech);
| 797| 797| 			};
| 798| 798| 
| 799|    |-			button.onPressRight = function () {
|    | 799|+			button.onPressRight = function() {
| 800| 800| 				let researcherTemplate;
| 801| 801| 				for (let selectedEntity of data.unitEntStates)
| 802| 802| 					if (selectedEntity.id == data.item.researchFacilityId)

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

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

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

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

Itms added a subscriber: Itms.Feb 13 2018, 4:10 PM
In D297#53130, @elexis wrote:

(Vulcan seems to be quiet).

The poor boy has been building Stan's commits all night and morning 😛 There you go though.

s0600204 updated this revision to Diff 5830.Feb 18 2018, 9:11 PM
s0600204 marked 9 inline comments as done.

De-duplicate list text formatting; and get technology researchers appearing again.

s0600204 requested review of this revision.Feb 18 2018, 9:11 PM

Returning to review queue to get a yes/no on the buildListText function and use of same. (Function might need a better name.) (Could be moved to tooltips.js and used inside getVisibleEntityClassesFormatted (in another revision/commit).)

binaries/data/mods/public/gui/reference/viewer/viewer.js
253 ↗(On Diff #5622)

Ok.

which would mean I should say thank you :-)

You're welcome.

binaries/data/mods/public/gui/session/selection_panels.js
1025 ↗(On Diff #5777)

The above array is used conditionally. Use of the viewer is not conditional on whether "showdetailedtooltips" is "true".

1173 ↗(On Diff #5777)

Uh, no.

Techs don't "have a civ" because they can belong to (or have) one or more civs. We don't know which of them we're looking at (and we need to know to get/use the correct specific name, cost, civ-bonus/penalty modifiers, etc).

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

Linter detected issues:
Executing section Default...
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 (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /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
| 226| 226| 		{
| 227| 227| 			let aura = auraTemplates[auraID];
| 228| 228| 			ret.auras[auraID] = {
| 229|    |-					"name": aura.auraName,
|    | 229|+				"name": aura.auraName,
| 230| 230| 					"description": aura.auraDescription || null,
| 231| 231| 					"radius": aura.radius || null
| 232| 232| 				};
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /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
| 227| 227| 			let aura = auraTemplates[auraID];
| 228| 228| 			ret.auras[auraID] = {
| 229| 229| 					"name": aura.auraName,
| 230|    |-					"description": aura.auraDescription || null,
|    | 230|+				"description": aura.auraDescription || null,
| 231| 231| 					"radius": aura.radius || null
| 232| 232| 				};
| 233| 233| 		}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /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
| 228| 228| 			ret.auras[auraID] = {
| 229| 229| 					"name": aura.auraName,
| 230| 230| 					"description": aura.auraDescription || null,
| 231|    |-					"radius": aura.radius || null
|    | 231|+				"radius": aura.radius || null
| 232| 232| 				};
| 233| 233| 		}
| 234| 234| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 4.
|----|    | /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
| 229| 229| 					"name": aura.auraName,
| 230| 230| 					"description": aura.auraDescription || null,
| 231| 231| 					"radius": aura.radius || null
| 232|    |-				};
|    | 232|+			};
| 233| 233| 		}
| 234| 234| 	}
| 235| 235| 

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 (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
| 481| 481| 				continue;
| 482| 482| 
| 483| 483| 			if (state.pack.progress == 0)
| 484|    |-			{
|    | 484|+			
| 485| 485| 				if (state.pack.packed)
| 486| 486| 					checks.unpackButton = true;
| 487| 487| 				else
| 488| 488| 					checks.packButton = true;
| 489|    |-			}
|    | 489|+			
| 490| 490| 			else if (state.pack.packed)
| 491| 491| 				checks.unpackCancelButton = true;
| 492| 492| 			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
| 796| 796| 				addResearchToQueue(data.item.researchFacilityId, tech);
| 797| 797| 			};
| 798| 798| 
| 799|    |-			button.onPressRight = function () {
|    | 799|+			button.onPressRight = function() {
| 800| 800| 				let researcherTemplate;
| 801| 801| 				for (let selectedEntity of data.unitEntStates)
| 802| 802| 					if (selectedEntity.id == data.item.researchFacilityId)

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

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

binaries/data/mods/public/gui/session/selection_panels.js
| 764| »   »   »   »   »   »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
|    | [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| function getRepairTimeTooltip(entState)
| 394| 394| {
| 395| 395| 	return sprintf(translate("%(label)s %(details)s"), {
| 396|    |-			"label": headerFont(translate("Number of repairers:")),
|    | 396|+		"label": headerFont(translate("Number of repairers:")),
| 397| 397| 			"details": entState.repairable.numBuilders
| 398| 398| 		}) + "\n" + (entState.repairable.numBuilders ?
| 399| 399| 		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
| 394| 394| {
| 395| 395| 	return sprintf(translate("%(label)s %(details)s"), {
| 396| 396| 			"label": headerFont(translate("Number of repairers:")),
| 397|    |-			"details": entState.repairable.numBuilders
|    | 397|+		"details": entState.repairable.numBuilders
| 398| 398| 		}) + "\n" + (entState.repairable.numBuilders ?
| 399| 399| 		sprintf(translatePlural(
| 400| 400| 			"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
| 395| 395| 	return sprintf(translate("%(label)s %(details)s"), {
| 396| 396| 			"label": headerFont(translate("Number of repairers:")),
| 397| 397| 			"details": entState.repairable.numBuilders
| 398|    |-		}) + "\n" + (entState.repairable.numBuilders ?
|    | 398|+	}) + "\n" + (entState.repairable.numBuilders ?
| 399| 399| 		sprintf(translatePlural(
| 400| 400| 			"Add another worker to speed up the repairs by %(second)s second.",
| 401| 401| 			"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
| 415| 415| function getBuildTimeTooltip(entState)
| 416| 416| {
| 417| 417| 	return sprintf(translate("%(label)s %(details)s"), {
| 418|    |-			"label": headerFont(translate("Number of builders:")),
|    | 418|+		"label": headerFont(translate("Number of builders:")),
| 419| 419| 			"details": entState.foundation.numBuilders
| 420| 420| 		}) + "\n" + (entState.foundation.numBuilders ?
| 421| 421| 		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
| 416| 416| {
| 417| 417| 	return sprintf(translate("%(label)s %(details)s"), {
| 418| 418| 			"label": headerFont(translate("Number of builders:")),
| 419|    |-			"details": entState.foundation.numBuilders
|    | 419|+		"details": entState.foundation.numBuilders
| 420| 420| 		}) + "\n" + (entState.foundation.numBuilders ?
| 421| 421| 		sprintf(translatePlural(
| 422| 422| 			"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
| 417| 417| 	return sprintf(translate("%(label)s %(details)s"), {
| 418| 418| 			"label": headerFont(translate("Number of builders:")),
| 419| 419| 			"details": entState.foundation.numBuilders
| 420|    |-		}) + "\n" + (entState.foundation.numBuilders ?
|    | 420|+	}) + "\n" + (entState.foundation.numBuilders ?
| 421| 421| 		sprintf(translatePlural(
| 422| 422| 			"Add another worker to speed up the construction by %(second)s second.",
| 423| 423| 			"Add another worker to speed up the construction by %(second)s seconds.",

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

elexis accepted this revision.Feb 19 2018, 3:06 PM

Returning to review queue

Looks good, but I didn't get my magnifying glasses with this iteration however.
showTemplateViewerOnRightClickTooltip change is nice.

to get a yes/no on the buildListText function and use of same

Comparing the diff with the history functions, it's indeed better this way, thanks.

Function might need a better name

Indeed, I like the tooltip name since the file and most functions are called like that. Perhaps we should use that for consistency now and rename all occurrences later. Or not, doesn't matter much.

Could be moved to tooltips.js and used inside getVisibleEntityClassesFormatted (in another revision/commit)

Ok

binaries/data/mods/public/globalscripts/Templates.js
393 ↗(On Diff #5777)

(I guess that's a no)

binaries/data/mods/public/gui/session/selection_panels.js
1025 ↗(On Diff #5777)

Oh yes

This revision is now accepted and ready to land.Feb 19 2018, 3:06 PM
This revision was automatically updated to reflect the committed changes.