Differential D4385 Diff 19236 binaries/data/mods/public/gui/lobby/LobbyPage/PlayerList.js

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

nwtourUnsubmitted

Done

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

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.

vladislavbelovUnsubmitted

Done

It'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

SilierUnsubmitted

Done

* triggering handlers when selecting or doubleclicking on a player.

- class PlayerList

+ class PlayerList

{

constructor(xmppMessages, buddyButton, gameList)

Silier:

{ {

constructor(xmppMessages, buddyButton, gameList) constructor(xmppMessages, buddyButton, gameList)

vladislavbelovUnsubmitted

Done

Variable 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…

{ {

SilierUnsubmitted

Not Done

constructor(xmppMessages, buddyButton, gameList)

{

+ this.playerStateSymbol = "●";

this.gameList = gameList;

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:

GrapjasAuthorUnsubmitted

Done

Yes 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.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);

this.playersBox.tooltip = `[font="sans-bold-14"]★ Buddy \n[color="0 219 0"]●[color="white"] Online \n[color="255 127 0"]●[color="white"] Away \n[color="200 0 0"]●[color="white"] Busy \n[color="0 0 0"]●[color="white"] Offline`;

StanUnsubmitted

Done

this.playersBox.onMouseLeftDoubleClickItem = this.onMouseLeftDoubleClickItem.bind(this);

- this.playersBox.tooltip = `[font="sans-bold-14"]★ Buddy \n[color="0 219 0"]●[color="white"] Online \n[color="255 127 0"]●[color="white"] Away \n[color="200 0 0"]●[color="white"] Busy \n[color="0 0 0"]●[color="white"] Offline`;

+ this.playersBox.tooltip = `[font="sans-bold-14"]${g_BuddySymbol} Buddy \n[color="0 219 0"]${g_PlayerSymbol}[color="white"] Online \n[color="255 127 0"]${g_PlayerSymbol}[color="white"] Away \n[color="200 0 0"]${g_PlayerSymbol}[color="white"] Busy \n[color="0 0 0"]${g_PlayerSymbol}[color="white"] Offline`;

buddyButton.registerBuddyChangeHandler(this.onBuddyChange.bind(this));

(Just because you can)

Stan: (Just because you can)

vladislavbelovUnsubmitted

Done

That'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.

SilierUnsubmitted

Done

this.playersBox.onMouseLeftDoubleClickItem = this.onMouseLeftDoubleClickItem.bind(this);

for (var key in this.PlayerStatuses)

Silier:

StanUnsubmitted

Done

Oh I thought this was debug code XD

Stan: Oh I thought this was debug code XD

GrapjasAuthorUnsubmitted

Done

"this.PlayerStatuses.reduce is not a function"

Grapjas: "this.PlayerStatuses.reduce is not a function"

Lint: no-trailing-spaces

Trailing spaces not allowed.

Lint: no-trailing-spaces: Trailing spaces not allowed.

Lint: Trailing Whitespace

This line contains trailing whitespace. Consider setting up your editor to automatically remove trailing whitespace.

Lint: Trailing Whitespace: This line contains trailing whitespace. Consider setting up your editor to automatically remove…

SilierUnsubmitted

Done

this.playersBox.onMouseLeftDoubleClickItem = this.onMouseLeftDoubleClickItem.bind(this);

- for (var key in this.PlayerStatuses)

+ for (let key in this.PlayerStatuses)

this.playersBox.tooltip += "[font=\"sans-bold-14\"]" + "[color=\"" + this.PlayerStatuses[key].tags.color + "\"]" + g_PlayerStateSymbol + "[/color] " + key[0].toUpperCase() + key.substring(1) + "\n" + "[/font]";

Silier:

buddyButton.registerBuddyChangeHandler(this.onBuddyChange.bind(this)); buddyButton.registerBuddyChangeHandler(this.onBuddyChange.bind(this));

xmppMessages.registerPlayerListUpdateHandler(this.rebuildPlayerList.bind(this)); xmppMessages.registerPlayerListUpdateHandler(this.rebuildPlayerList.bind(this));

SilierUnsubmitted

Done

it would be better if you could use setStringtags function here

Silier: it would be better if you could use setStringtags function here

SilierUnsubmitted

Done

for (let key in this.PlayerStatuses)

- this.playersBox.tooltip += setStringTags("[font=\"sans-bold-14\"]" + "[color=\"" + this.PlayerStatuses[key].tags.color + "\"]" + this.PlayerStatuses[key].symbol + "[/color] " + this.PlayerStatuses[key].status + "\n" + "[/font]");

+ this.playersBox.tooltip += setStringTags(coloredText(this.PlayerStatuses[key].symbol, this.PlayerStatuses[key].tags.color) + this.PlayerStatuses[key].status, this.TooltipTags) + "\n";

buddyButton.registerBuddyChangeHandler(this.onBuddyChange.bind(this));

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.

SilierUnsubmitted

Not Done

not done

Silier: not done

GrapjasAuthorUnsubmitted

Done

I 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…

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();

} }

selectPlayer() selectPlayer()

{ {

▲ Show 20 Lines • Show All 55 Lines • ▼ Show 20 Lines 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': case 'buddy':

player.sortValue = (player.isBuddy ? 1 : 2) + 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 'rating': case 'rating':

player.sortValue = +player.rating; player.sortValue = +player.rating;

SilierUnsubmitted

Done

btw this is change is not correct
the + is supposed to be next to player.rating as it is not =+ operator but it is converting player.rating to number

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 = 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");

GrapjasAuthorUnsubmitted

Done

Just noticed this, isn't it a typo?

Grapjas: Just noticed this, isn't it a typo?

StanUnsubmitted

Done

It 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();

Show All 14 Lines 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 statusTags = this.PlayerStatuses[presence].tags; let statusTags = this.PlayerStatuses[presence].tags;

this.buddyStatusList[i] = player.isBuddy ? setStringTags(g_BuddySymbol, statusTags) : ""; this.buddyStatusList[i] = player.isBuddy ? setStringTags("★", statusTags) : "";

this.playerList[i] = PlayerColor.ColorPlayerName(player.name, "", player.role); this.playerList[i] = PlayerColor.ColorPlayerName(player.name, "", player.role);

this.presenceList[i] = setStringTags(this.PlayerStatuses[presence].status, statusTags); this.presenceList[i] = setStringTags(this.PlayerStatuses[presence].status, statusTags);

this.ratingList[i] = setStringTags(rating, statusTags); this.ratingList[i] = setStringTags(rating, statusTags);

this.nickList[i] = escapeText(player.name); this.nickList[i] = escapeText(player.name);

}); });

Engine.ProfileStop(); Engine.ProfileStop();

GrapjasAuthorUnsubmitted

Done

make 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.

SilierUnsubmitted

Done

hmm, why not just keep the original function to return the color and build the tag here, or make addition function for that.
Else if just using player color for this purpose, use coloredText and there is no need to change those two functions to return or accept the object and that would probably solve problems on another places as well.

Silier: hmm, why not just keep the original function to return the color and build the tag here, or…

Engine.ProfileStart("copyToGUI"); Engine.ProfileStart("copyToGUI");

this.playersBox.list_buddy = this.buddyStatusList; 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();

Show All 17 Lines class PlayerList

} }

/** /**

* 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("●"),

SilierUnsubmitted

Done

similar 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

"tags": { "tags": {

SilierUnsubmitted

Done

this is actually wrong
this does not refer to PlayerList class

Silier: this is actually wrong `this` does not refer to PlayerList class

"color": "0 219 0" "color": "0 219 0"

} }

}, },

"away": { "away": {

"status": translate("Away"), "status": translate("●"),

"tags": { "tags": {

"color": "255 127 0" "color": "255 127 0"

} }

}, },

"playing": { "playing": {

"status": translate("Busy"), "status": translate("●"),

StanUnsubmitted

Done

g_PlayerSymbol? Also, you probably don't want people to translate those...

Stan: g_PlayerSymbol? Also, you probably don't want people to translate those...

GrapjasAuthorUnsubmitted

Done

g_PlayerSymbol? Also, you probably don't want people to translate those...

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("●"),

vladislavbelovUnsubmitted

Done

New 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.

StanUnsubmitted

Done

I 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"),

SilierUnsubmitted

Done

forgotten

Silier: forgotten

"tags": { "tags": {

"color": "178 178 178" "color": "178 178 178"

} }

}; };