Page MenuHomeWildfire Games

Rename timeToString to durationToString and move to l10n
Needs RevisionPublic

Authored by temple on Dec 6 2017, 4:25 AM.

Details

Reviewers
s0600204
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Summary

Rename timeToString to durationToString and move to l10n.
Also remove extra leading 0's.

Test Plan

Agree it's a better name and location.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3974
Build 6963: Vulcan BuildJenkins
Build 6962: arc lint + arc unit

Event Timeline

temple created this revision.Dec 6 2017, 4:25 AM
Owners added a subscriber: Restricted Owners Package.Dec 6 2017, 4:25 AM
temple edited the summary of this revision. (Show Details)Dec 6 2017, 4:26 AM
temple edited the summary of this revision. (Show Details)
Vulcan added a subscriber: Vulcan.Dec 6 2017, 4:27 AM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
temple updated this revision to Diff 4595.Dec 6 2017, 4:51 AM

Fixed a comment.

Vulcan added a comment.Dec 6 2017, 4:52 AM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
elexis added a subscriber: elexis.Dec 7 2017, 3:37 PM
elexis added inline comments.
binaries/data/mods/public/gui/common/l10n.js
11

Since we already have a multi line translation comment in lobby.js we at least woudn't be the first one to add an incomplete one, yay. To be sure one could look in extractors.py or look into the wiki pages (Internationalization_and_Localization and iirc more).

From a quick look messages.json register the Translation keyword under commentTags and that is parsed by extractJavascriptFromFile in extractors.py. We see multilinecomment there. The only other reference to that keyword is in jslexer.py:

('multilinecomment', re.compile(r'/\*.*?\*/(?us)')),

So perhaps it is wrong in lobby.js after all.

Experimental evidence comes in from the *po files that this script produces (and that are getting uploaded to transifex automatically).

-> it's broken

#. Translation: Time prefix as shown in the multiplayer lobby (when you enable it in the options page).
#: gui/lobby/lobby.js:1409
#, javascript-format
msgid "\\[%(time)s]"
msgstr ""

seems to be the case since rP14098

elexis added inline comments.Dec 7 2017, 3:43 PM
binaries/data/mods/public/gui/common/l10n.js
11

nope, rP14954

temple added inline comments.Dec 7 2017, 4:47 PM
binaries/data/mods/public/gui/common/l10n.js
11

I'm not following.

If you're talking about the translate("HH:mm") a couple lines before this in lobby.js, it's ignored because there's another translate("HH:mm") line earlier in the file.

temple added a comment.Dec 7 2017, 5:06 PM

Again, maybe you're talking about something else.

common/tooltips.js:			// Translation: This string is part of the resources cost string on
common/tooltips.js-			// the tooltip for wall structures.
common/tooltips.js-			out.push(sprintf(translate("%(resourceIcon)s %(minimum)s to %(resourceIcon)s %(maximum)s"), {

lobby/lobby.js:	// Translation: Time as shown in the multiplayer lobby (when you enable it in the options page).
lobby/lobby.js-	// For a list of symbols that you can use, see:
lobby/lobby.js-	// https://sites.google.com/site/icuprojectuserguide/formatparse/datetime?pli=1#TOC-Date-Field-Symbol-Table

pregame/mainmenu.js:			// Translation: This is the second paragraph of a warning. The
pregame/mainmenu.js-			// warning explains that the user is using “non-shader“ graphics,
pregame/mainmenu.js-			// and that in the future this will not be supported by the game, so

Lobby is ignored as I said, but the other two are found (in public-gui-other.po):

#. Translation: This string is part of the resources cost string on
#. the tooltip for wall structures.
#: gui/common/tooltips.js:522
#, javascript-format
msgid "%(resourceIcon)s %(minimum)s to %(resourceIcon)s %(maximum)s"
msgstr ""

#. Translation: This is the second paragraph of a warning. The
#. warning explains that the user is using “non-shader“ graphics,
#. and that in the future this will not be supported by the game, so
#. the user will need a better graphics card.
#: gui/pregame/mainmenu.js:155
msgid "Please press \"Read More\" for more information or \"OK\" to continue."
msgstr ""
temple added a comment.Dec 8 2017, 1:16 AM

I'm going a bit crazy, I thought I tested but it's not working anymore. I get errors like this:

TIMER| pregame/styles.xml: 182.07 us
ERROR: JavaScript error: gui/common/settings.js line 100
ReferenceError: translateObjectKeys is not defined
  loadAIDescriptions@gui/common/settings.js:100:2
  loadSettingsValues@gui/common/settings.js:38:21
  @gui/common/settings.js:27:20
ERROR: JavaScript error: gui/common/tooltips.js line 9
ReferenceError: translate is not defined
  @gui/common/tooltips.js:9:2
TIMER| pregame/mainmenu.xml: 26.7428 ms
etc.

Commenting out all the code in l10n.js and I still get the error. But if I rename l10n.js to l20n.js then it works. Tried deleting the cache, add/delete from svn, same result.
So yeah, gonna take a little break...

temple updated this revision to Diff 4641.Dec 8 2017, 4:18 AM

Renamed l10n to datetime, seems more appropriate.
There's an l10n directory, maybe that's causing conflicts with recent commits? Or maybe I forgot to test and it was an older commit. Or maybe it's just my computer...

Vulcan added a comment.Dec 8 2017, 5:08 AM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
elexis added inline comments.Dec 8 2017, 9:46 PM
binaries/data/mods/public/gui/common/l10n.js
11

I'm talking about the translation comments starting at // Translation: and ending at the URL.
Those are supposed to be extracted to the *pot files because this information is supposed to be displayed on www.transifex.com for translators without them having to open the source code.

The mentioned extractor python tool is supposed to do that job, but // failts the pattern for a "multiline" comment.
Hence it only extracts the first line of the multiline comment and your nice URL isnt actually seen on transifex.

You can see that its already broken in svn and the current "*pot" file shows it too.
You can also run the script yourself (should be updateTemplates.py afaik) and generate the pot files yourself with the updated code and see if the multiline comment including the URL was successfully extracted.

The pattern /\*.*?\*/(?us) looks a bit obfuscated to me, perhaps they mean /** Translation: foo on the first line, * no the second, just like JSdoc?

Multiline translation comments are done though, as shown in my two examples.
So you mean because there's // in urls also? I'll look through the details later.

In D1116#45191, @temple wrote:

Multiline translation comments are done though, as shown in my two examples.

What do you mean with done?
Consider lobby.js L1400 in the currently committed code:

	// Translation: Time as shown in the multiplayer lobby (when you enable it in the options page).
	// For a list of symbols that you can use, see:
	// https://sites.google.com/site/icuprojectuserguide/formatparse/datetime?pli=1#TOC-Date-Field-Symbol-Table
	let timeString = Engine.FormatMillisecondsIntoDateStringLocal(msg.time ? msg.time * 1000 : Date.now(), translate("HH:mm"));

But in public-gui-lobby.pot we only find the first line extracted:

#. Translation: Time prefix as shown in the multiplayer lobby (when you enable it in the options page).
#: gui/lobby/lobby.js:1409
#, javascript-format
msgid "\\[%(time)s]"
msgstr ""

So that URL doesn't make it to transifex this way.

// Translation: is a single line comment.

The one browing through wiki pages or interpreting the mentioned multiline pattern line will find the solution.

Correction, not even the first line of the multiline comment is extrated.

#: gui/lobby/lobby.js:1045 gui/lobby/lobby.js:1406
msgid "HH:mm"
msgstr ""
temple added a comment.Dec 9 2017, 3:08 PM
('linecomment', re.compile(r'//.*')),
('multilinecomment', re.compile(r'/\*.*?\*/(?us)')),

It seems like multilinecomment would catch things like

/**
 * this
 */

Checking... I find one example, in summary/summary.js:

/**
 * Translation: Unicode encoded infinity symbol indicating a division by zero in the summary screen.
 */
var g_InfinitySymbol = translate("\u221E");

And that would show up in public-gui-other.pot. And... it doesn't? So it seems you're right that multilinecomment is broken. (On the other hand, "\u221E" is weird so maybe something else is going on instead.)

But having multiple linecomments does work evidently, and that's what I do here (it should match even with the url).

temple added a comment.Dec 9 2017, 5:15 PM

Getting off topic of course.

/**
 * Translation: blah
 * blah blah
 */

It doesn't pick up the multiline translation because the comment starts with the last * on the first line, instead of "Translation:".
In cpp multiline comments often start on the first line, so there you might see something like this:

/* Translation: blah
 * blah blah
 */

However, even that won't work! The final line is treated as empty, and translation comments need to be right before the translation!
So instead to get multiline translation comments working you have to do something like this :

/* Translation: blah
 * blah blah
 * blah */

(In source there's only two translation lines (in ps/scripting/JSInterface_Debug.cpp) and both use linecomment rather than attempting multilinecomment.)

temple added a comment.Dec 9 2017, 6:03 PM

The final line is treated as empty

Treated as empty because it removes the */ then strips the whitespace including newlines. The comments are stored along with their line numbers, so later when it encounters a translation command it checks if the last translation comments ended on the previous line. Because the newlines were stripped that check fails so the translation comments are ignored.

Could be fixed, I suppose. Not sure it's worth the effort though.

temple added a reviewer: Restricted Owners Package.Jan 5 2018, 4:20 PM
temple added a reviewer: Restricted Owners Package.Jan 17 2018, 1:17 AM
s0600204 requested changes to this revision.Oct 22 2018, 4:59 PM
s0600204 added a subscriber: s0600204.

Problems with multi-line comments not being picked up by the string extractor aside, may I request this patch be updated?

Notably, in the months since the last update, the function timeToString has been relocated to the mod mod (as it is used within the modio gui), and so the changes should be (re)applied there, and the instances of timeToString within the modio gui adapted.


I agree that the idea of having a function called durationToString fits these locations better. However, personally I'd want the output to be something more like 2h 30m or 5m 10s, as 2:30 seems more a defined time than a duration. (Not something I'm going to require as a change, just putting it out there.)

This revision now requires changes to proceed.Oct 22 2018, 4:59 PM