Changeset View
Standalone View
binaries/data/mods/public/gui/lobby/LobbyPage/PlayerList.js
/** | /** | ||||||||||
* This class is concerned with displaying players who are online and | * This class is concerned with displaying players who are online and | ||||||||||
nwtour: It is not very clear how it is planned to work
The variable is declared here
gamedescription. | |||||||||||
* triggering handlers when selecting or doubleclicking on a player. | * triggering handlers when selecting or doubleclicking on a player. | ||||||||||
Done Inline ActionsIt's not just player symbol, it's at least g_PlayerStateSymbol. vladislavbelov: It's not just player symbol, it's at least `g_PlayerStateSymbol`. | |||||||||||
*/ | */ | ||||||||||
class PlayerList | class PlayerList | ||||||||||
Done Inline Actions
Silier: | |||||||||||
{ | { | ||||||||||
constructor(xmppMessages, buddyButton, gameList) | constructor(xmppMessages, buddyButton, gameList) | ||||||||||
Done Inline ActionsVariable for each state of a list sounds redundant. I believe colors might be extracted from PlayerStatuses. vladislavbelov: Variable for each state of a list sounds redundant. I believe colors might be extracted from… | |||||||||||
{ | { | ||||||||||
Not Done Inline Actions
since playerlist ist a class and you use playerstatesymbol only here, you can do this: Silier: since playerlist ist a class and you use playerstatesymbol only here, you can do this:
| |||||||||||
Done Inline ActionsYes i have tried that before, but for some reason it returns undefined. Triple checked for spelling errors. Grapjas: Yes i have tried that before, but for some reason it returns undefined. Triple checked for… | |||||||||||
this.gameList = gameList; | this.gameList = gameList; | ||||||||||
this.selectedPlayer = undefined; | this.selectedPlayer = undefined; | ||||||||||
this.statusOrder = Object.keys(this.PlayerStatuses); | this.statusOrder = Object.keys(this.PlayerStatuses); | ||||||||||
// Avoid repeated array construction for performance | // Avoid repeated array construction for performance | ||||||||||
this.buddyStatusList = []; | |||||||||||
this.playerList = []; | this.playerList = []; | ||||||||||
this.presenceList = []; | this.presenceList = []; | ||||||||||
this.nickList = []; | this.nickList = []; | ||||||||||
this.ratingList = []; | this.ratingList = []; | ||||||||||
this.playersFilter = Engine.GetGUIObjectByName("playersFilter"); | this.playersFilter = Engine.GetGUIObjectByName("playersFilter"); | ||||||||||
this.playersFilter.onPress = this.selectPlayer.bind(this); | this.playersFilter.onPress = this.selectPlayer.bind(this); | ||||||||||
this.playersFilter.onTab = this.autocomplete.bind(this); | this.playersFilter.onTab = this.autocomplete.bind(this); | ||||||||||
this.playersFilter.tooltip = colorizeAutocompleteHotkey(); | this.playersFilter.tooltip = colorizeAutocompleteHotkey(); | ||||||||||
this.selectionChangeHandlers = new Set(); | this.selectionChangeHandlers = new Set(); | ||||||||||
this.mouseLeftDoubleClickItemHandlers = new Set(); | this.mouseLeftDoubleClickItemHandlers = new Set(); | ||||||||||
this.playersBox = Engine.GetGUIObjectByName("playersBox"); | this.playersBox = Engine.GetGUIObjectByName("playersBox"); | ||||||||||
this.playersBox.onSelectionChange = this.onPlayerListSelection.bind(this); | this.playersBox.onSelectionChange = this.onPlayerListSelection.bind(this); | ||||||||||
this.playersBox.onSelectionColumnChange = this.rebuildPlayerList.bind(this); | this.playersBox.onSelectionColumnChange = this.rebuildPlayerList.bind(this); | ||||||||||
this.playersBox.onMouseLeftClickItem = this.onMouseLeftClickItem.bind(this); | this.playersBox.onMouseLeftClickItem = this.onMouseLeftClickItem.bind(this); | ||||||||||
this.playersBox.onMouseLeftDoubleClickItem = this.onMouseLeftDoubleClickItem.bind(this); | this.playersBox.onMouseLeftDoubleClickItem = this.onMouseLeftDoubleClickItem.bind(this); | ||||||||||
Done Inline Actions
(Just because you can) Stan: (Just because you can) | |||||||||||
Done Inline ActionsThat's really unreadable and too much duplication, it might be like: this.playersBox.tooltip = "[font=\"sans-bold-14\"]" + this.PlayerStatuses.reduce((all, status) => all.concat("[color=\"" + status.tag.color + "\"]" + g_PlayerSymbol + "[/color]" + status.status + "\n")) + "[/font]"; vladislavbelov: That's really unreadable and too much duplication, it might be like:
```lang=js
this.playersBox. | |||||||||||
Done Inline ActionsOh I thought this was debug code XD Stan: Oh I thought this was debug code XD | |||||||||||
Done Inline Actions"this.PlayerStatuses.reduce is not a function" Grapjas: "this.PlayerStatuses.reduce is not a function" | |||||||||||
Done Inline Actions
Silier: | |||||||||||
for (let key in this.PlayerStatuses) | |||||||||||
Done Inline Actions
Silier: | |||||||||||
this.playersBox.tooltip += setStringTags("[color=\"" + this.PlayerStatuses[key].tags.color + "\"]" + this.PlayerStatuses[key].symbol + "[/color] " + this.PlayerStatuses[key].status + "\n", { "font": "sans-bold-14" }); | |||||||||||
Done Inline Actionsit would be better if you could use setStringtags function here Silier: it would be better if you could use setStringtags function here | |||||||||||
Done Inline Actions
this is wrong usage of the function, if there is some function for the task, it should be used. and then somewhere in the file PlayerList.prototype.TooltipTags = { "font": "sans-bold-14" }; Silier: this is wrong usage of the function, if there is some function for the task, it should be used. | |||||||||||
Not Done Inline Actionsnot done Silier: not done | |||||||||||
Done Inline ActionsI don't see the point of this, can you explain? From what i see setStringTags is now used correctly. How is specifying somewhere in the file better when it's only used in 1 place and takes more lines of code? Grapjas: I don't see the point of this, can you explain? From what i see setStringTags is now used… | |||||||||||
buddyButton.registerBuddyChangeHandler(this.onBuddyChange.bind(this)); | buddyButton.registerBuddyChangeHandler(this.onBuddyChange.bind(this)); | ||||||||||
xmppMessages.registerPlayerListUpdateHandler(this.rebuildPlayerList.bind(this)); | xmppMessages.registerPlayerListUpdateHandler(this.rebuildPlayerList.bind(this)); | ||||||||||
this.registerSelectionChangeHandler(buddyButton.onPlayerSelectionChange.bind(buddyButton)); | this.registerSelectionChangeHandler(buddyButton.onPlayerSelectionChange.bind(buddyButton)); | ||||||||||
this.registerMouseLeftDoubleClickItemHandler(buddyButton.onPress.bind(buddyButton)); | this.registerMouseLeftDoubleClickItemHandler(buddyButton.onPress.bind(buddyButton)); | ||||||||||
this.rebuildPlayerList(); | this.rebuildPlayerList(); | ||||||||||
} | } | ||||||||||
▲ Show 20 Lines • Show All 53 Lines • ▼ Show 20 Lines | class PlayerList | ||||||||||
} | } | ||||||||||
parsePlayer(sortKey, player) | parsePlayer(sortKey, player) | ||||||||||
{ | { | ||||||||||
player.isBuddy = g_Buddies.indexOf(player.name) != -1; | player.isBuddy = g_Buddies.indexOf(player.name) != -1; | ||||||||||
switch (sortKey) | switch (sortKey) | ||||||||||
{ | { | ||||||||||
case 'buddy': | |||||||||||
player.sortValue = (player.isBuddy ? 1 : 2) + this.statusOrder.indexOf(player.presence) + player.name.toLowerCase(); | |||||||||||
break; | |||||||||||
case 'rating': | case 'rating': | ||||||||||
player.sortValue = +player.rating; | player.sortValue = + player.rating; | ||||||||||
Done Inline Actionsbtw this is change is not correct Silier: btw this is change is not correct
the `+` is supposed to be next to `player.rating` as it is… | |||||||||||
break; | break; | ||||||||||
case 'status': | case 'status': | ||||||||||
player.sortValue = this.statusOrder.indexOf(player.presence) + player.name.toLowerCase(); | player.sortValue = (player.isBuddy ? 1 : 2) + this.statusOrder.indexOf(player.presence) + player.name.toLowerCase(); | ||||||||||
break; | break; | ||||||||||
case 'name': | case 'name': | ||||||||||
default: | default: | ||||||||||
player.sortValue = player.name.toLowerCase(); | player.sortValue = player.name.toLowerCase(); | ||||||||||
break; | break; | ||||||||||
} | } | ||||||||||
} | } | ||||||||||
/** | /** | ||||||||||
* Do a full update of the player listing, including ratings from cached C++ information. | * Do a full update of the player listing, including ratings from cached C++ information. | ||||||||||
* Important: This should only be performed once if | * Important: This should only be performed once if | ||||||||||
* there have been multiple messages received changing this list. | * there have been multiple messages received changing this list. | ||||||||||
*/ | */ | ||||||||||
rebuildPlayerList() | rebuildPlayerList() | ||||||||||
{ | { | ||||||||||
Engine.ProfileStart("rebuildPlaersList"); | Engine.ProfileStart("rebuildPlayersList"); | ||||||||||
Done Inline ActionsJust noticed this, isn't it a typo? Grapjas: Just noticed this, isn't it a typo? | |||||||||||
Done Inline ActionsIt is. It has no obvious visual impact because it's used to label the performance of that page. (Might show up if you press F11) At least present since rP23172 maybe earlier. Stan: It is. It has no obvious visual impact because it's used to label the performance of that page. | |||||||||||
Engine.ProfileStart("getPlayerList"); | Engine.ProfileStart("getPlayerList"); | ||||||||||
let playerList = Engine.GetPlayerList(); | let playerList = Engine.GetPlayerList(); | ||||||||||
Engine.ProfileStop(); | Engine.ProfileStop(); | ||||||||||
Engine.ProfileStart("parsePlayers"); | Engine.ProfileStart("parsePlayers"); | ||||||||||
playerList.forEach(this.parsePlayer.bind(this, this.playersBox.selected_column)); | playerList.forEach(this.parsePlayer.bind(this, this.playersBox.selected_column)); | ||||||||||
Engine.ProfileStop(); | Engine.ProfileStop(); | ||||||||||
Engine.ProfileStart("sortPlayers"); | Engine.ProfileStart("sortPlayers"); | ||||||||||
playerList.sort(this.sortPlayers.bind(this, this.playersBox.selected_column_order)); | playerList.sort(this.sortPlayers.bind(this, this.playersBox.selected_column_order)); | ||||||||||
Engine.ProfileStop(); | Engine.ProfileStop(); | ||||||||||
Engine.ProfileStart("prepareList"); | Engine.ProfileStart("prepareList"); | ||||||||||
let length = playerList.length; | let length = playerList.length; | ||||||||||
this.buddyStatusList.length = length; | |||||||||||
this.playerList.length = length; | this.playerList.length = length; | ||||||||||
this.presenceList.length = length; | this.presenceList.length = length; | ||||||||||
this.nickList.length = length; | this.nickList.length = length; | ||||||||||
this.ratingList.length = length; | this.ratingList.length = length; | ||||||||||
playerList.forEach((player, i) => { | playerList.forEach((player, i) => { | ||||||||||
// TODO: COList.cpp columns should support horizontal center align | // TODO: COList.cpp columns should support horizontal center align | ||||||||||
let rating = player.rating ? (" " + player.rating).substr(-5) : " -"; | let rating = player.rating ? (" " + player.rating).substr(-5) : " -"; | ||||||||||
let presence = this.PlayerStatuses[player.presence] ? player.presence : "unknown"; | let presence = this.PlayerStatuses[player.presence] ? player.presence : "unknown"; | ||||||||||
if (presence == "unknown") | if (presence == "unknown") | ||||||||||
warn("Unknown presence:" + player.presence); | warn("Unknown presence:" + player.presence); | ||||||||||
// let randomRatingTest = Math.floor(Math.random() * 2500); | |||||||||||
let statusTags = this.PlayerStatuses[presence].tags; | let statusTags = this.PlayerStatuses[presence].tags; | ||||||||||
this.buddyStatusList[i] = player.isBuddy ? setStringTags(g_BuddySymbol, statusTags) : ""; | this.playerList[i] = coloredText(this.ApplyTagByRole(player.name, player.role), PlayerColor.ColorPlayerNameByRole(player.role)); | ||||||||||
this.playerList[i] = PlayerColor.ColorPlayerName(player.name, "", player.role); | this.presenceList[i] = player.isBuddy ? setStringTags(g_BuddySymbol, statusTags) : setStringTags(this.PlayerStatuses[presence].symbol, statusTags); | ||||||||||
this.presenceList[i] = setStringTags(this.PlayerStatuses[presence].status, statusTags); | // this.ratingList[i] = coloredText(randomRatingTest, PlayerColor.GetColorByRating(randomRatingTest)); | ||||||||||
this.ratingList[i] = setStringTags(rating, statusTags); | this.ratingList[i] = coloredText(rating, PlayerColor.GetColorByRating(rating)); | ||||||||||
Done Inline Actionsmake this line a comment and make L156 & L160 normal to test player rating colors. Grapjas: make this line a comment and make L156 & L160 normal to test player rating colors. | |||||||||||
Done Inline Actionshmm, why not just keep the original function to return the color and build the tag here, or make addition function for that. Silier: hmm, why not just keep the original function to return the color and build the tag here, or… | |||||||||||
this.nickList[i] = escapeText(player.name); | this.nickList[i] = escapeText(player.name); | ||||||||||
}); | }); | ||||||||||
Engine.ProfileStop(); | Engine.ProfileStop(); | ||||||||||
Engine.ProfileStart("copyToGUI"); | Engine.ProfileStart("copyToGUI"); | ||||||||||
this.playersBox.list_buddy = this.buddyStatusList; | |||||||||||
this.playersBox.list_name = this.playerList; | this.playersBox.list_name = this.playerList; | ||||||||||
this.playersBox.list_status = this.presenceList; | this.playersBox.list_status = this.presenceList; | ||||||||||
this.playersBox.list_rating = this.ratingList; | this.playersBox.list_rating = this.ratingList; | ||||||||||
this.playersBox.list = this.nickList; | this.playersBox.list = this.nickList; | ||||||||||
Engine.ProfileStop(); | Engine.ProfileStop(); | ||||||||||
Engine.ProfileStart("selectionChange"); | Engine.ProfileStart("selectionChange"); | ||||||||||
this.playersBox.selected = this.playersBox.list.indexOf(this.selectedPlayer); | this.playersBox.selected = this.playersBox.list.indexOf(this.selectedPlayer); | ||||||||||
Show All 9 Lines | sortPlayers(sortOrder, player1, player2) | ||||||||||
if (player1.sortValue > player2.sortValue) | if (player1.sortValue > player2.sortValue) | ||||||||||
return +sortOrder; | return +sortOrder; | ||||||||||
return 0; | return 0; | ||||||||||
} | } | ||||||||||
} | } | ||||||||||
// Prepends the moderator tag if true | |||||||||||
PlayerList.prototype.ApplyTagByRole = function(name, role) | |||||||||||
{ | |||||||||||
return (role == "moderator") ? escapeText("@" + name) : escapeText(name); | |||||||||||
}; | |||||||||||
const g_PlayerStateSymbol = "●"; | |||||||||||
Done Inline Actionsthis is actually wrong Silier: this is actually wrong
`this` does not refer to PlayerList class | |||||||||||
/** | /** | ||||||||||
* The playerlist will be assembled using these values. | * The playerlist will be assembled using these values. | ||||||||||
*/ | */ | ||||||||||
PlayerList.prototype.PlayerStatuses = { | PlayerList.prototype.PlayerStatuses = { | ||||||||||
"available": { | "available": { | ||||||||||
"status": translate("Online"), | "status": translate("Online"), | ||||||||||
Done Inline Actionssimilar string to this should be put here under chosen variable and used in L36 Silier: similar string to this should be put here under chosen variable and used in L36 | |||||||||||
"symbol": g_PlayerStateSymbol, | |||||||||||
"tags": { | "tags": { | ||||||||||
"color": "0 219 0" | "color": "0 219 0" | ||||||||||
} | } | ||||||||||
}, | }, | ||||||||||
"away": { | "away": { | ||||||||||
"status": translate("Away"), | "status": translate("Away"), | ||||||||||
"symbol": g_PlayerStateSymbol, | |||||||||||
"tags": { | "tags": { | ||||||||||
"color": "255 127 0" | "color": "255 127 0" | ||||||||||
} | } | ||||||||||
}, | }, | ||||||||||
"playing": { | "playing": { | ||||||||||
"status": translate("Busy"), | "status": translate("Busy"), | ||||||||||
"symbol": g_PlayerStateSymbol, | |||||||||||
Done Inline Actionsg_PlayerSymbol? Also, you probably don't want people to translate those... Stan: g_PlayerSymbol? Also, you probably don't want people to translate those... | |||||||||||
Done Inline Actions
Ofc i do, ● in english is obviously ● in dutch. Grapjas: > g_PlayerSymbol? Also, you probably don't want people to translate those...
Ofc i do, ● in… | |||||||||||
"tags": { | "tags": { | ||||||||||
"color": "200 0 0" | "color": "200 0 0" | ||||||||||
} | } | ||||||||||
}, | }, | ||||||||||
"offline": { | "offline": { | ||||||||||
"status": translate("Offline"), | "status": translate("Offline"), | ||||||||||
"symbol": g_PlayerStateSymbol, | |||||||||||
Done Inline ActionsNew players don't know what these colors mean, it should be explained at least in tooltips. vladislavbelov: New players don't know what these colors mean, it should be explained at least in tooltips. | |||||||||||
Done Inline ActionsI agree we need a tooltip and/or legend with colors Stan: I agree we need a tooltip and/or legend with colors | |||||||||||
"tags": { | "tags": { | ||||||||||
"color": "0 0 0" | "color": "0 0 0" | ||||||||||
} | } | ||||||||||
}, | }, | ||||||||||
"unknown": { | "unknown": { | ||||||||||
"status": translateWithContext("lobby presence", "Unknown"), | "status": translateWithContext("lobby presence", "Unknown"), | ||||||||||
"symbol": g_PlayerStateSymbol, | |||||||||||
Done Inline Actionsforgotten Silier: forgotten | |||||||||||
"tags": { | "tags": { | ||||||||||
"color": "178 178 178" | "color": "178 178 178" | ||||||||||
} | } | ||||||||||
} | } | ||||||||||
}; | }; |
It is not very clear how it is planned to work
The variable is declared here
gamedescription.js: var g_BuddySymbol = '•';
It is logical to immediately define it with a new symbol than reassign