Page MenuHomeWildfire Games

Rewrite Structure Tree and Template Viewer to use OOP principles
Needs ReviewPublic

Authored by s0600204 on May 16 2020, 12:50 AM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#5387
Summary

Inspired by @elexis' ticket (#5387) and his (and others') work on various gui pages and code thereof.

Functions/variables that cannot be removed from the global namespace have a comment explaining why.

Changeset also includes a reduction in xml gui objects - instead of having four rows of 20 "structure boxes" (the boxes that represent structures) - most of which were never really used - there are now 40 total that are programatically positioned vertically as well as horizontally, serving all phases. Taking into account the children of each "structure box", this is a reduction of ~3964 UI objects(!).

We also now take into account the length of a structure/unit's name-text when determining the width of a "structure/trainer box". No more truncated names!

Otherwise, end result should be pretty much the same.

Test Plan

Check code sanity.

Verify the Structure Tree and Viewer pages open and perform as expected:

  • From the Main Menu,
  • From the Game Setup,
  • From in the middle of a game (including viewing the Viewer on gaia entities)

Diff Detail

Event Timeline

s0600204 created this revision.May 16 2020, 12:50 AM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2126/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1600/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/711/display/redirect

s0600204 planned changes to this revision.EditedMay 16 2020, 12:55 AM

D'oh! Forgot the "History" -> "Civilization Overview" text change, didn't I? *sigh*

Stan awarded a token.May 16 2020, 9:01 AM
s0600204 updated this revision to Diff 11879.May 16 2020, 4:24 PM

Rebase to include the "History" -> "Civilization Overview" string change

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (quote-props):
|    | Unquoted property 'value' found.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/EntityBox.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/EntityBox.js
| 106| 106| /**
| 107| 107|  * Minimum width of the boxes, and the margin between them
| 108| 108|  */
| 109|    |-Object.defineProperty(EntityBox, 'boxMinWidth', { value: 96 });
|    | 109|+Object.defineProperty(EntityBox, 'boxMinWidth', { "value": 96 });
| 110| 110| Object.defineProperty(EntityBox, 'boxMargin', { value: 8 });
| 111| 111| 
| 112| 112| /**
|    | [NORMAL] ESLintBear (quote-props):
|    | Unquoted property 'value' found.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/EntityBox.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/EntityBox.js
| 107| 107|  * Minimum width of the boxes, and the margin between them
| 108| 108|  */
| 109| 109| Object.defineProperty(EntityBox, 'boxMinWidth', { value: 96 });
| 110|    |-Object.defineProperty(EntityBox, 'boxMargin', { value: 8 });
|    | 110|+Object.defineProperty(EntityBox, 'boxMargin', { "value": 8 });
| 111| 111| 
| 112| 112| /**
| 113| 113|  * Functions used to collate the contents of a tooltip.
|    | [NORMAL] ESLintBear (quote-props):
|    | Unquoted property 'value' found.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/EntityBox.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/EntityBox.js
| 112| 112| /**
| 113| 113|  * Functions used to collate the contents of a tooltip.
| 114| 114|  */
| 115|    |-Object.defineProperty(EntityBox, 'TooltipFunctions', { value: [
|    | 115|+Object.defineProperty(EntityBox, 'TooltipFunctions', { "value": [
| 116| 116| 	getEntityNamesFormatted,
| 117| 117| 	getEntityCostTooltip,
| 118| 118| 	getEntityTooltip,
|    | [NORMAL] ESLintBear (quote-props):
|    | Unquoted property 'value' found.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/reference/common/ReferencePage.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/reference/common/ReferencePage.js
|  53|  53|  *
|  54|  54|  * The functions listed are defined in gui/common/tooltips.js
|  55|  55|  */
|  56|    |-Object.defineProperty(ReferencePage, 'StatsFunctions', { value: [
|    |  56|+Object.defineProperty(ReferencePage, 'StatsFunctions', { "value": [
|  57|  57| 	getHealthTooltip,
|  58|  58| 	getHealerTooltip,
|  59|  59| 	getAttackTooltip,
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/reference/common/ReferencePage.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/reference/common/ReferencePage.js
|  67|  67| 	getPopulationBonusTooltip,
|  68|  68| 	getResourceTrickleTooltip,
|  69|  69| 	getLootTooltip
|  70|    |-]});
|    |  70|+] });

binaries/data/mods/public/gui/reference/common/Lister.js
|  34| »   »   do·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.
|    | [NORMAL] ESLintBear (quote-props):
|    | Unquoted property 'value' found.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/reference/viewer/ViewerPage.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/reference/viewer/ViewerPage.js
| 142| 142|  * The result of these functions appear to the right of the entity icon
| 143| 143|  * on the page.
| 144| 144|  */
| 145|    |-Object.defineProperty(ViewerPage, 'StatsFunctions', { value: [getEntityCostTooltip].concat(ReferencePage.StatsFunctions) });
|    | 145|+Object.defineProperty(ViewerPage, 'StatsFunctions', { "value": [getEntityCostTooltip].concat(ReferencePage.StatsFunctions) });
| 146| 146| 
| 147| 147| /**
| 148| 148|  * Used to display textual information and the build/train lists of the
|    | [NORMAL] ESLintBear (quote-props):
|    | Unquoted property 'value' found.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/reference/viewer/ViewerPage.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/reference/viewer/ViewerPage.js
| 153| 153|  * The functions listed can be found in gui/common/tooltips.js or
| 154| 154|  * gui/reference/common/tooltips.js
| 155| 155|  */
| 156|    |-Object.defineProperty(ViewerPage, 'InfoFunctions', { value: [
|    | 156|+Object.defineProperty(ViewerPage, 'InfoFunctions', { "value": [
| 157| 157| 	getEntityTooltip,
| 158| 158| 	getHistoryTooltip,
| 159| 159| 	getDescriptionTooltip,
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/reference/viewer/ViewerPage.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/reference/viewer/ViewerPage.js
| 166| 166| 	getTrainText,
| 167| 167| 	getResearchText,
| 168| 168| 	getUpgradeText
| 169|    |-]});
|    | 169|+] });
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/draw.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/draw.js
|  88|  88| 	size += rowHeight * (phases * idx - (idx - 1) * idx / 2); // phase rows (phase-currphase+1 per row)
|  89|  89| 
|  90|  90| 	return size;
|  91|    |-}
|    |  91|+};
|  92|  92| 
|  93|  93| /**
|  94|  94|  * Positions certain elements that only need to be positioned once
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/draw.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/draw.js
| 151| 151| {
| 152| 152| 	for (let i = 0; i < this.Parser.phaseList.length; ++i)
| 153| 153| 	{
| 154|    |-		this.drawPhaseIcon(this.guiElems.phase[i].icon, i)
|    | 154|+		this.drawPhaseIcon(this.guiElems.phase[i].icon, i);
| 155| 155| 
| 156| 156| 		for (let j = 1; j < this.Parser.phaseList.length - i; ++j)
| 157| 157| 			this.drawPhaseIcon(this.guiElems.phase[i].bars.children[(j - 1)].children[0], j + i);

binaries/data/mods/public/gui/reference/structree/draw.js
|  91| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/gui/reference/structree/draw.js
| 154| »   »   this.drawPhaseIcon(this.guiElems.phase[i].icon,·i)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2131/display/redirect

s0600204 updated this revision to Diff 11880.May 16 2020, 4:40 PM

Apply linting suggestions from ESLintBear/JSHintBear

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2132/display/redirect

s0600204 added a child revision: Restricted Differential Revision.May 16 2020, 6:37 PM

Inline comments for two versions of the patch, I suppose not much changed.
I did not perform dynamic analysis.
To be clear, I do not request any change or commit, only try to offer anecdotes of experience, inspiration and technical backgrounds where noticeable.

The patch is furthering the objective of the JS GUI class rewrite #5387, so in that sense I would endorse it assuming that it works correctly.
bb mentioned that this can affect mods, so one can scout which mods modify this page and how it affects the existing modifications directly (if that is severe enough to stall any change for an a24 release) or
whether the benefits of the refactoring outweigh the possible estimated harm.

In my experience with the gamesetup, lobby and especially session page it paid off to split things into components,
whereas many smaller pages have only few components so that it doesn't matter that much.

For the smaller pages where it doesnt matter much (loading screen for instance), it can still be benefitial to have them in class syntax
in order to obtain global consistency which in turn will invite new modifications to be inserted without coupling and hardcoding logically separate concerns.
(I.e. new patches will be inserted cleanly before it becomes spaghetti code, compare with how the original gamesetup spaghetti grew historically).

The class inheritance appears elegant and makes the hierarchical structure of the function calls more transparent than the previous procedural labyrinth.

Strictly speaking I can't conclude with a thank you for a patch because I didn't find a deeply hidden issue, but I say thank for writing the patch based on what I've seen.

binaries/data/mods/public/gui/reference/common/Loader.js
14 ↗(On Diff #11873)

Loader.prototype.AuraPath = "simulation/data/auras/"; for constants (separation of data and code)

145 ↗(On Diff #11873)

perhaps production.techs.splice(0, 0 clone(this.loadTechnologyPairTemplate(technologyName, civCode).techs))
but this function returns an object not an array, does that even work? other than that, the function could also insert into the array directly

binaries/data/mods/public/gui/reference/common/Parser.js
4 ↗(On Diff #11873)

Parser is a too common name, should be TemplateParser or similar so that one is still free to add many other parsers without wondering which one is which

275 ↗(On Diff #11873)

(en-US)

binaries/data/mods/public/gui/reference/common/ReferencePage.js
11
	/* This class possesses static fields. Unfortunately, the version of
	 * SpiderMonkey we're using does not support the syntax to add them
	 * within the body of the class. Also, as class definitions are not
	 * hoisted, they are defined at the bottom of this file.
	 */

This refers to StatsFunctions?

ReferencePage.prototype.StatsFunctions = [func1, func2, ...]; doesnt work?

binaries/data/mods/public/gui/reference/structree/EntityBox.js
110 ↗(On Diff #11880)

EntityBox.prototype.boxMinWidth = 96? I guess its because of the static functions. Any reason we need static functions to begin with? I hadn't used them so far (also not getter and setter syntax) but perhaps there is good reason to?

binaries/data/mods/public/gui/reference/structree/StructreePage.js
16

guiObjects or guiElements

41

I did not check if the the anonymous function () => this.selectCiv(civSelection.list_data[civSelection.selected]); is faster than bind this.selectCiv.bind(this);, either way I suppose its negligible considering how often the function is called (once per click).

50

(In many classes one can find foo.prototype.FooTooltip = translate("%(hotkey)s: Close Structure Tree.") and foo.prototype.FooHotkey = "cancel". It can alllow mods to change the strings, furthers separation of data and code, in few cases the hotkey is reused, otherwise its not so important to spend the time splitting that and may be ignored)

Hotkeys can change in the future (and for mods I guess), then the tooltip should also become updated (in the hypothetical future then).
(I had started writing that code for the session but didn't finish it.
There is an event/subscription based system implemented in the session, so that the many session GUI components internals (function names) then won't be hardwired in the session global logic but rather can communicate with one another.
For hotkey events, the idea was that the hotkey editor or options dialog or config system triggers a message to any subscribing GUI class. So one can find some "onHotkeyChange" functions in session that arent triggered yet except on init.)

binaries/data/mods/public/gui/reference/structree/StructureBox.js
27 ↗(On Diff #11880)

Separation of concerns - The task to draw a ProductionQueue item type 1 (unit) and a ProductionQueue type 2 (tech), the task to draw Upgrade components and the task to distribute boxes could be split into separate functions.

A disadvantage of having more functions is that it becomes less transparent which of these functions are called from other files ("public") and which ones are called only internally ("private").

An advantage of having more functions is that mods (for example nanis autociv mod) can hook into existing functions (replace them with functions that call the previous function and add some statements for example).

Another advantage of having more functions, especially in this case, is that it could ease introduction of new components.
If a simulation author adds a new Component and sees there are functions for SimulationComponent1 and SimulationComponent2, it will be inviting to add a function for SimulationComponent3.
Depending on how far one wants to open up that room, one could even split them into different subclasses somehow.

I dont mind how it's implemented, just mentioning this as an alternative with possible pros and cons.

97 ↗(On Diff #11880)

(StructreePage.getPositionOffset = this.getPositionOffset if its not static I think)

binaries/data/mods/public/gui/reference/structree/TrainerBox.js
13 ↗(On Diff #11873)

(the less names one uses, the less indirection there will be and the less time it consumes to look up which code refers to which GUI object and which GUI object refers to which code)

		this.trainer = Engine.GetGUIObjectByName("trainer[" + this.guiIdx + "]");
		this.trainer_row = Engine.GetGUIObjectByName("trainer[" + this.guiIdx + "]_row");

perhaps

		this.trainer_row = Engine.GetGUIObjectByName("trainer_row[" + this.guiIdx + "]");
59 ↗(On Diff #11880)

This loop seems duplicated, I would need to run the code to see how the duplication could be minimized ideally. I remember the structree deduplication topic before - it doesnt have to be performed in this diff, and perhaps it already was partially performed using the prototype inheritance (class extends).

88 ↗(On Diff #11880)

super.captionWidth() -> this.captionWidth() unless you really need to call the inherited function (for example when wanting to call super.captionWidth() from this.captionWidth(). This way for example mods can replace the captionWidth function with a custom function and its getting called, without having to overwrite the protoype property. (There could even be class1 inheriting class2 inheriting class3 and the topmost function would be called, not that it matters significantly)

binaries/data/mods/public/gui/reference/structree/draw.js
1

(That 28 sounds like width of a button, could be obtained from the size property maybe, and 4 could be a prototype constant named Margin or so)

binaries/data/mods/public/gui/reference/viewer/ViewerPage.js
18

prototype const

119

(maybe prototype property for the path to split data from code, doesnt matter)

Stan added a subscriber: Stan.May 16 2020, 7:57 PM
Stan added inline comments.
binaries/data/mods/public/gui/reference/structree/structree.xml
62–63

I believe those are migrated to the js code as well in the gamesetup patch.

s0600204 updated this revision to Diff 11965.May 22 2020, 5:58 AM
s0600204 marked 16 inline comments as done.

Break structree gui up further into separate files and classes

Also:

  • Replace all "static" fields with prototype members
  • Rename the three Template handling classes
  • Reuse new CloseButton class (and xml) for viewer page as well
  • Sort new classes into subfolders

@elexis: I thank you for your thanks. I figured you would appreciate the intent of the proposed change, even if you disagreed with parts of the actual implementation. I acknowledge that you had not and do not request any changes, and I thank you for sharing your insight.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/779/display/redirect

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/Sections/Tree/TreeSection.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/Sections/Tree/TreeSection.js
|  43|  43| 
|  44|  44| 		phaseIcon.sprite = "stretched:" + ReferencePage.prototype.IconPath + prodPhaseTemplate.icon;
|  45|  45| 		phaseIcon.tooltip = getEntityNamesFormatted(prodPhaseTemplate);
|  46|    |-	};
|    |  46|+	}
|  47|  47| 
|  48|  48| 	onTrainerSectionWidthChange(trainerSectionWidth, trainerSectionVisible)
|  49|  49| 	{
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/Sections/Tree/TreeSection.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/Sections/Tree/TreeSection.js
|  71|  71| 		size += rowHeight * (phases * idx - (idx - 1) * idx / 2); // phase rows (phase-currphase+1 per row)
|  72|  72| 
|  73|  73| 		return size;
|  74|    |-	};
|    |  74|+	}
|  75|  75| 
|  76|  76| }

binaries/data/mods/public/gui/reference/structree/Sections/Tree/TreeSection.js
|  46| »   };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.

binaries/data/mods/public/gui/reference/structree/Sections/Tree/TreeSection.js
|  74| »   };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/Boxes/ProductionRow.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/Boxes/ProductionRow.js
|  52|  52| 
|  53|  53| 	hide()
|  54|  54| 	{
|  55|    |-		this.productionRow.hidden = true
|    |  55|+		this.productionRow.hidden = true;
|  56|  56| 	}
|  57|  57| }

binaries/data/mods/public/gui/reference/structree/Boxes/ProductionRow.js
|  55| »   »   this.productionRow.hidden·=·true
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/Sections/Tree/PhaseIdent.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/Sections/Tree/PhaseIdent.js
|  52|  52| 
|  53|  53| 		phaseIcon.sprite = "stretched:" + ReferencePage.prototype.IconPath + prodPhaseTemplate.icon;
|  54|  54| 		phaseIcon.tooltip = getEntityNamesFormatted(prodPhaseTemplate);
|  55|    |-	};
|    |  55|+	}
|  56|  56| 
|  57|  57| }

binaries/data/mods/public/gui/reference/structree/Sections/Tree/PhaseIdent.js
|  55| »   };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.

binaries/data/mods/public/gui/reference/common/Dropdowns/CivSelectDropdown.js
|  45| »   »   return·!this.civSelection.list.length·==·0;
|    | [NORMAL] JSHintBear:
|    | Confusing use of '!'.
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/Boxes/ProductionRowManager.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/reference/structree/Boxes/ProductionRowManager.js
|  46|  46| 				}
|  47|  47| 
|  48|  48| 				let rowIdx = this.sortProductionsByPhase ? Math.max(0, pIdx - phaseIdx) : 0;
|  49|    |-				this.productionRows[rowIdx].drawIcon(prod, civCode)
|    |  49|+				this.productionRows[rowIdx].drawIcon(prod, civCode);
|  50|  50| 			}
|  51|  51| 
|  52|  52| 		if (template.upgrades)

binaries/data/mods/public/gui/reference/structree/Boxes/ProductionRowManager.js
|  49| »   »   »   »   this.productionRows[rowIdx].drawIcon(prod,·civCode)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2192/display/redirect

Angen added a subscriber: Angen.May 22 2020, 8:45 AM

Hi, nice work :)

binaries/data/mods/public/gui/reference/common/Dropdowns/CivSelectDropdown.js
24

this could be inlined

33

possibly not needed, this.handlers is Set so adding same thing will not insert it into the set

41

I think you can call delete without previous if, nothing will happen if it is not there

46

! could be moved to == and become != I think that would be cleaner to read

Stan added inline comments.May 22 2020, 9:28 AM
binaries/data/mods/public/gui/reference/common/Dropdowns/CivSelectDropdown.js
46

I believe it does the left computation first then check == 0 so it should be changed indeed. js is weird.

(So still thanks and no objections for this iteration of this patch, just some thoughts and observations and this batch of inline comments may be ignored, and the one with parentheses even more so than the others)

binaries/data/mods/public/gui/reference/common/Buttons/CivInfoButton.js
11

(The hotkey identifier "civinfo" can also be moved to a prototype constant, that's what some other classes do because the string is reused in some places, for example one use of the string to assign the hotkey and one use of the string to string to construct a hotkey tooltip. Now where I see that, I wonder whether the hotkey assignment could be in here.)

binaries/data/mods/public/gui/reference/common/ReferencePage.js
9

"gaia" could be this.DefaultCiv, string is reused in multiple places too. Even across multiple classes if I see correctly, which might make it a bit indirect to refer to with a single reference however.

binaries/data/mods/public/gui/reference/common/TemplateLister.js
26

(Could be (let templatesToParse = civData[civCode].StartEntities.map(entity => entity.Template); but I didn't measure if its relevantly slower with the function call)

binaries/data/mods/public/gui/reference/common/TemplateLoader.js
58

(["GenericName", "SpecificName", "Tooltip", "History"]` could be a prototype property:

  • It can be argued that it decreases cohesion.
  • It can be argued it furthers separation of code and data thus increasing ability to exchange data without having to change or read code and perhaps making changing this easier for modders.)
94

(load tech and load aura are the same function except with 3 arguments differing, which I guess isnt sufficient amount for deduplication when comparison to the 5 lines of code.)

binaries/data/mods/public/gui/reference/common/common.js
29

If following JS GUI class rewrite consequently, tooltips.js file would become some Tooltip class too.

Then the callers to these two functions could become this.templateLoader.GetTemplateData and this.templateLoader.TechnologyTemplateExists.

(Once the two functions have left global scope, there are only two globals variables remaining here.
If all globals are defined in the first file the page opens, then there won't be hidden surprise globals for the reader.
The danger is that the file will contain more new procedural code as long as it exists.
For those two reasons, this file should be removed (similar pattern with general "common" GUI code that actually is very specific most often))

binaries/data/mods/public/gui/reference/common/tooltips.js
2

(class FooTooltips {}

FooTooltips.BuildListText = function() { ... }; (or static function?)

ViewerPage.prototype.InfoFunctions = [
getEntityTooltip,
FooTooltips.getResearchedByText,

would this work and be more in line with OOP?)

binaries/data/mods/public/gui/reference/structree/Boxes/EntityBox.js
58

Its the same 96?

binaries/data/mods/public/gui/reference/structree/Boxes/ProductionIcon.js
20

(I dont know how often this function is called. If its quite often, then Engine.GetGUIObjectByName("phase[0]_bar[0]_icon").size could be considered for caching in a class instance.)

binaries/data/mods/public/gui/reference/structree/Sections/Tree/PhaseIdentManager.js
14

(The name draw (and render as I saw in a different diff) seem somewhat misleading, because GUI Objects are drawn and rendered regardless of what JS does. So when I read this.Idents[i].draw I have no information as to what one might expect in that function. Granted that a setup, build, or updatefunction name as seen in some of my commits are just as indescriptive, so I guess it's arbitrary here.)

binaries/data/mods/public/gui/reference/structree/Sections/Tree/TreeSection.js
54

this.SectionGap? this.page.SectionGap?

Accessing StructreePage.prototype can often be ugly and deprives ability to access the actual instance to utilize its state and caching. Using something like this.page gains that ability at the cost of allowing for new kinds of unanticipating dependencies. Here this number is used in only one place, so it could be moved to this class' prototype.

However I see more references to ReferencePage.prototype and that class inherits StructreePage, so having a this.page already seems to be the case, just being restricted to static members and losing inheritance by having to distinguish using hardcoding between the two page classes.

s0600204 updated this revision to Diff 12364.Thu, Jun 18, 12:20 AM
s0600204 marked 8 inline comments as done.

Rebase, and update in response to line comments

binaries/data/mods/public/gui/reference/common/tooltips.js
2

I was hoping the gui/common/tooltips.js file would be refactored into a class at some point (potentially removing 50+ items from the global scope in the process), whereupon these could become member functions of said class, added when this page loaded.

binaries/data/mods/public/gui/reference/structree/Boxes/EntityBox.js
58

Heh, no.

IconAndCaptionHeight was the combined height of the icon, the caption, plus some padding for taste.

MinWidth is an arbitrary number, chosen because it led to an outcome that "looked" okay.

That they both turned out the same is pure coincidence.

I've rewritten the code so IconAndCaptionHeight is calculated rather than being hard-coded. (I'm open to alternate names, though.)

binaries/data/mods/public/gui/reference/structree/Boxes/ProductionIcon.js
20

This function (in this form) is called once, as it replaces itself in its second-to-last line.

Hence the comment about assumptions above, and the comments about replacement both above and below.

binaries/data/mods/public/gui/reference/structree/Sections/Tree/TreeSection.js
54

ReferencePage does not inherit from StructreePage. It's the other way round.

However, this should now be handled more to your satisfaction.

s0600204 removed a child revision: Restricted Differential Revision.Thu, Jun 18, 12:51 AM