Page MenuHomeWildfire Games

lobby: Display observer count in gameslist
Needs ReviewPublic

Authored by Krinkle on Apr 2 2017, 1:46 AM.

Details

Summary

Show the observer count in addition to the player count, to show some activity in a game.

Test Plan

Agree with the change.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
arcpatch
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8641
Build 14158: Vulcan BuildJenkins
Build 14157: arc lint + arc unit

Event Timeline

ffffffff created this revision.EditedApr 2 2017, 1:46 AM

Gameslist (only look Players (number) column)

vladislavbelov requested changes to this revision.Apr 2 2017, 5:44 PM
vladislavbelov added a subscriber: vladislavbelov.
vladislavbelov added inline comments.
binaries/data/mods/public/gui/lobby/lobby.js
638

Also if you/elexis want to minize, you could write something like this:

game.observerNumbers = stringifiedTeamListToPlayerData(game.players)).filter(() => { return player.Team == "observer" }).length;
644

I think counting of observers shouldn't be here? Because this is a sorting or filter.
So split on few lines at least.

695

Why different quotes?

696

And empty line?

This revision now requires changes to proceed.Apr 2 2017, 5:44 PM
ffffffff updated this revision to Diff 1080.Apr 2 2017, 6:16 PM
ffffffff edited edge metadata.

minor changes by vladis

ffffffff updated this revision to Diff 1081.Apr 2 2017, 6:33 PM

changes from vladis

ur right valdis way better

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

this is minimize getting gamelist sorting filtering altering? <i can split>
ive seen this before doesnt differ

695

no mean change.

696

no mean change.

Looks better, but if look at it from player, he may think that "+" symbol means added number of players, not observers, so I suggest, to make string like "current/max (observers)", and then we should add title, like "Players (Observers), but I'm not sure how it looks like for others.

Krinkle commandeered this revision.Jul 21 2019, 12:48 AM
Krinkle added a reviewer: ffffffff.
This revision now requires changes to proceed.Jul 21 2019, 12:48 AM
Krinkle added inline comments.Jul 21 2019, 1:13 AM
binaries/data/mods/public/gui/lobby/lobby.js
678

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

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 updated this revision to Diff 9024.Jul 21 2019, 1:21 AM
Krinkle retitled this revision from Observer number in lobby's gameslist to lobby: Display observer count in gameslist.
Krinkle edited the summary of this revision. (Show Details)
Krinkle edited the test plan for this revision. (Show Details)
  • Rebased on latest trunk.
  • Re-used earlier result of parsing the game.players string.
  • Implemented the suggestion of displaying (N) instead of +N.
  • Increase colour and brightness for consistency with other text.

Current revision looks like so:

Owners added a subscriber: Restricted Owners Package.Jul 21 2019, 1:21 AM
Krinkle updated this revision to Diff 9025.Jul 21 2019, 1:21 AM
Krinkle marked 3 inline comments as done.

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

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

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

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

nani added a subscriber: nani.Jul 21 2019, 2:39 AM

Looks better, but if look at it from player, he may think that "+" symbol means added number of players, not observers, so I suggest, to make string like "current/max (observers)", and then we should add title, like "Players (Observers), but I'm not sure how it looks like for others.

Strongly disagree. + Has a much better implicit meaning. From the point of view of a regular player number(number) has much less meaning that number + number.

On the format "x/y (z)" vs "x/y+z", it seems in the former might be considered ambiguous, the number in the parentheses seems to indicate a relation to that number, like x or y indicating a subset of z, whereas the latter format clearly indicates the distinctiveness of the two counted groups.
+ however is a mathmatical interpretation, so if you use the slash / it sounds like division, making this appear to be some wrong equation.

What I see on this screenshot looks ok to me though, just a bit too dark:

Gameslist (only look Players (number) column)

Linting & l10n:
From http://irclogs.wildfiregames.com/2019-07/2019-07-21-QuakeNet-%230ad-dev.log

11:02 < elexis> Krinkle: we use ++x unless the return value matters
11:03 < elexis> and for "color="190 160 70" there is a new helper function setGuiTags(text, { "color": rgb })
11:03 < elexis> with the color being moved to the top, so it can be configured
11:04 < elexis> x + "/" + y sounds like it should use sprintf(translate(..
11:04 < elexis> since some language might use a different symbol, may want to prepend or append to that string or use a different order
11:06 < elexis> and I suppose the more catchy color should be used for players than observers

Should test with >= 10 observers and 1024*768.

Krinkle added inline comments.Wed, Jul 24, 2:30 AM
binaries/data/mods/public/gui/lobby/lobby.js
999

(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 updated this revision to Diff 9157.Sun, Jul 28, 4:27 PM
Krinkle marked an inline comment as not done.
  • Use the coloredText() function instead of an inline [color] construct (the utility was added in Dec 2017, a few months after the patch started).
  • Switched from postfix to prefix increment.
  • Switch away from (N) format, back to +N, and allow the observer count formatting to be translated.
  • Change observer count colour from yellow to grey, so that the player count remains more prominent.

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  47|  47|  * The playerlist will be assembled using these values.
|  48|  48|  */
|  49|  49| var g_PlayerStatuses = {
|  50|    |-	"available": { "color": "0 219 0",     "status": translate("Online") },
|    |  50|+	"available": { "color": "0 219 0", "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'away'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  48|  48|  */
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|    |-	"away":      { "color": "229 76 13",   "status": translate("Away") },
|    |  51|+	"away": { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  48|  48|  */
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|    |-	"away":      { "color": "229 76 13",   "status": translate("Away") },
|    |  51|+	"away":      { "color": "229 76 13", "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'playing'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|    |-	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|    |  52|+	"playing": { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|    |-	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|    |  52|+	"playing":   { "color": "200 0 0", "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'offline'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|    |-	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    |  53|+	"offline": { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|  56|  56| 
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|    |-	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    |  53|+	"offline":   { "color": "0 0 0", "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|  56|  56| 
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'unknown'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|    |-	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    |  54|+	"unknown": { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|  56|  56| 
|  57|  57| var g_RoleNames = {
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 219| 219| 					me ?
| 220| 220| 						translate("You have been muted.") :
| 221| 221| 						translate("%(nick)s has been muted.") :
| 222|    |-				newrole == "moderator" ?
|    | 222|+					newrole == "moderator" ?
| 223| 223| 					me ?
| 224| 224| 						translate("You are now a moderator.") :
| 225| 225| 						translate("%(nick)s is now a moderator.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 220| 220| 						translate("You have been muted.") :
| 221| 221| 						translate("%(nick)s has been muted.") :
| 222| 222| 				newrole == "moderator" ?
| 223|    |-					me ?
|    | 223|+						me ?
| 224| 224| 						translate("You are now a moderator.") :
| 225| 225| 						translate("%(nick)s is now a moderator.") :
| 226| 226| 				msg.oldrole == "visitor" ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 221| 221| 						translate("%(nick)s has been muted.") :
| 222| 222| 				newrole == "moderator" ?
| 223| 223| 					me ?
| 224|    |-						translate("You are now a moderator.") :
|    | 224|+							translate("You are now a moderator.") :
| 225| 225| 						translate("%(nick)s is now a moderator.") :
| 226| 226| 				msg.oldrole == "visitor" ?
| 227| 227| 					me ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 222| 222| 				newrole == "moderator" ?
| 223| 223| 					me ?
| 224| 224| 						translate("You are now a moderator.") :
| 225|    |-						translate("%(nick)s is now a moderator.") :
|    | 225|+							translate("%(nick)s is now a moderator.") :
| 226| 226| 				msg.oldrole == "visitor" ?
| 227| 227| 					me ?
| 228| 228| 						translate("You have been unmuted.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 223| 223| 					me ?
| 224| 224| 						translate("You are now a moderator.") :
| 225| 225| 						translate("%(nick)s is now a moderator.") :
| 226|    |-				msg.oldrole == "visitor" ?
|    | 226|+						msg.oldrole == "visitor" ?
| 227| 227| 					me ?
| 228| 228| 						translate("You have been unmuted.") :
| 229| 229| 						translate("%(nick)s has been unmuted.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 224| 224| 						translate("You are now a moderator.") :
| 225| 225| 						translate("%(nick)s is now a moderator.") :
| 226| 226| 				msg.oldrole == "visitor" ?
| 227|    |-					me ?
|    | 227|+							me ?
| 228| 228| 						translate("You have been unmuted.") :
| 229| 229| 						translate("%(nick)s has been unmuted.") :
| 230| 230| 					me ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 225| 225| 						translate("%(nick)s is now a moderator.") :
| 226| 226| 				msg.oldrole == "visitor" ?
| 227| 227| 					me ?
| 228|    |-						translate("You have been unmuted.") :
|    | 228|+								translate("You have been unmuted.") :
| 229| 229| 						translate("%(nick)s has been unmuted.") :
| 230| 230| 					me ?
| 231| 231| 						translate("You are not a moderator anymore.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 226| 226| 				msg.oldrole == "visitor" ?
| 227| 227| 					me ?
| 228| 228| 						translate("You have been unmuted.") :
| 229|    |-						translate("%(nick)s has been unmuted.") :
|    | 229|+								translate("%(nick)s has been unmuted.") :
| 230| 230| 					me ?
| 231| 231| 						translate("You are not a moderator anymore.") :
| 232| 232| 						translate("%(nick)s is not a moderator anymore.");
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 227| 227| 					me ?
| 228| 228| 						translate("You have been unmuted.") :
| 229| 229| 						translate("%(nick)s has been unmuted.") :
| 230|    |-					me ?
|    | 230|+							me ?
| 231| 231| 						translate("You are not a moderator anymore.") :
| 232| 232| 						translate("%(nick)s is not a moderator anymore.");
| 233| 233| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 228| 228| 						translate("You have been unmuted.") :
| 229| 229| 						translate("%(nick)s has been unmuted.") :
| 230| 230| 					me ?
| 231|    |-						translate("You are not a moderator anymore.") :
|    | 231|+								translate("You are not a moderator anymore.") :
| 232| 232| 						translate("%(nick)s is not a moderator anymore.");
| 233| 233| 
| 234| 234| 			addChatMessage({
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/lobby/lobby.js
| 229| 229| 						translate("%(nick)s has been unmuted.") :
| 230| 230| 					me ?
| 231| 231| 						translate("You are not a moderator anymore.") :
| 232|    |-						translate("%(nick)s is not a moderator anymore.");
|    | 232|+								translate("%(nick)s is not a moderator anymore.");
| 233| 233| 
| 234| 234| 			addChatMessage({
| 235| 235| 				"text": "/special " + sprintf(txt, { "nick": msg.nick }),

binaries/data/mods/public/gui/lobby/lobby.js
|1038| »   »   switch·(sortBy)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

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

ffffffff accepted this revision.Mon, Aug 12, 4:30 PM

i like that, apply?

Stan added a subscriber: Stan.Tue, Aug 13, 11:34 AM
Stan added inline comments.
binaries/data/mods/public/gui/lobby/lobby.js
1091

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

ffffffff added inline comments.Tue, Aug 13, 5:23 PM
binaries/data/mods/public/gui/lobby/lobby.js
1091

indeed

What I see on the screenshot looks acceptable.

However it wouldn't hurt if there was a tooltip stating that the N'th number means X(N).

The GUI Object type "list" that "olist" and "dropdown" inherit provides a onHoverChange event, see the code in gamesetup.js for example

		dropdown.onHoverChange = function() {
			this.tooltip = data.tooltip(this.hovered, playerIdx);
		};

So perhaps one can make it so that it shows "Players: 5, Observers: 8" when hovering a game, so that there is no more doubt what the numbers mean.

Perhaps something like:

gameList.onHoverChange = function() {
this.tooltip = allGames[this.hovered].tooltip;
};

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

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.

1093

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