Paths

Table of Contentst

Differential D514

Store lobby timestamps in C++
ClosedPublic
Actions

Authored by elexis on May 17 2017, 4:56 PM.

Details

Reviewers

Itms

Commits

rP19801: Correct lobby chat timestamps.

Summary

Lobby timestamps are currently only stored when parsing them in JS, whereas they are cached in C++ with a correct timestamp they are pulled from JS.
So if someone returns from a lobbied game, those historical messages show the date when the user returned to the lobby, not when the messages were sent.
As a side-effect, this breaks the clientside lobby chat message spamfilter, which could be removed (as proposed by D512).

Josh had fixed this in a patch that was committed in rP17928, but it had two issues:

Forgot to include wposix.
Used std::time_t while the type of that is not specified, u64 on some platforms which can't be converted to a JS number.

Both issues have a trivial fix.
We can store the timestamp as u64 (the type a timestamp ought to have) but have to use double when converting it to a JS number,
since JS has only one number type which is equivalent to number (see D84, D205).
The same double conversion is used for savegames and replays, so it's not only correct but also consistent with the rest of the code.

The lobby.js part needs to be amended if D512 is rejected.

Test Plan

Join the lobby with a chat client like pidgin or psi-plus and see the correct timestamps of the historical messages.
Join with the patch compiled to see that those are still in the right timezone (as #3350 was fixed meanwhile).

Start a lobbied game, post a message with the chat client, wait a minute, return to the lobby and see that the timestamp is from the
date when the message was sent, not wen the client returned to the lobby window.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

elexis created this revision.May 17 2017, 4:56 PM

Add missing, recent addChatMessage calls and use current time as a default for chatmessages that are created in JS.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1239/ for more details.

Harbormaster completed remote builds in B1777: Diff 1998.May 17 2017, 5:43 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1240/ for more details.

Harbormaster completed remote builds in B1778: Diff 1999.May 17 2017, 6:30 PM

This looks fine and works on Windows. Here are a few nitpicks ? only the cast actually worries me and would warrant a comment if it is really needed.

source/lobby/XmppClient.cpp
25 ↗	(On Diff #1999)	We don't indent preprocessor
570 ↗	(On Diff #1999)	Why the cast?
943 ↗	(On Diff #1999)	`if (!strptime(timestampStr.c_str(), "%Y-%m-%dT%H:%M:%SZ", &timestamp))` maybe?

This revision now requires changes to proceed.May 18 2017, 11:56 PM

elexis added inline comments.May 18 2017, 11:59 PM

source/lobby/XmppClient.cpp
570 ↗	(On Diff #1999)	D205

leper added a subscriber: leper.May 19 2017, 2:09 AM

leper added inline comments.

source/lobby/XmppClient.cpp
25 ↗	(On Diff #1999)	Or at least not like that. #if ... # if .... ... # endif #else ... if there is a lot of nesting or long blocks going on, but for this that seems quite pointless.

Itms added inline comments.May 19 2017, 9:04 AM

source/lobby/XmppClient.cpp
570 ↗	(On Diff #1999)	Got an idea overnight, we should overload `ToJSVal` when the value is a u64/i64 (if we don't already), and cast to double there. We can even issue a warning there if the cast loses data (not an assertion, because crashing the game for a timestamp or a file size is a bit too much ?). It is also a perfect place to write a comment about "why the cast". And we get rid of the few non-self-documenting casts scattered in the code.

elexis added inline comments.May 19 2017, 9:59 AM

source/lobby/XmppClient.cpp
570 ↗	(On Diff #1999)	People might start using u64 (esp crucially to avoid in the simulation) and assume that might even arrive at JS. For the same reason I agreed with echotangoecho to remove the safe JS number to u64 conversion (assuming it wasn't a decimal). JS::NumberValue can't be used as it can't be initialized from an `u64`, so we'd have to convert before converting before converting. We could also add a `typedef double JSNumber` if that helps documenting.

elexis added inline comments.May 22 2017, 5:03 AM

source/lobby/XmppClient.cpp
25 ↗	(On Diff #1999)	Ack
570 ↗	(On Diff #1999)	And in case we go with that, we should also use it for D190.
931 ↗	(On Diff #1999)	@rturns seconds since the epoch
933 ↗	(On Diff #1999)	This should still be `std::time_t` to avoid the pointless conversion `std::time_t` -> `u64` -> `double` (which is also bad practice becasue we now have to ensure that this unneeded conversion is not lossy).
943 ↗	(On Diff #1999)	should work
source/lobby/XmppClient.h
128 ↗	(On Diff #1999)	eh const

I swar I had updated the diff alreay, phabricator even has shown me the update comment I typed last time, hmm.
D512 was committed, so no more need to amend this patch.

Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  36|  36|  * The playerlist will be assembled using these values.
|  37|  37|  */
|  38|  38| const g_PlayerStatuses = {
|  39|    |-	"available": { "color": "0 219 0",     "status": translate("Online") },
|    |  39|+	"available": { "color": "0 219 0", "status": translate("Online") },
|  40|  40| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  41|  41| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  42|  42| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  37|  37|  */
|  38|  38| const g_PlayerStatuses = {
|  39|  39| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  40|    |-	"away":      { "color": "229 76 13",   "status": translate("Away") },
|    |  40|+	"away":      { "color": "229 76 13", "status": translate("Away") },
|  41|  41| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  42|  42| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  43|  43| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  38|  38| const g_PlayerStatuses = {
|  39|  39| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  40|  40| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  41|    |-	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|    |  41|+	"playing":   { "color": "200 0 0", "status": translate("Busy") },
|  42|  42| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  43|  43| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  44|  44| };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  39|  39| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  40|  40| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  41|  41| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  42|    |-	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    |  42|+	"offline":   { "color": "0 0 0", "status": translate("Offline") },
|  43|  43| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  44|  44| };
|  45|  45| 

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

binaries/data/mods/public/gui/lobby/lobby.js
|1086| »   while·(true)
|    | [NORMAL] ESLintBear (no-constant-condition):
|    | Unexpected constant condition.

binaries/data/mods/public/gui/lobby/lobby.js
|1225| »   »   switch·(command)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/lobby.js
| 450| »   if·(mapSizeFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 454| »   if·(playersNumberFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 458| »   if·(mapTypeFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 579| »   »   case·'name':
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'default'.

binaries/data/mods/public/gui/lobby/lobby.js
|1106| »   »   if·(msg.type·==·"chat"·&&·Engine.LobbyGetMucMessageCount()·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/55/ for more details.

Harbormaster failed remote builds in B1977: Diff 2217!May 26 2017, 2:24 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1369/ for more details.

ping

Itms added inline comments.May 28 2017, 10:48 PM

source/lobby/XmppClient.cpp
569 ↗	(On Diff #2217)	Why don't you check for `message.time`?

elexis added inline comments.May 29 2017, 3:57 AM

source/lobby/XmppClient.cpp
569 ↗	(On Diff #2217)	It could only occur if someone is constructing a `GUIMessage` without `CreateGUIMessage` and violating the fact that the coder should store the timestamps in C++ (instead of the time when returning from a game in JS). The checks for non-empty-strings of `type` and `level` could be removed as well as they should be considered mandatory. (See the 2 levels being represented as object keys in `g_NetMessageTypes` of `lobby.js`)) The only check we could add here is: `scriptInterface.SetProperty(ret, "time", (double)(message.time ? message.time : std::time(nullptr)));` and then remove L742. That will prevent someone from being able to pass 0 as the time. This check can't prevent any errors since the variable is already initialized when constructing the struct. The checks basically come from the fact that this function is used as an abbreviation. #4482 proposes to remove this function altogether to use more descriptive variable names, remove the need for checks and not be bound to at most 4 string properties anymore`.

Thanks for the testing and perusal Itms!

source/lobby/XmppClient.cpp
936 ↗	(On Diff #2217)	Adding a comment stating that only historic messages contain a timestamp
source/lobby/XmppClient.h
140 ↗	(On Diff #2217)	`std::time_t`