Page MenuHomeWildfire Games
Differential D284 Diff 1081 binaries/data/mods/public/gui/lobby/lobby.js

Changeset View

Standalone View

View Options

binaries/data/mods/public/gui/lobby/lobby.js

Context not available.
let game = g_GameList[i]; let game = g_GameList[i];
let gameName = escapeText(game.name); let gameName = escapeText(game.name);
let mapTypeIdx = g_MapTypes.Name.indexOf(game.mapType); let mapTypeIdx = g_MapTypes.Name.indexOf(game.mapType);
let observerNumbers = stringifiedTeamListToPlayerData(game.players).filter(player => { return player.Team == "observer"}).length;
KrinkleUnsubmitted
Done
Inline Actions

The filter closure can be further simplified given it's only a single expression:

.filter(player => { return player.Team == "observer"});
.filter(player => player.Team === "observer");
Krinkle: The filter closure can be further simplified given it's only a single expression: ```lang=js .
KrinkleUnsubmitted
Done
Inline Actions

Higher up in the code, the gamer.players string is also parsed. Rather than parsing it again, might be worth re-using that result here.

Krinkle: Higher up in the code, the `gamer.players` string is also parsed. Rather than parsing it again…
if (game.ip == g_SelectedGameIP && game.port == g_SelectedGamePort) if (game.ip == g_SelectedGameIP && game.port == g_SelectedGamePort)
selectedGameIndex = +i; selectedGameIndex = +i;
Context not available.
list_mapName.push(translateMapTitle(game.niceMapName)); list_mapName.push(translateMapTitle(game.niceMapName));
list_mapSize.push(translateMapSize(game.mapSize)); list_mapSize.push(translateMapSize(game.mapSize));
list_mapType.push(g_MapTypes.Title[mapTypeIdx] || ""); list_mapType.push(g_MapTypes.Title[mapTypeIdx] || "");
list_nPlayers.push(game.nbp + "/" + game.maxnbp); list_nPlayers.push(game.nbp + '/' + game.maxnbp + (observerNumbers > 0 ? '[color="0 128 128 128"]+' + observerNumbers + '[/color]' : '' ));
list.push(gameName); list.push(gameName);
list_data.push(i); list_data.push(i);
} }
Context not available.
vladislavbelovUnsubmitted
Not Done
Inline Actions

Why different quotes?

vladislavbelov: Why different quotes?
ffffffffUnsubmitted
Not Done
Inline Actions

no mean change.

ffffffff: no mean change.
vladislavbelovUnsubmitted
Not Done
Inline Actions

And empty line?

vladislavbelov: And empty line?
ffffffffUnsubmitted
Not Done
Inline Actions

no mean change.

ffffffff: no mean change.
KrinkleUnsubmitted
Not Done
Inline Actions

(FYI: I change this number locally to e.g. 1, 2 or 14, to see what it would look like with that number of observers).

Krinkle: //(FYI: I change this number locally to e.g. 1, 2 or 14, to see what it would look like with…
StanUnsubmitted
Done
Inline Actions

Might be interesting to use setStringTags for this. See D2151.

Stan: Might be interesting to use setStringTags for this. See D2151.
ffffffffUnsubmitted
Done
Inline Actions

indeed

ffffffff: indeed
elexisAuthorUnsubmitted
Done
Inline Actions

Translators won't know what this is about, so it should have a translation comment:
// Translation: Number of observers in the gamelist
or similar

Don't split the translate and sprintf, so that people see the string arguments right near the string.
If we want to split that from the code logic to allow modders to change it more easily, then splitting both sprintf and translate would keep the string and its arguments grouped.

elexis: Translators won't know what this is about, so it should have a translation comment: //…
elexisAuthorUnsubmitted
Done
Inline Actions

At least it would be consistent to make "gray" a global. And the more recent version of that would be to use setStringTags, so that people can also specify a font alongside if they want.

That += " " + is preventing translators to change the order of the arguments.

Same for playerText and the "/" operator.
Some languages might want to use a different symbol than slash, and thus also a different order.

So

// Translation: meaningful information
sprintf(translate("%(foo)s / %(bar)s")`...

(
On that > 0 check, I wonder if one could do some magic using plural strings.

translatePlural(singularMessage, pluralMessage, number)

But that won't work, since the english strings can only be specified for singular n=1 and plural n>1, but we need 0 and > 0 checks here.
(The translators can propose translations for arbitrary N intervals afaik)
)

elexis: At least it would be consistent to make "gray" a global. And the more recent version of that…
KrinkleUnsubmitted
Done
Inline Actions

I don't know if the order needs to be localisable, and using plural for this seems like a workaround. Do you want me to split it into two separate messages?

e.g. one for x/y and one for x/y +z?

Krinkle: I don't know if the order needs to be localisable, and using plural for this seems like a…
elexisAuthorUnsubmitted
Done
Inline Actions

Yes, the order should be localizable, everything that is displayed should be.
Whenever there is a string displayed in the GUI, some language may require to change something about it, so provide translators the opportunity. (right to left languages for instance, or different slash symbol, or addition of something before or after the string)
Mostly it should be the translators decision whether they want to localize it.
There are plenty examples of such things in the codebase, for example tooltips.js, for example %(healthLabel)s %(current)s / %(max)s.

Plural strings allow the translator to use a different format depending on the count.
Thats not working around anything but allowing them to for example show +1 observer and +2 observers for n>2 (and languages can specify custom ranges).
When we have a count given, its not unlikely that one may have different strings for different counts.
Some examples

translatePlural("meter", "meters", maxRange)
sprintf(translatePlural("%(label)s %(val)s %(unit)s", "%(label)s %(val)s %(unit)s", hp), {
"unit": unitFont(translatePlural("HP", "HP", hp))

e.g. one for x/y and one for x/y +z?

Yes. It's not our task to judge what makes sense in every language, but to provide translators the choice to decide it for them.
If we don't add the translation possibility, we'd be making a claim that no language exists where this should be changeable and that usually means one would have to know every language. -> Erroring on the side of providing freedom.

elexis: Yes, the order should be localizable, everything that is displayed should be. Whenever there is…
elexisAuthorUnsubmitted
Done
Inline Actions

(> 0 unneeded I guesS)

elexis: (`> 0` unneeded I guesS)
KrinkleUnsubmitted
Done
Inline Actions

Yeah, wasn't sure which style is preferred.

Typecasting is sometimes frowned upon, but for properties where only one type is allowed, I think it's fair to expect the reader to know those rules for the shorter form to be an easily recognised idiom.

Krinkle: Yeah, wasn't sure which style is preferred. Typecasting is sometimes frowned upon, but for…
elexisAuthorUnsubmitted
Done
Inline Actions

+%(observercount)s or +%(observers)s?

elexis: `+%(observercount)s` or `+%(observers)s`?
elexisAuthorUnsubmitted
Done
Inline Actions

non-bot players -> lobby players? Also seen "human player" somewhere. Otherwise bot -> AI.

(Its also not obvious whether thats a bug, whether it should reflect human players or ingame players including AI. For example 2 human vs 6 AI player looks like 2/8, as in 6 missing, but theyre not missing. Can be changed, but not sure if it should, also OT)

elexis: non-bot players -> lobby players? Also seen "human player" somewhere. Otherwise bot -> AI.
elexisAuthorUnsubmitted
Not Done
Inline Actions

The duplication can be avoided by using game.observerCount ? translate("%(current)s/%(max)s +%(observercount)s") : translate("%(current)s/%(max)s")

(optionally with ?\n :\n and some tabs)

elexis: The duplication can be avoided by using `game.observerCount ? translate("%(current)s/%(max)s +%…
KrinkleUnsubmitted
Not Done
Inline Actions

The duplication of what exactly? Do you mean we should pass all three variables to both?

I avoided that as I wasn't sure whether that'd be allowed (might trip a warning for unused variable, depending on how strict we want things to be). The following could work though if that's preferred.

let plainText = game.observerCount ?
  // Translation: The number of human players and observers in this game
  translate("%(current)s/%(max)s +%(observercount)s") :
  // Translation: The number of human players in this game
  translate("%(current)s/%(max)s");

let playerText = setStringTags(sprintf(plainText, {
  "current": game.nbp,
  "max": game.maxnbp,
  "observercount": game.observerCount
}), g_ObserverCountTags);
Krinkle: The duplication of what exactly? Do you mean we should pass all three variables to both? I…
elexisAuthorUnsubmitted
Not Done
Inline Actions

(Don't blame me, it was my computer science teacher who recommended to change if (x != y) to if (x==y) in if-else patterns to reduce the complexity by one operation)

elexis: (Don't blame me, it was my computer science teacher who recommended to change `if (x != y)` to…
elexisAuthorUnsubmitted
Not Done
Inline Actions

The gray g_ObserverCountTags is applied to the entire text, shouldn't the playercount have a different color?

White and gray, or whatever.

elexis: The gray `g_ObserverCountTags` is applied to the entire text, shouldn't the playercount have a…
elexisAuthorUnsubmitted
Done
Inline Actions

One could say that the unification of the two sprintf's makes the close slightly slower, but then again one could care less if there are only < 30 games at a time and it costs probably something of the order of magnitude of 1 microseconds.

elexis: One could say that the unification of the two sprintf's makes the close slightly slower, but…
Wildfire Games · Phabricator