HomeWildfire Games

Fix BSD and OSX build that don't provide the AT&T version of timezone used by…
AuditedrP19947

Description

Fix BSD and OSX build that don't provide the AT&T version of timezone used by rP19801. Refs #3832, D514.

Mostly reviewed by: leper
Tested on Windows by: Stan

Details

Event Timeline

Actually OSX does provide that, see the second comment on rP19801. They just didn't update the documentation.

leper added inline comments.Aug 23 2017, 11:54 PM
/ps/trunk/source/lobby/XmppClient.cpp
948

Actually why does that use Locale::getUS() when we are parsing something in Zulu time, which is GMT?

Actually why does that use Locale::getUS() when we are parsing something in Zulu time, which is GMT?

Our L10n.cpp function expects a locale, so we have to provide it one or change it.
I aimed for a minimalistic patch that is as easy to review and apply as possible.

I passed the US locale because I copied it from GetBuildTimestamp of ScriptFunctions.cpp.
(US locale is correct in that place, see https://gcc.gnu.org/onlinedocs/cpp/Standard-Predefined-Macros.html )

Other locales can be found here, but the people of republic of zulu don't have a locale yet.
http://icu-project.org/apiref/icu4c/classicu_1_1Locale.html#pub-static-methods

There is a constructor that doesn't receive a locale argument, but uses the Default locale.
http://icu-project.org/apiref/icu4c/classicu_1_1SimpleDateFormat.html#a4c6fb4cda13384e3536360d7aff75428

The default locale can be set by the user, so that seems less desirable.
http://icu-project.org/apiref/icu4c/classicu_1_1Locale.html#a020c6966493a8f00572616b64b5527c3

If the XMPP specs wouldn't be language agnostic, they would rather use the english names, so US locale seems the most intuitive choice amongst all choices.

The locale doesn't affect the parsing, nor the conversion to the unix timestamp.

If the Xmpp specs change and pass the month number as a word (like __DATE__ does), then the month names would be most likely in english.


Proofs as far as possible:
The timestamp that goes from ejabberd to the XMPP specs to gloox to glooxwrapper to XmppClient.cpp to L10n.cpp to ICU.

Our L10n.cpp only uses the locale for this specific SimpleDateFormat constructor http://icu-project.org/apiref/icu4c/classicu_1_1SimpleDateFormat.html#aea33520a89bb548165f7f3a3552d3617

The locale is used to obtain the symbols used in formatting (e.g., the names of the months), but not to provide the pattern.

Because gloox expects the timestamp string to confirm with both XEP-0082 and XEP-0203,
the string specifies a date+time and the timezone.

So ICU has to load the datetime in that timezone (regardless of the passed locale).
This datetime is converted to an UDate again which is also in UTC by definition.

The xmpp server sends the timestamp of historic chat messages as a string to the xmpp client.
An example from our ejabberd server is 2017-08-23T14:27:57Z.

ejabberd uses mod_muc to send historic messages https://docs.ejabberd.im/admin/configuration/#mod-muc
https://github.com/processone/ejabberd/blob/master/src/mod_muc.erl
(Besides finding out that the 20 historic chat message default is coded there, I didn't become more educated from reading the code.)
From reading the code, I couldn't confirm whether ejabberd implements XEP-0203 which uses XEP-0082.
https://xmpp.org/extensions/xep-0203.html
https://xmpp.org/extensions/xep-0082.html

Our XmppClient receives the timestamp from gloox as msg.when()->stamp(), which is of the type glooxwrapper::DelayedDelivery.
That type is defined in gloox/delayeddelivery.h:
https://camaya.net/api/gloox/classgloox_1_1DelayedDelivery.html#a2aff496083efbe50ffd8a914b1af47be

@param stamp The datetime stamp of the original send.

In delayeddelivery.cpp, we can see that gloox indeed only passes on the string received from the xmpp server
https://camaya.net/download/gloox-1.0.20.tar.bz2

So our XmppClient that uses gloox expects that the timeformat conforms to both XEP-0203 and XEP-0082.
XEP-0203 says the timestamp

MUST be expressed in UTC.

The return type of ParseDateTime is a UDate, i.e. in UTC.


Remotely related, reading
https://stackoverflow.com/questions/39146561/ejabberd-delayed-delivery-timestamp
it sounds like we could use XEP-0012 to show when the user logged in the last time.


So better comments is all I can offer:

Index: source/lobby/XmppClient.cpp
===================================================================
--- source/lobby/XmppClient.cpp	(revision 20030)
+++ source/lobby/XmppClient.cpp	(working copy)
@@ -933,9 +933,10 @@
  *****************************************************/
 
 /**
- * Compute the POSIX timestamp of a message. Uses message datetime when possible, current time otherwise.
+ * Parse the timestamp of a historic chat message and use the current time for new chat messages.
+ * Historic chat messages implement DelayedDelivers as specified in XEP-0203.
+ * Hence, the timestamp MUST be in UTC and conform to the DateTime format XEP-0082.
  *
- * @param msg The message on which to base the computation.
  * @returns Seconds since the epoch.
  */
 std::time_t XmppClient::ComputeTimestamp(const glooxwrapper::Message& msg) const
@@ -944,7 +945,7 @@
 	if (!msg.when())
 		return std::time(nullptr);
 
-	// See XEP-0082 for the date format
+	// The locale is irrelevant, because the XMPP date format doesn't contain written month names
 	return g_L10n.ParseDateTime(msg.when()->stamp().to_string(), "Y-M-d'T'H:m:sZ", Locale::getUS()) / 1000.0;
 }

Those green checkboxes are expensive :S

This commit now requires audit.Aug 24 2017, 7:23 PM

Actually why does that use Locale::getUS() when we are parsing something in Zulu time, which is GMT?

Our L10n.cpp function expects a locale, so we have to provide it one or change it.
I aimed for a minimalistic patch that is as easy to review and apply as possible.

Ah, makes sense. I guess there is no point in having a default parameter since that would just hide possible issues.

Other locales can be found here, but the people of republic of zulu don't have a locale yet.

Zulu time, as indicated by the trailing Z, is not related to the Zulu, but is a shorthand for UTC/GMT (see https://en.wikipedia.org/wiki/Zulu_Time#Time_zones).

[very detailed research]

Interesting, though I would have been fine with the first part of the quote too ;-)

Remotely related, reading
https://stackoverflow.com/questions/39146561/ejabberd-delayed-delivery-timestamp
it sounds like we could use XEP-0012 to show when the user logged in the last time.

But do we really want to show that? If you do you might also want to stop the pointless copying of rating data on new releases, given that quite a few entries might just show how pointless that is (not logged in for years).

So better comments is all I can offer:

Looks good.

Those green checkboxes are expensive :S

Well, that was mostly to satisfy my curiosity.

Other locales can be found here, but the people of republic of zulu don't have a locale yet.

Tried to be funny. but I didn't know about the https://en.wikipedia.org/wiki/Zulu_Kingdom Just shows that no research is complete :P

pointless copying of rating data on new releases, accounts not logged in for years

Those people are scrolled out of view. Many of those that are in the top 100 often come back after some months or even a year. At least currently it doesn't look like hard competition to inactive players. If someone wants to get a top 20 ranking, it's not too hard to do so with some dedication (saying that with my 1150 rating account).
Some accounts could be fleshed out if they are particularly annoying.
Perhaps there could also be a room rating and an all-time-rating db in a world with many active lobby developers.

do we want XEP-0012?

Players could estimate the likelihood of finding others.
gloox supports it, but I read that ejabberd didn't implement it (last activity only for offline chat messages)

Looks good.

Thanks for the feedback.

Perhaps there could also be a room rating and an all-time-rating db in a world with many active lobby developers.

At least for some time there was no all-time-rating since that was considered pointless for a game where a new version changes the balancing completely. And what point is there in keeping people that haven't played for years in the list. But I guess that's how the lobby is run.

do we want XEP-0012?

Players could estimate the likelihood of finding others.
gloox supports it, but I read that ejabberd didn't implement it (last activity only for offline chat messages)

ejabberd does store last login, check the web interface.

leper accepted this commit.Sep 20 2017, 12:25 AM

Given that downstream now ships with that patch and the question was answered.

All concerns with this commit have now been addressed.Sep 20 2017, 12:25 AM

Given that downstream now ships with that patch and the question was answered.

Thanks!