Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Translate timezone names #12901

Merged
merged 7 commits into from Jun 7, 2018
Merged

Conversation

c960657
Copy link
Contributor

@c960657 c960657 commented May 13, 2018

The timezone selector in the sites manager is not localised but shows all city names in English.

When using Matomo in Italian, a user would expect the list to contain “Roma”, not “Rome”. And a user using Matomo in Russian would look for “Москва́”, not “Moscow”. Even an English user would expect “Dumont d’Urville” instead of “DumontDUrville” (for Antarctica/DumontDUrville).

CLDR contains translations of all city names. However, the CLDR spec suggests that timezone selectors should allow users to select the country name, not the city name. City names should only be shown for countries spanning multiple timezones. Of course, this is just a UI recommendation, not a requirement of any sort, but I think it makes sense.

You quite often see this long list of cities in timezone selectors in various software, and for a developer who is familiar with the tz database it may seem natural. I suspect that this UI is popular, because it is easy to construct the list based on the timezone identifiers alone. From and end-user point of view I think that a list of countries is the natural choice. For some countries it is not obvious which city is the one used in the tz database (the capital may not be the largest city), and for some regions the tz identifier is not a city name but the name of an island. Countries are well-defined and used in many contexts.

This PR implements a UI as suggested in the CLDR spec. It looks like this (excerpt):

  • Europe
    • Poland
    • Portugal - Azores
    • Portugal - Lisbon
    • Portugal - Madeira
    • Romania

The timezones are now grouped by the same continent definition as used elsewhere in Matomo, instead of the definition used in the tz database. This is more consistent from an end-user point of view.

@Findus23 Findus23 added the c: Usability For issues that let users achieve a defined goal more effectively or efficiently. label May 13, 2018
@sgiehl sgiehl added the Needs Review PRs that need a code review label May 13, 2018
@c960657 c960657 force-pushed the translate-timezones branch 2 times, most recently from c49303e to 6a7a52f Compare May 15, 2018 06:26
@sgiehl sgiehl added this to the 3.6.0 milestone May 21, 2018
@c960657 c960657 force-pushed the translate-timezones branch 4 times, most recently from b66b6f0 to 8e20917 Compare May 23, 2018 22:18
Copy link
Member

@diosmosis diosmosis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, works in sites manage page & install page. Left a couple comments re code.

@sgiehl @tsteur or @mattab maybe you guys want to take a look too.

ksort($return);
foreach ($return as $continent => $countries) {
asort($return[$continent]);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think you'll need to iterate by reference for the asort() to work (ie, foreach ($return as $continent => &$countries)).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could need the & if I did asort($countries). But here I just sort the value in the outer array by reference, so I believe it's okay.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, misread this line.

}
}
} catch (\Exception $e) {
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What specifically can throw an exception here? Would be good put the try-catch around only the code we're expecting could throw, so other exceptions aren't silenced. (Same for some try-catches below.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only new DateTimeZone() will throw. I have changed the try/catch block to include only that, and do skip the rest of the loop if an exception is thrown.

@@ -337,6 +338,10 @@ public function getSitesWithAdminAccess($fetchAliasUrls = false, $pattern = fals
}
}

foreach ($sites as &$site) {
$site['timezone_name'] = $this->getTimezoneName($site['timezone']);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, this property should be added to other getSitesWith... methods. Would be good to put it in getSitesFromIds() & then a for loop after the above call to getPatternMatchSites().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


$resultWanted = array(
0 => array("idsite" => 1, "name" => "site1", "main_url" => "http://piwik.net", "ecommerce" => 0, "excluded_ips" => "", 'sitesearch' => 1, 'sitesearch_keyword_parameters' => '', 'sitesearch_category_parameters' => '', 'excluded_parameters' => '', 'excluded_user_agents' => '', 'timezone' => 'UTC', 'currency' => 'USD', 'group' => '', 'keep_url_fragment' => 0, 'type' => 'website', 'exclude_unknown_urls' => 0),
1 => array("idsite" => 3, "name" => "site3", "main_url" => "http://piwik.org", "ecommerce" => 0, "excluded_ips" => "", 'sitesearch' => 1, 'sitesearch_keyword_parameters' => '', 'sitesearch_category_parameters' => '', 'excluded_parameters' => '', 'excluded_user_agents' => '', 'timezone' => 'UTC', 'currency' => 'USD', 'group' => '', 'keep_url_fragment' => 0, 'type' => 'website', 'exclude_unknown_urls' => 0),
0 => array("idsite" => 1, "name" => "site1", "main_url" => "http://piwik.net", "ecommerce" => 0, "excluded_ips" => "", 'sitesearch' => 1, 'sitesearch_keyword_parameters' => '', 'sitesearch_category_parameters' => '', 'excluded_parameters' => '', 'excluded_user_agents' => '', 'timezone' => 'UTC', 'timezone_name' => 'GMT', 'currency' => 'USD', 'group' => '', 'keep_url_fragment' => 0, 'type' => 'website', 'exclude_unknown_urls' => 0),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timezone name for UTC should be UTC and not GMT, right?

Copy link
Contributor Author

@c960657 c960657 May 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, CLDR uses GMT. This translation is used in most locales. I don't have much opinion on the specific translation, but I generally think we should follow CLDR (a high-quality source backed by large industry players such as Microsoft, Google and Apple) unless there are strong arguments against it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an astronomy pedantic I have to mention that UTC and GMT are not the same, even though they are quite similar:

For most purposes, UTC is considered interchangeable with Greenwich Mean Time (GMT) but GMT is no longer precisely defined by the scientific community.
https://en.wikipedia.org/wiki/Coordinated_Universal_Time

But to be fair the differences don't really matter, even though I think most people expect reading UTC in an IT-context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, it's important to write "UTC" everywhere (and "GMT" should not be visible anywhere) 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is consensus that we want this, I'll skip importing translations for “UTC” from CLDR and fetch them from Transifex instead. However, I would much prefer if we could outsource such decisions to CLDR, so we can focus on matters specific to web analytics.

Even though I tend to agree that UTC is more prevalent these days and is technically more correct, I would say that both are acceptable. E.g. Google Analytics uses GMT.

FWIW, Microsoft used GMT in previous versions of Windows, but now they use UTC, at least in the English version. Microsoft are active contributors to CLDR, but it seems they have chosen to deviate from the CLDR data her. They may wish to support a proposal to change the CLDR translation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes @c960657 we want to see UTC everywhere (and nowhere GMT)

$this->markTestSkipped('timezones needs to be supported');
}

Translate::loadAllTranslations();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to not use translations in this test and instead assert that the translation keys are what we expect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really. This would make e.g. “UTC+1” be returned as “Intl_GmtFormat” (the offset placeholder would be ignores), and we wouldn't be able to test the distinction between known and unknown cities (Asia/Foo_Bar vs. America/New_York).

* @group APITest
* @group Plugins
*/
class APITest extends \PHPUnit_Framework_TestCase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for adding a test!

@c960657
Copy link
Contributor Author

c960657 commented May 30, 2018

Thanks for the review :)

@c960657
Copy link
Contributor Author

c960657 commented May 31, 2018

The translation for UTC is now fetched from plugins/SitesManager/lang/en.json instead.

@diosmosis diosmosis merged commit 3bb7054 into matomo-org:3.x-dev Jun 7, 2018
@c960657 c960657 deleted the translate-timezones branch June 8, 2018 04:58
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
* Translate timezone names

* Add translations

* Remove unnecessary help text

* Wrap less code in try/catch

* Always populate timezone_name

* Use "UTC" instead of "GMT" from CLDR

* Update screenshot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Usability For issues that let users achieve a defined goal more effectively or efficiently. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants