Merge lp:~widelands-dev/widelands/test-ngettext into lp:widelands

[GunChleoc]

Revised Lua ngettext to allow only unsigned integers. Created test for lua gettext functions.

We missed fixing ngettext when updating eris, so I've created a test.

[Miroslav Remák (miroslavr256)]

I'm not a fan of flooring the number in our C++ code. Passing a non-integer value to ngettext from Lua should result in an error (no idea what crashes you're talking about), IMO.

Where do we need to pass floats?

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1000. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/122450845.
Appveyor build 833. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_test_ngettext-833.

[Klaus Halfmann (klaus-halfmann)]

Having more tests is always good and the code looks OK for me.

test-ngettext klaus$ ./regression_test.py -b ./widelands

OTOH Miroslav question is valid, where or when do we need to show someting like
"You have 3.145972 Item(plurals) in you Inventory"
?

review: Approve (compile, regressiontest.py +/-)

[GunChleoc (gunchleoc)]

An example:

"If the food supply is steady, this mine can produce iron ore in %s on average."

%s is replaced by "%d second(s)", and %d can become floating point.

The floor is what we have always had implicitly (and it also happens to be what my particular language does), so there is no change in functionality. What we really want is a floating-point version of ngettext, but that will be some work.

[Miroslav Remák (miroslavr256)]

According to the gettext manual, "a simple gettext call with a form suitable for all values will do"[1] -- something like http://pastebin.com/raw/R2tGW9XH. The CLDR tables[2] clearly disprove this rule being universal, though.

Flooring produces incorrect results for Slovak and I bet a lot of other languages too, including English. Actually, assuming the information in the CLDR table is correct (and that my understanding of it is correct), Scottish Gaelic suffers as well.

I haven't really looked into it, but I think a fully-featured floating-point version of ngettext wouldn't be possible without writing our own i18n system/forking gettext. We'd probably need to extend the plural expression format and who knows if/how that would work with Transifex.

I'd really like to know how other projects deal with this issue, that is if they care at all. Perhaps we'd be better off using an abbreviation or a universal unit symbol, like "%d s", for the time being.

[1] http://www.gnu.org/savannah-checkouts/gnu/gettext/manual/html_node/Plural-forms.html (at the end of the page)
[2] http://www.unicode.org/cldr/charts/latest/supplemental/language_plural_rules.html

[GunChleoc (gunchleoc)]

The actual rule for Scottish Gaelic is to discard everything after the ., because we say "x seconds point y" for x.y seconds.

Not having things correct for Slovak doesn't please me though.

Maybe we could write a wrapper that will map floating point numbers to the appropriate integer numbers for the languages in our list, and then use those integers to call ngettext?

The alternative would be not to use any placeholders in these strings - that would increase the translation effort though. For example, we have 6 different types of Iron Mine, which currently can use the same string for their productivity texts. So, without the placeholders we get 6x the translation effort, plus if the calculation should change in the future, the strings would need to be retranslated.

[Miroslav Remák (miroslavr256)]

> Maybe we could write a wrapper that will map floating point numbers to the
> appropriate integer numbers for the languages in our list, and then use those
> integers to call ngettext?

That wouldn't work for Slovak, because floats need their own, different form. There is nothing to map to.

> The alternative would be not to use any placeholders in these strings - that
> would increase the translation effort though. For example, we have 6 different
> types of Iron Mine, which currently can use the same string for their
> productivity texts. So, without the placeholders we get 6x the translation
> effort, plus if the calculation should change in the future, the strings would
> need to be retranslated.

That doesn't sound like it's worth it, IMO, especially when we can simply use the symbol of the second. I'm not completely opposed to it, though -- it does get the job done.

[GunChleoc (gunchleoc)]

Floating point numbers can also become integers - how about we extend the %s second(s) translations to the actual numbers? On the other hand, we also have a translation memory with fuzzy matches now, so pulling similar strings from the Suggestions tab in Transifex shouldn't take that long...

[Miroslav Remák (miroslavr256)]

> Floating point numbers can also become integers
I don't see how that is relevant, what's it in response to?

[Klaus Halfmann (klaus-halfmann)]

Natural Languages normally have no idea about floating point numbers. But they can use fractions (will be ready in half a minute, can be done in a quarte of an hour). Some progress messages use phrases like "less then 5 seconds remaining" or "estimating 20 min for the Installation".

So we should provide a well defined and _known_ way to handle floats but encourage the programmers to use some of the above mentioned, or better, ideas.

So we might use floor _or_ round but only one of these and then stick with it.

So we should make this a tranlators hint on the homepage and merge this branch :-).

[GunChleoc (gunchleoc)]

> > Floating point numbers can also become integers
> I don't see how that is relevant, what's it in response to?

Let's say the value is currently 25.5 - then you would need to translate "%d seconds" with the floating point translation in Slovak. If the value changes to 25, you would need to change your translation - but how would you know? How would a programmer who is not aware of the issue know to give you a hint?

> Natural Languages normally have no idea about floating point numbers. But they
> can use fractions (will be ready in half a minute, can be done in a quarte of
> an hour). Some progress messages use phrases like "less then 5 seconds
> remaining" or "estimating 20 min for the Installation".

Not true. I can say "twenty-five point six seconds" just fine in any natural language on the planet. And Miroslav just stated that his language has special plural forms for floating point numbers ;)

To give you some examples. English and German will simply use the plural, Gaelic will say "five over twenty second point six" (the number 20 takes the singular, so it's "second", not "seconds")

Or maybe I'm just using too much linguist-speak here - "natural language" it meant as opposed to artificial language - i.e. Russian, not C++. It does not mean "what sounds more natural to you, how would you say it naturally?"

> So we should provide a well defined and _known_ way to handle floats but
> encourage the programmers to use some of the above mentioned, or better,
> ideas.
>
> So we might use floor _or_ round but only one of these and then stick with it.

The problem here is that it can give incorrect translations for some languages. There is nothing more annoying for translator than having grammar errors forced on them by a piece of software they're translating. It's the translator who will get blamed for it by the user, and not being able to fix it is very frustrating. I have been down that road many times.

> So we should make this a tranlators hint on the homepage and merge this branch
> :-).

I'd rather have consensus on what we want to do first - this is "just" a bunch of tests, so there is no hurry merging this.

[Miroslav Remák (miroslavr256)]

I fully agree with GunChleoc regarding Klaus' post. In Slovak we would say "twenty-five whole (ones) (and) six tenths of a second" and write "25,6 sekundy", where "sekundy" is the genitive case form of the word second. That's why a special plural form is needed.

As for the merge request, I will sum up my stance:
 - Ngettext should not accept a float parameter. Scripters are falsely led to believe that it will be handled correctly for every language.
 - To fix existing usages, instead of ngettext we should use a simple gettext (_) call with a symbol/abbreviation for the unit, or not use formatting placeholders at all, as long as the number is static. I am leaning towards the first option because it is easier to maintain.

> Let's say the value is currently 25.5 - then you would need to translate "%d
> seconds" with the floating point translation in Slovak. If the value changes
> to 25, you would need to change your translation - but how would you know? How
> would a programmer who is not aware of the issue know to give you a hint?

I am aware of that. This is the issue we're trying to solve/work around here. I thought you were replying to my previous post...

[GunChleoc (gunchleoc)]

Because the units appear in a longer sentence, I am in favour of having long unit names rather than symbols. Let's disallow the float and remove the placeholders. I can do that in his branch.

[Miroslav Remák (miroslavr256)]

Ok, sounds good to me.

[GunChleoc (gunchleoc)]

Done :)

review: Needs Resubmitting

[Miroslav Remák (miroslavr256)]

LGTM.

review: Approve

[Klaus Halfmann (klaus-halfmann)]

Fine for me.

review: Approve (compile / regression / review / check f1-help)

[GunChleoc (gunchleoc)]

Thanks!

@bunnybot merge

=== modified file 'data/tribes/buildings/productionsites/barbarians/coalmine/helptexts.lua'
--- data/tribes/buildings/productionsites/barbarians/coalmine/helptexts.lua	2015-12-02 04:01:09 +0000
+++ data/tribes/buildings/productionsites/barbarians/coalmine/helptexts.lua	2016-04-15 17:35:34 +0000
@@ -21,5 +21,5 @@
 
 function building_helptext_performance()
    -- TRANSLATORS: Performance helptext for a building
-   return pgettext("barbarians_building", "If the food supply is steady, this mine can produce coal in %s on average."):bformat(ngettext("%d second", "%d seconds", 32.5):bformat(32.5))
+   return pgettext("barbarians_building", "If the food supply is steady, this mine can produce coal in 32.5 seconds on average.")
 end

=== modified file 'data/tribes/buildings/productionsites/barbarians/coalmine_deep/helptexts.lua'
--- data/tribes/buildings/productionsites/barbarians/coalmine_deep/helptexts.lua	2015-12-02 04:01:09 +0000
+++ data/tribes/buildings/productionsites/barbarians/coalmine_deep/helptexts.lua	2016-04-15 17:35:34 +0000
@@ -21,5 +21,5 @@
 
 function building_helptext_performance()
    -- TRANSLATORS: Performance helptext for a building
-   return pgettext("barbarians_building", "If the food supply is steady, this mine can produce coal in %s on average."):bformat(ngettext("%d second", "%d seconds", 19.5):bformat(19.5))
+   return pgettext("barbarians_building", "If the food supply is steady, this mine can produce coal in 19.5 seconds on average.")
 end

=== modified file 'data/tribes/buildings/productionsites/barbarians/coalmine_deeper/helptexts.lua'
--- data/tribes/buildings/productionsites/barbarians/coalmine_deeper/helptexts.lua	2015-12-02 04:01:09 +0000
+++ data/tribes/buildings/productionsites/barbarians/coalmine_deeper/helptexts.lua	2016-04-15 17:35:34 +0000
@@ -21,5 +21,5 @@
 
 function building_helptext_performance()
    -- TRANSLATORS: Performance helptext for a building
-   return pgettext("barbarians_building", "If the food supply is steady, this mine can produce coal in %s on average."):bformat(ngettext("%d second", "%d seconds", 14.4):bformat(14.4))
+   return pgettext("barbarians_building", "If the food supply is steady, this mine can produce coal in 14.4 seconds on average.")
 end

=== modified file 'data/tribes/buildings/productionsites/barbarians/gamekeepers_hut/helptexts.lua'
--- data/tribes/buildings/productionsites/barbarians/gamekeepers_hut/helptexts.lua	2015-12-02 04:01:09 +0000
+++ data/tribes/buildings/productionsites/barbarians/gamekeepers_hut/helptexts.lua	2016-04-15 17:35:34 +0000
@@ -21,5 +21,5 @@
 
 function building_helptext_performance()
    -- TRANSLATORS: Performance helptext for a building
-   return pgettext("barbarians_building", "The gamekeeper pauses %s before going to work again."):bformat(ngettext("%d second", "%d seconds", 52.5):bformat(52.5))
+   return pgettext("barbarians_building", "The gamekeeper pauses 52.5 seconds before going to work again.")
 end

=== modified file 'data/tribes/buildings/productionsites/barbarians/goldmine_deep/helptexts.lua'
--- data/tribes/buildings/productionsites/barbarians/goldmine_deep/helptexts.lua	2015-11-17 09:15:18 +0000
+++ data/tribes/buildings/productionsites/barbarians/goldmine_deep/helptexts.lua	2016-04-15 17:35:34 +0000
@@ -20,5 +20,5 @@
 
 function building_helptext_performance()
    -- TRANSLATORS: Performance helptext for a building
-   return pgettext("barbarians_building", "If the food supply is steady, this mine can produce gold ore in %s on average."):bformat(ngettext("%d second", "%d seconds", 19.5):bformat(19.5))
+   return pgettext("barbarians_building", "If the food supply is steady, this mine can produce gold ore in 19.5 seconds on average.")
 end

=== modified file 'data/tribes/buildings/productionsites/barbarians/goldmine_deeper/helptexts.lua'
--- data/tribes/buildings/productionsites/barbarians/goldmine_deeper/helptexts.lua	2015-11-17 09:15:18 +0000
+++ data/tribes/buildings/productionsites/barbarians/goldmine_deeper/helptexts.lua	2016-04-15 17:35:34 +0000
@@ -20,5 +20,5 @@
 
 function building_helptext_performance()
    -- TRANSLATORS: Performance helptext for a building
-   return pgettext("barbarians_building", "If the food supply is steady, this mine can produce gold ore in %s on average."):bformat(ngettext("%d second", "%d seconds", 18.5):bformat(18.5))
+   return pgettext("barbarians_building", "If the food supply is steady, this mine can produce gold ore in 18.5 seconds on average.")
 end

=== modified file 'data/tribes/buildings/productionsites/barbarians/ironmine_deep/helptexts.lua'
--- data/tribes/buildings/productionsites/barbarians/ironmine_deep/helptexts.lua	2015-11-08 11:49:22 +0000
+++ data/tribes/buildings/productionsites/barbarians/ironmine_deep/helptexts.lua	2016-04-15 17:35:34 +0000
@@ -20,5 +20,5 @@
 
 function building_helptext_performance()
    -- TRANSLATORS: Performance helptext for a building
-   return pgettext("barbarians_building", "If the food supply is steady, this mine can produce iron ore in %s on average."):bformat(ngettext("%d second", "%d seconds", 39.5):bformat(39.5))
+   return pgettext("barbarians_building", "If the food supply is steady, this mine can produce iron ore in 39.5 seconds on average.")
 end

=== modified file 'data/tribes/buildings/productionsites/barbarians/ironmine_deeper/helptexts.lua'
--- data/tribes/buildings/productionsites/barbarians/ironmine_deeper/helptexts.lua	2015-11-08 11:49:22 +0000
+++ data/tribes/buildings/productionsites/barbarians/ironmine_deeper/helptexts.lua	2016-04-15 17:35:34 +0000
@@ -20,5 +20,5 @@
 
 function building_helptext_performance()
    -- TRANSLATORS: Performance helptext for a building
-   return pgettext("barbarians_building", "If the food supply is steady, this mine can produce iron ore in %s on average."):bformat(ngettext("%d second", "%d seconds", 17.6):bformat(17.6))
+   return pgettext("barbarians_building", "If the food supply is steady, this mine can produce iron ore in 17.6 seconds on average.")
 end

=== modified file 'data/tribes/scripting/help/global_helptexts.lua'
--- data/tribes/scripting/help/global_helptexts.lua	2015-11-20 16:39:38 +0000
+++ data/tribes/scripting/help/global_helptexts.lua	2016-04-15 17:35:34 +0000
@@ -50,6 +50,7 @@
 -- .. function:: format_seconds(seconds)
 --
 --    :arg seconds: number of seconds
+--    :type seconds: An unsigned integer
 --
 --    Returns a localized string to tell the time in seconds with the proper plural form.
 --    :returns: "1 second", or "20 seconds" etc.
@@ -62,6 +63,7 @@
 -- .. function:: format_minutes(minutes)
 --
 --    :arg minutes: number of minutes
+--    :type minutes: An unsigned integer
 --
 --    Returns a localized string to tell the time in minutes with the proper plural form.
 --    :returns: "1 minute", or "20 minutes" etc.
@@ -74,7 +76,9 @@
 -- .. function:: format_minutes_seconds(minutes, seconds)
 --
 --    :arg minutes: number of minutes
+--    :type minutes: An unsigned integer
 --    :arg seconds: number of seconds
+--    :type seconds: An unsigned integer
 --
 --    Returns a localized string to tell the time in minutes and seconds with the proper plural form.
 --    :returns: "1 minute and 20 seconds" etc.

=== modified file 'src/scripting/lua_globals.cc'
--- src/scripting/lua_globals.cc	2016-04-11 08:00:46 +0000
+++ src/scripting/lua_globals.cc	2016-04-15 17:35:34 +0000
@@ -176,12 +176,14 @@
 
 	:returns: The translated string.
 */
-// UNTESTED
 static int L_ngettext(lua_State * L) {
 	//  S: msgid msgid_plural n
 	const char* msgid = luaL_checkstring(L, 1);
 	const char* msgid_plural = luaL_checkstring(L, 2);
-	const uint32_t n = floor(luaL_checkdouble(L, 3));
+	const int32_t n = luaL_checkint32(L, 3);
+	if (n < 0) {
+		report_error(L, "Call to ngettext with negative number %d", n);
+	}
 
 	lua_getglobal(L, "__TEXTDOMAIN");
 	if (!lua_isnil(L, -1)) {

=== added file 'test/maps/lua_testsuite.wmf/scripting/gettext.lua'
--- test/maps/lua_testsuite.wmf/scripting/gettext.lua	1970-01-01 00:00:00 +0000
+++ test/maps/lua_testsuite.wmf/scripting/gettext.lua	2016-04-15 17:35:34 +0000
@@ -0,0 +1,29 @@
+test_descr = lunit.TestCase("Gettext Test")
+
+--  Since we run the test suite in English, this will only test
+--  whether gettext functions crash or not
+
+function test_descr:test_gettext()
+   set_textdomain("tribes")
+   assert_equal("foo", _"foo")
+   assert_equal("foo", _("foo"))
+   assert_equal(_"Carrier", _("Carrier"))
+end
+
+function test_descr:test_ngettext()
+   assert_equal("foo", ngettext("foo", "bar", 1))
+   assert_equal("bar", ngettext("foo", "bar", 1000))
+   assert_equal("1 foo", ngettext("%1% foo", "bar %1%", 1):bformat(1))
+   assert_equal("bar 2", ngettext("%1% foo", "bar %1%", 2):bformat(2))
+   -- ngettext is undefined for floating point or negative numbers.
+   assert_error("Call to ngettext with floating point", function()
+      ngettext("foo", "bar", 1.5)
+   end)
+   assert_error("Call to ngettext with negative number", function()
+      ngettext("foo", "bar", -1)
+   end)
+end
+
+function test_descr:test_pgettext()
+   assert_equal("bar", pgettext("foo", "bar"))
+end

=== modified file 'test/maps/lua_testsuite.wmf/scripting/init.lua'
--- test/maps/lua_testsuite.wmf/scripting/init.lua	2016-01-25 18:50:54 +0000
+++ test/maps/lua_testsuite.wmf/scripting/init.lua	2016-04-15 17:35:34 +0000
@@ -23,6 +23,7 @@
 -- =================================
 include "map:scripting/egbase.lua"
 
+include "map:scripting/gettext.lua"
 include "map:scripting/math_random.lua"
 include "map:scripting/string_bformat.lua"
 include "map:scripting/path.lua"