Changeset View
Standalone View
binaries/data/mods/public/gui/summary/summary.js
const g_MaxHeadingTitle= 9; | const g_MaxHeadingTitle= 9; | ||||
// const for filtering long collective headings | // const for filtering long collective headings | ||||
const g_LongHeadingWidth = 250; | const g_LongHeadingWidth = 250; | ||||
const g_PlayerBoxYSize = 40; | const g_PlayerBoxYSize = 40; | ||||
const g_PlayerBoxGap = 2; | const g_PlayerBoxGap = 2; | ||||
const g_PlayerBoxAlpha = " 50"; | const g_PlayerBoxAlpha = " 50"; | ||||
const g_PlayerColorBoxAlpha = " 255"; | const g_PlayerColorBoxAlpha = " 255"; | ||||
const g_TeamsBoxYStart = 40; | const g_TeamsBoxYStart = 40; | ||||
// Colors used for units and buildings | // Colors used for units and buildings | ||||
const g_TrainedColor = '[color="201 255 200"]'; | const g_SummaryColors = { | ||||
const g_LostColor = '[color="255 213 213"]'; | "trained": '[color="201 255 200"]', | ||||
const g_KilledColor = '[color="196 198 255"]'; | "constructed": '[color="201 255 200"]', | ||||
const g_CapturedColor = '[color="255 255 157"]'; | "lost": '[color="255 213 213"]', | ||||
"killed": '[color="196 198 255"]', | |||||
const g_BuildingsTypes = [ "total", "House", "Economic", "Outpost", "Military", "Fortress", "CivCentre", "Wonder" ]; | "destroyed": '[color="196 198 255"]', | ||||
const g_UnitsTypes = [ "total", "Infantry", "Worker", "Cavalry", "Champion", "Hero", "Siege", "Ship", "Trader" ]; | "captured": '[color="255 255 157"]', | ||||
"income": '[color="201 255 200"]', | |||||
// Colors used for gathered and traded resources | "gathered": '[color="201 255 200"]', | ||||
const g_IncomeColor = '[color="201 255 200"]'; | "sent": '[color="201 255 200"]', | ||||
const g_OutcomeColor = '[color="255 213 213"]'; | "bought": '[color="201 255 200"]', | ||||
"outcome": '[color="255 213 213"]', | |||||
"used": '[color="255 213 213"]', | |||||
"received": '[color="255 213 213"]', | |||||
"sold": '[color="255 213 213"]' | |||||
}; | |||||
elexis: The color closing tag can be added if and only if the color element is defined.
Those three… | |||||
Not Done Inline Actions
What if the postfix should be colored too for a new value some day?
As thats not sure, I'll leave it as is
I think thats ok as it is, though it could be postfix isn't the best word for this property. Imarok: > The color closing tag can be added if and only if the color element is defined.
What if the… | |||||
Not Done Inline ActionsThen the slash should explicitly add color tags. It's really bad to have the opening tag in one place of the code and the closing tag on the other side of the world. elexis: Then the slash should explicitly add color tags.
It's really bad to have the opening tag in… | |||||
Not Done Inline Actionsthat's because the colors are not only used together with the postfix. Imarok: that's because the colors are not only used together with the postfix. | |||||
const g_TypeTranslations = { | |||||
"percent": "%", | |||||
"constructed": translate("Constructed"), | |||||
"trained": translate("Trained"), | |||||
"captured": translate("Captured"), | |||||
"gathered": translate("Gathered"), | |||||
"sent": translate("Sent"), | |||||
"bought": translate("Bought"), | |||||
"income": translate("Income"), | |||||
"destroyed": translate("Destroyed"), | |||||
"killed": translate("Killed"), | |||||
"lost": translate("Lost"), | |||||
"used": translate("Used"), | |||||
"received": translate("Received"), | |||||
"sold": translate("Sold"), | |||||
"outcome": translate("Outcome") | |||||
}; | |||||
Done Inline ActionsThis one could be used numerous times in previous file I guess, removes duplicate translate calls. bb: This one could be used numerous times in previous file I guess, removes duplicate translate… | |||||
Not Done Inline Actionswhere? Imarok: where? | |||||
Done Inline ActionsL126, L127 f.e. (don't want to give a full list) bb: L126, L127 f.e. (don't want to give a full list) | |||||
Done Inline Actionssemicolomn bb: semicolomn | |||||
Not Done Inline Actions
elexis: * Is there a reason for the const? Otherwise, const -> var, so that modders could extend it… | |||||
Not Done Inline Actions"Some colors are reused, having globals for that might make it easier to see the pattern. Not sure if we should go for this, but should be explored. Looks like 4-5 differnet colors for 13 entries." - don't think its worth it Imarok: "Some colors are reused, having globals for that might make it easier to see the pattern. Not… | |||||
Not Done Inline Actions
That's bad IMO. I couldn't find by testing the patch where the color is actually used. If you can't thow out the percent value, give it a color too. Not sure if we need colors in the first place elexis: * Color value duplication should be avoided, I didn't find the pattern yet
* "color="… | |||||
Not Done Inline ActionsMove those color opening and closing tags to the place that uses them, so that we have actual separation of code and data plus duplication reduction elexis: Move those color opening and closing tags to the place that uses them, so that we have actual… | |||||
Not Done Inline ActionsThat would mean adding > 10 color tags... Imarok: That would mean adding > 10 color tags...
So I don't think this is a good idea. | |||||
const g_InfiniteSymbol = "\u221E"; | const g_InfiniteSymbol = "\u221E"; | ||||
var g_CivData = loadCivData(); | var g_CivData = loadCivData(); | ||||
var g_Teams = []; | var g_Teams = []; | ||||
// TODO set g_PlayerCount as playerCounters.length | // TODO set g_PlayerCount as playerCounters.length | ||||
var g_PlayerCount = 0; | var g_PlayerCount = 0; | ||||
Show All 10 Lines | function selectPanel(panel) | ||||
for (let button of Engine.GetGUIObjectByName("summaryWindow").children) | for (let button of Engine.GetGUIObjectByName("summaryWindow").children) | ||||
if (button.name.endsWith("PanelButton")) | if (button.name.endsWith("PanelButton")) | ||||
button.sprite = "BackgroundTab"; | button.sprite = "BackgroundTab"; | ||||
panel.sprite = "ForegroundTab"; | panel.sprite = "ForegroundTab"; | ||||
adjustTabDividers(panel.size); | adjustTabDividers(panel.size); | ||||
let generalPanel = Engine.GetGUIObjectByName("generalPanel"); | |||||
let chartsPanel = Engine.GetGUIObjectByName("chartsPanel"); | |||||
if (panel.name != "chartsPanelButton") | |||||
Done Inline ActionsSure you don't want to make it: let chartsVisible = panel.name == "chartsPanelButton"; generalPanel.hidden =chartsVisible; chartsPanel.hidden = !chartsVisible; if (chartsVisible) for (let i=0; i<2; ++i) updateCategoryDropdown(i); else updatePanelData(g_ScorePanelsData[panel.name.substr(0, panel.name.length - "PanelButton".length)]); ? elexis: Sure you don't want to make it:
```
let chartsVisible = panel.name == "chartsPanelButton"… | |||||
Not Done Inline ActionsNow I get your point :D Imarok: Now I get your point :D | |||||
{ | |||||
generalPanel.hidden = false; | |||||
Not Done Inline Actionshidden assignments can be pulled out of the if statement. Probably new var isBlaHidden or so elexis: hidden assignments can be pulled out of the if statement. Probably new var isBlaHidden or so | |||||
Not Done Inline ActionsWhat does this improve? Imarok: What does this improve? | |||||
Not Done Inline ActionsDuplication removal elexis: Duplication removal | |||||
Not Done Inline Actionswhich duplication? Imarok: which duplication? | |||||
chartsPanel.hidden = true; | |||||
updatePanelData(g_ScorePanelsData[panel.name.substr(0, panel.name.length - "PanelButton".length)]); | updatePanelData(g_ScorePanelsData[panel.name.substr(0, panel.name.length - "PanelButton".length)]); | ||||
Not Done Inline Actions"PanelButton".length==11 not sure what you mean by this bb: "PanelButton".length==11 not sure what you mean by this | |||||
Not Done Inline ActionsThat's the same as in the old file. A panels button name is e.g.: "chartsPanelButton" so we use the substring "charts" to access the entry in g_ScorePanelData. Imarok: That's the same as in the old file. A panels button name is e.g.: "chartsPanelButton" so we use… | |||||
Not Done Inline ActionsAnyway ugly IMO bb: Anyway ugly IMO | |||||
Not Done Inline Actionswhat should I do instead? just use 11? Imarok: what should I do instead? just use 11?
Then nobody knows why it is 11 and not 12 or 10. | |||||
Not Done Inline ActionsWe could give a comment and use 11, but indeed that wouldn't be much (if any) better, maybe leave as is. bb: We could give a comment and use 11, but indeed that wouldn't be much (if any) better, maybe… | |||||
} | } | ||||
else | |||||
Not Done Inline Actionslooks unconventional. Would be the first time we do such a thing while all the other code uses for() or map() elexis: looks unconventional. Would be the first time we do such a thing while all the other code uses… | |||||
Not Done Inline ActionsSure, but why not? ;D Imarok: Sure, but why not? ;D
(for() will be much more code and map() is not suited for this usecase) | |||||
Done Inline Actionsmissing whitespace bb: missing whitespace | |||||
{ | |||||
generalPanel.hidden = true; | |||||
chartsPanel.hidden = false; | |||||
updateCategoryDropdown(1); | |||||
updateCategoryDropdown(2); | |||||
} | |||||
} | |||||
function initCharts() | |||||
{ | |||||
let player_colors = []; | |||||
for (let i = 1; i <= g_PlayerCount; ++i) | |||||
Done Inline Actionslet i loop from 1 to g_PlayerCount + 1 bb: let i loop from 1 to g_PlayerCount + 1 | |||||
Not Done Inline Actionsfor...of? brace removal then elexis: for...of? brace removal then | |||||
Not Done Inline Actionshmm, not really, as we still have gaia in it Imarok: hmm, not really, as we still have gaia in it | |||||
{ | |||||
let playerState = g_GameData.sim.playerStates[i]; | |||||
player_colors.push( | |||||
Math.floor(playerState.color.r * 255) + " " + | |||||
Math.floor(playerState.color.g * 255) + " " + | |||||
Math.floor(playerState.color.b * 255) | |||||
Done Inline ActionsUse a loop? bb: Use a loop? | |||||
); | |||||
Not Done Inline ActionsAt some point we might want to extend color.js with a fct doing that elexis: At some point we might want to extend color.js with a fct doing that | |||||
} | |||||
let chart1 = Engine.GetGUIObjectByName("chart1"); | |||||
chart1.series_color = player_colors; | |||||
let chart2 = Engine.GetGUIObjectByName("chart2"); | |||||
chart2.series_color = player_colors; | |||||
let chartLegend = Engine.GetGUIObjectByName("chartLegend"); | |||||
chartLegend.caption = g_GameData.sim.playerStates.slice(1).map( | |||||
Done Inline Actionssuggesting to split over 2 lines bb: suggesting to split over 2 lines | |||||
(state, index) => '[color="' + player_colors[index] + '"]■[/color] ' + state.name | |||||
Not Done Inline Actionssounds familiar elexis: sounds familiar | |||||
).join(" "); | |||||
} | |||||
Done Inline Actionssplit over several lines bb: split over several lines | |||||
function resizeDropdown(dropdown) | |||||
{ | |||||
let size = dropdown.size; | |||||
size.bottom = dropdown.size.top + | |||||
(Engine.GetTextWidth(dropdown.font, dropdown.list[dropdown.selected]) > | |||||
dropdown.size.right - dropdown.size.left - 32 ? 42 : 27); | |||||
Not Done Inline ActionsCan these hardcoded numbers become replaced by coordinates of GUI objects? elexis: Can these hardcoded numbers become replaced by coordinates of GUI objects? | |||||
Not Done Inline Actionsnope, they are just empiric numbers Imarok: nope, they are just empiric numbers | |||||
Not Done Inline Actions(Still a minus point and will remain a TODO for future devs. Arbitrary numbers ought to be solely in the XML) elexis: (Still a minus point and will remain a TODO for future devs. Arbitrary numbers ought to be… | |||||
Not Done Inline ActionsThis would require a cpp function to get the possible textwidth of dropdowns. (Because that is dropdown width - dropdown arrow icon width) Imarok: This would require a cpp function to get the possible textwidth of dropdowns. (Because that is… | |||||
dropdown.size = size; | |||||
} | |||||
Not Done Inline Actionswondering if arrow function would be nice here, don't think so bb: wondering if arrow function would be nice here, don't think so | |||||
function updateCategoryDropdown(number) | |||||
{ | |||||
let chartCategory = Engine.GetGUIObjectByName("chart" + number + "CategorySelection"); | |||||
chartCategory.list_data = Object.keys(g_ScorePanelsData); | |||||
chartCategory.list = Object.keys(g_ScorePanelsData).map(panel => g_ScorePanelsData[panel].caption); | |||||
Not Done Inline ActionsThose hardcoded numbers are still meh. The numbers will be in trouble if I find time elexis: Those hardcoded numbers are still meh. The numbers will be in trouble if I find time | |||||
chartCategory.onSelectionChange = function() { | |||||
Done Inline Actionsincorrect intentation bb: incorrect intentation | |||||
if (this.list_data[this.selected]) | |||||
{ | |||||
resizeDropdown(this); | |||||
updateValueDropdown(number, this.list_data[this.selected]); | |||||
} | |||||
}; | |||||
chartCategory.selected = 0; | |||||
} | |||||
Done Inline Actionspotentially early return elexis: potentially early return | |||||
function updateValueDropdown(number, category) | |||||
{ | |||||
let chartValue = Engine.GetGUIObjectByName("chart" + number + "ValueSelection"); | |||||
let list = g_ScorePanelsData[category].headings.map(heading => heading.caption); | |||||
Not Done Inline Actionscan't these lines be merged? bb: can't these lines be merged? | |||||
Not Done Inline Actionsnope, as shift modifies the array, but just returns the removed element. Imarok: nope, as shift modifies the array, but just returns the removed element. | |||||
list.shift(); | |||||
chartValue.list = list; | |||||
let list_data = g_ScorePanelsData[category].headings.map(heading => heading.identifier); | |||||
list_data.shift(); | |||||
chartValue.list_data = list_data; | |||||
chartValue.onSelectionChange = function() { | |||||
Done Inline Actionsincorrect intentation bb: incorrect intentation | |||||
if (this.list_data[this.selected]) | |||||
{ | |||||
resizeDropdown(this); | |||||
updateTypeDropdown(number, category, this.list_data[this.selected], this.selected); | |||||
} | |||||
}; | |||||
chartValue.selected = 0; | |||||
Done Inline Actionssplit over several lines bb: split over several lines | |||||
} | |||||
function updateTypeDropdown(number, category, item, itemNumber) | |||||
{ | |||||
let testValue = g_ScorePanelsData[category].counters[itemNumber].fn(g_GameData.sim.playerStates[1], 0, item); | |||||
let hide = !g_ScorePanelsData[category].counters[itemNumber].fn || | |||||
Not Done Inline ActionsI guess last argument needs to be defined undefined? bb: I guess last argument needs to be defined undefined? | |||||
Not Done Inline Actionsyep Imarok: yep | |||||
typeof testValue != "object" || Object.keys(testValue).length < 2; | |||||
Engine.GetGUIObjectByName("chart" + number + "TypeLabel").hidden = hide; | |||||
let chartType = Engine.GetGUIObjectByName("chart" + number + "TypeSelection"); | |||||
chartType.hidden = hide; | |||||
if (hide) | |||||
{ | |||||
updateChart(number, category, item, itemNumber, Object.keys(testValue)[0] || undefined); | |||||
return; | |||||
} | |||||
chartType.list = Object.keys(testValue).map(type => g_TypeTranslations[type]); | |||||
chartType.list_data = Object.keys(testValue); | |||||
chartType.onSelectionChange = function() { | |||||
Done Inline Actionsintentation bb: intentation | |||||
if (this.list_data[this.selected]) | |||||
{ | |||||
resizeDropdown(this); | |||||
updateChart(number, category, item, itemNumber, this.list_data[this.selected]); | |||||
} | |||||
}; | |||||
chartType.selected = 0; | |||||
} | |||||
function updateChart(number, category, item, itemNumber, type) | |||||
{ | |||||
if (!g_ScorePanelsData[category].counters[itemNumber].fn) | |||||
return; | |||||
let chart = Engine.GetGUIObjectByName("chart" + number); | |||||
Done Inline Actionsj from 1 to g_PlayerCount+1 bb: j from 1 to g_PlayerCount+1 | |||||
let series = []; | |||||
for (let j = 1; j <= g_PlayerCount; ++j) | |||||
{ | |||||
let playerState = g_GameData.sim.playerStates[j]; | |||||
let data = []; | |||||
for (let index in playerState.sequences.time) | |||||
{ | |||||
let value = g_ScorePanelsData[category].counters[itemNumber].fn(playerState, index, item); | |||||
if (type) | |||||
value = value[type]; | |||||
data.push([playerState.sequences.time[index], value]); | |||||
} | |||||
series.push(data); | |||||
} | |||||
chart.series = series; | |||||
} | |||||
function adjustTabDividers(tabSize) | function adjustTabDividers(tabSize) | ||||
{ | { | ||||
let leftSpacer = Engine.GetGUIObjectByName("tabDividerLeft"); | let leftSpacer = Engine.GetGUIObjectByName("tabDividerLeft"); | ||||
let rightSpacer = Engine.GetGUIObjectByName("tabDividerRight"); | let rightSpacer = Engine.GetGUIObjectByName("tabDividerRight"); | ||||
leftSpacer.size = [ | leftSpacer.size = [ | ||||
20, | 20, | ||||
▲ Show 20 Lines • Show All 61 Lines • ▼ Show 20 Lines | for (let i = 0; i < g_PlayerCount; ++i) | ||||
setOutcomeIcon(playerState.state, playerOutcome); | setOutcomeIcon(playerState.state, playerOutcome); | ||||
Engine.GetGUIObjectByName(playerNameColumn).caption = g_GameData.sim.playerStates[i+1].name; | Engine.GetGUIObjectByName(playerNameColumn).caption = g_GameData.sim.playerStates[i+1].name; | ||||
let civIcon = Engine.GetGUIObjectByName(playerCivicBoxColumn); | let civIcon = Engine.GetGUIObjectByName(playerCivicBoxColumn); | ||||
civIcon.sprite = "stretched:" + g_CivData[playerState.civ].Emblem; | civIcon.sprite = "stretched:" + g_CivData[playerState.civ].Emblem; | ||||
civIcon.tooltip = g_CivData[playerState.civ].Name; | civIcon.tooltip = g_CivData[playerState.civ].Name; | ||||
updateCountersPlayer(playerState, panelInfo.counters, playerCounterValue); | updateCountersPlayer(playerState, panelInfo.counters, panelInfo.headings, playerCounterValue); | ||||
calculateTeamCounters(playerState); | calculateTeamCounters(playerState); | ||||
} | } | ||||
let teamCounterFn = panelInfo.teamCounterFn; | let teamCounterFn = panelInfo.teamCounterFn; | ||||
if (g_Teams && teamCounterFn) | if (g_Teams && teamCounterFn) | ||||
teamCounterFn(panelInfo.counters); | teamCounterFn(panelInfo.counters, g_GameData.sim.playerStates[1].sequences.time.length - 1); | ||||
Done Inline Actionsmissing whitespace bb: missing whitespace | |||||
} | } | ||||
function confirmStartReplay() | function confirmStartReplay() | ||||
{ | { | ||||
if (Engine.HasXmppClient()) | if (Engine.HasXmppClient()) | ||||
messageBox( | messageBox( | ||||
400, 200, | 400, 200, | ||||
translate("Are you sure you want to quit the lobby?"), | translate("Are you sure you want to quit the lobby?"), | ||||
▲ Show 20 Lines • Show All 106 Lines • ▼ Show 20 Lines | function init(data) | ||||
g_WithoutTeam = g_PlayerCount; | g_WithoutTeam = g_PlayerCount; | ||||
if (g_Teams) | if (g_Teams) | ||||
{ | { | ||||
// Count players without team (or all if teams are not displayed) | // Count players without team (or all if teams are not displayed) | ||||
for (let i = 0; i < g_Teams.length; ++i) | for (let i = 0; i < g_Teams.length; ++i) | ||||
g_WithoutTeam -= g_Teams[i] ? g_Teams[i] : 0; | g_WithoutTeam -= g_Teams[i] ? g_Teams[i] : 0; | ||||
} | } | ||||
initCharts(); | |||||
selectPanel(Engine.GetGUIObjectByName("scorePanelButton")); | selectPanel(Engine.GetGUIObjectByName("scorePanelButton")); | ||||
} | } |
The color closing tag can be added if and only if the color element is defined.
Those three elements might fit into a single line if and only if you assume they will never be extended.
This percent item still seems out of place to me, but will have to become more familiar with the code to see what we can do.
Seems a bit odd that " / " is a postfix, isn't it supposed to be a delimiter or something to that effect actually? Like "x / y" instead of "x / y /"?
-1 Tab for the closing braces
The color globals seem quite nice to me and the structure overall looks much better to me IMO