Page MenuHomeWildfire Games

Fix timestamps in lobby for delayed messages with microseconds
ClosedPublic

Authored by Dunedan on Jan 5 2018, 6:51 PM.

Details

Summary

The format of timestamps for delayed XMPP messages differs slightly between ejabberd versions. While old versions don't include microseconds, newer ones do. This patch ensures 0ad shows the correct timestamp no matter if it contains microseconds or not.

Test Plan

Apply the patch and connect to a lobby run by an old ejabberd (like the official lobby) and one run by a recent ejabberd. Notice that both show correct timestamps for messages in the backlog.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Dunedan created this revision.Jan 5 2018, 6:51 PM
elexis added a subscriber: elexis.Jan 5 2018, 8:01 PM

XEP numbers and links should be documented in the code (see ticket, see documentation of the function).
As mentioned in the ticket, it might be a gloox bug, not our bug.
Looks like the duplication can't be removed with a ternary.
But we have a GLOOXVERSION macro which would make that a bit cleaner.
Or maybe some pattern matching is supported ? (IIRC that format ends up in ICU)

In D1197#48513, @elexis wrote:

XEP numbers and links should be documented in the code (see ticket, see documentation of the function).

Well, they are. It's just that XEP-0082 allows the date time to contain microseconds as well.

As mentioned in the ticket, it might be a gloox bug, not our bug.

It's our, see ticket for more details.

Looks like the duplication can't be removed with a ternary.
But we have a GLOOXVERSION macro which would make that a bit cleaner.

Doesn't matter which gloox version it is. It's just related to the ejabberd version.

Or maybe some pattern matching is supported ? (IIRC that format ends up in ICU)

Well, I don't know. I just chose the easiest path.

elexis added a comment.Jan 5 2018, 8:34 PM

I see. Can you try to take a look to unify that, preferably by just updating the pattern? You will also find some related differential revisions using blame.

In D1197#48522, @elexis wrote:

Can you try to take a look to unify that, preferably by just updating the pattern?

Internally L10n::ParseDateTime uses [[ https://ssl.icu-project.org/apiref/icu4c/classSimpleDateFormat.html | SimpleDateTime from ICU ]]. As far as I understand SimpleDateFormat doesn't offer the ability to match multiple different datetime formats with a single expression. The internet seems to agree with that (e.g. see https://stackoverflow.com/questions/36337511/parse-an-iso-8601-date-time-with-optional-fractional-second-using-icu and https://stackoverflow.com/questions/26895428/how-do-i-parse-an-iso-8601-date-with-optional-milliseconds-to-a-struct-tm-in-c). So I still think the current implementation is the most straight-forward one, but I'm open to any alternative.

elexis added a comment.Jan 5 2018, 9:29 PM

Too bad. I can confirm reading the links and https://ssl.icu-project.org/apiref/icu4c/classicu_1_1SimpleDateFormat.html
Well, then as ICU doesn't support that, it could use a macro (for instance)

#define doSomething(format)\
....

doSomething(format1);
doSomething(format2);
#undef doSomething

or a range-based loop, i.e.

for (someStringType format : {"format1", "format2"} )

I guess the loop would look cleaner.

Dunedan updated this revision to Diff 5101.Jan 6 2018, 8:40 AM

As suggested changed the if to a for loop.

Dunedan updated this revision to Diff 5102.Jan 6 2018, 8:41 AM

Fix diff

Dunedan updated this revision to Diff 5117.Jan 6 2018, 5:00 PM

Improved docstrings

elexis accepted this revision.Jan 6 2018, 5:05 PM

Thanks for the patch Dunedan! Makes it a bit less effort to upgrade :-)

Assuming you tested with your more recent ejabberd version, I only tested against ours.

source/lobby/XmppClient.cpp
940 ↗(On Diff #5102)

Don't need a copy, a reference is sufficient, so &
Adding const and the type std::vector<std::string> for good measure.

+3 spaces

941 ↗(On Diff #5102)

tabs instead of spaces

This revision is now accepted and ready to land.Jan 6 2018, 5:05 PM
This revision was automatically updated to reflect the committed changes.