-
Notifications
You must be signed in to change notification settings - Fork 64
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
Add Hungarian localization #274
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these turned out a bit differently from what I expected after Transifex, so I had to do some updating on Transifex. Sorry for the inconvenience.
No problem, that's the review goal is 😉 |
Ok, will I get a new notification if the new review is available, or I have to check back regularly? Thanks. |
I'll add comment here and then you'll get a notification. Did you already update translation on Transifex? |
Yes, I did it before commenting on the review here. |
IMHO missing whitespace after "merge": {
"default": {
"default": "Soroljon be {modifier}",
"name": "Soroljon be {modifier}a(z) {way_name} felé",
"destination": "Soroljon be {modifier} ehhez: {destination}"
}, |
Fixed it, tho it's really hard to keep track since almost every value exist 6 times. |
Actually no, only "merge": {
"default": {
"default": "Soroljon be {modifier}",
"name": "Soroljon be {modifier}a(z) {way_name} felé",
"destination": "Soroljon be {modifier} a(z) {destination} irányába"
}, And there are several similar cases, I recommend to use Transifex search for translated strings with |
I only saw one, fixed, then searched for }a and found nothing. Is it possible to remove the ultimate redundancy somehow so every string would be there only 1 times? |
I pushed your updated translation, please review result again. IMHO there are extra whitespaces in "phrase": {
"two linked by distance": "{instruction_one} , majd {distance} múlva, {instruction_two}",
"two linked": "{instruction_one} , majd {instruction_two}",
"one in distance": "{distance} múlva {instruction_one}",
"name and ref": "{name} ( {ref} )",
"exit with number": "{exit} kijárat"
}, |
Sorry, i wanted to make sure there is always whitespace, tomorrow I will check them, so to be clear there can be braces and commas after them ? |
I suppose there should be just |
@@ -18,6 +18,7 @@ | |||
"fi": "Käänny vasempaan tielle Way Name (Ref1)", | |||
"fr": "Tourner à gauche sur Way Name (Ref1)", | |||
"he": "פנה שמאלה לWay Name (Ref1)", | |||
"hu": "Forduljon be balra a(z) Way Name ( Ref1 ) irányába", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is extra whitespaces sample
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed a few lines again on Transifex.
languages/translations/hu.json
Outdated
"namedistance": "Hajtson {direction} felé a(z) {way_name} , {distance} távolságig" | ||
"default": "Hajtson a(z) {direction} irányába", | ||
"name": "Hajtson a(z) {direction} irányába a(z) {way_name} szakaszon", | ||
"namedistance": "Hajtson a(z) {direction} irányába a(z) {way_name} felé {distance} távolságig" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra whitespace after {distance}
IMHO
languages/translations/hu.json
Outdated
"name": "Hajtson a körforgalomba, majd hagyja el a körforgalmat a(z) {way_name} felé", | ||
"destination": "Hajtson a körforgalomba, majd hagyja el a körforgalmat a(z) {destination} irányába" | ||
"name": "Hajtson a körforgalomba, majd hagyja el a körforgalmat a(z) {way_name} felé", | ||
"destination": "Hajtson a körforgalomba, majd hagyja el a körforgalmat a(z) {destination} irányába" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here after {way_name}
and {destination}
too
Next commit pushed. But now there are extra whitespaces 😞 |
Transifex really plays out its cards... oh well, tried to fix them all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed on transifex
Committed last fix. Translation looks good for me now 👍 BTW maybe you could also add translation to the frontend? It's necessary just add new hu.js into i18n folder with similar content:
and register it in src/localization.js? You could do this in your own new pull request or just send/type translation for me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, also I created a pull request for the frontend language file.
Translation is also checked with frontend: So if all is ok, maybe this could be merged? Also IMHO new version of whole package should be released already, maybe maintainers could do this finally? /cc @1ec5 |
It looks and reads correct. |
Maybe @danpaz could merge this PR? |
languages/translations/hu.json
Outdated
"modes": { | ||
"ferry": { | ||
"default": "Hajtson fel a kompra", | ||
"name": "Hajtson fel a kompra a(z) {way_name} felé", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Wiktionary, it would be “a” or “az” depending on the first letter of way_name
. a(z)
likely won’t sound right when spoken by a text-to-speech engine. We should add a simple grammar rule that changes it to “a” or “az” based on way_name
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and @kiskunk and I agreed to try to do this in separate PR. But I could try to include this into already prepared. I just worry that PR may become too huge to be reviewed completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kiskunk, would these regular expressions correctly match all the vowels and consonants that could appear at the beginning of a name?
"article": [
["^ [aáäåâeéëiíïoóőöuúűüyÿ]", " az "],
["^ [^aáäåâeéëiíïoóőöuúűüyÿ]", " a "]
]
There would also need to be an override file similar to the French one:
osrm-text-instructions/languages/overrides/fr.js
Lines 3 to 9 in ed1a4a9
var replaces = [ | |
[' (de )?\{destination\}', ' {destination:preposition}'], // eslint-disable-line no-useless-escape | |
[' (le rond-point )?\{rotary_name\}', ' {rotary_name:rotary}'], // eslint-disable-line no-useless-escape | |
[' fin (de )?(la route )?\{way_name\}', ' fin {way_name:preposition}'], // eslint-disable-line no-useless-escape | |
[' \{way_name\}', ' {way_name:article}'], // eslint-disable-line no-useless-escape | |
[' (à )?\{waypoint_name\}', ' {waypoint_name:arrival}'] // eslint-disable-line no-useless-escape | |
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kiskunk, would these regular expressions correctly match all the vowels and consonants that could appear at the beginning of a name?
"article": [ ["^ [aáäåâeéëiíïoóőöuúűüyÿ]", " az "], ["^ [^aáäåâeéëiíïoóőöuúűüyÿ]", " a "] ]There would also need to be an override file similar to the French one:
osrm-text-instructions/languages/overrides/fr.js
Lines 3 to 9 in ed1a4a9
var replaces = [
[' (de )?{destination}', ' {destination:preposition}'], // eslint-disable-line no-useless-escape
[' (le rond-point )?{rotary_name}', ' {rotary_name:rotary}'], // eslint-disable-line no-useless-escape
[' fin (de )?(la route )?{way_name}', ' fin {way_name:preposition}'], // eslint-disable-line no-useless-escape
[' {way_name}', ' {way_name:article}'], // eslint-disable-line no-useless-escape
[' (à )?{waypoint_name}', ' {waypoint_name:arrival}'] // eslint-disable-line no-useless-escape
];
It would be good for most of the time, but not always, if there is any way to determine that a way_name/destination is a highway or not, it could be helpful, without that the globally accepted "a(z)" format is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it seems we need add rules for Hungarian grammatical cases where we will recognize status street names and also change a/az
articles appropriately. We can treat all unrecognized as highway refs and always use az
there (but all foreign street names will be also here 😢) or add special rules to match "typical" highway refs (capital letter with digits or digits only, etc).
Currently I know 11 Hungarian status street names:
https://github.com/yuryleb/garmin-russian-tts-voices/blob/121a9bfe49eba702d5f244cd34ec8b0b7201aabb/src/Pycckuu__Milena%202.10/RP_RSET.TXT#L482-L493
Maybe you could extend this list more?
Also what grammatical cases could be applied for Hungarian messages? For example Russian localization supports 4 cases (accusative, dative, genitive and prepositional).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, so whether the instruction uses “a” or “az” depends on whether it’s a motorway? Or does it depend on some other aspect of the road related to motorways, like whether there’s a ref
instead of a name
?
Also I already had to write my own logic for correcting a(z) including highway names, since my project needed to proceed further. (Its on android, not js and not regex based) |
If you’re using the Mapbox Navigation SDK for Android, note that improvements to OSRM Text Instructions are incorporated into the Mapbox Directions API and therefore the navigation SDK as well. Ideally the navigation SDK would take care of details like this so you don’t have to in your application code. |
An example of special case dor a az: usually highways in hungary starts with an M and then a number and the pronounciation is "emm" so thats why it needs an az before it in this special case. |
Makes sense 😉 @kiskunk, could you please also answer for my questions above? And also should Hungarian street names be changed together with status names for a grammatical case similar to Russian streets? |
So here is a complete list of possible street ending names (i mean like in Boulevard Street where "Street" is the ending) it includes those 11 too from your link. WayNameEnding[] wayNameEndings = { Grammatical cases as far as i could determine it would require only 2 of ours, but i do not know the name of those cases. I think with using the grammar file for standard street names to balance out a/az AND making a rule for M0, M1 ... M85 ... etc would cover almost all except the non-hungarian street names (there are only a few) For the grammatical cases and the M0..M1 highway names its another dimension of difficulty (at least when the Mapbox SDK tries to read it out loud), because of how it would say the numbers, the numbers also would require a different suffix. For example M0 would be read like "emm nulla" but since we want to use it in context and a sentence, it would be "emm nullás", and thats not the end because it would still need the "section" word after it (which i think its already there in transifex because its a general translation) or to be better for the ears, the logic from the previous grammar cases, making it into "emm nulláson" or "emm nullásra", to write this logic it requires to have the numbers 0-20,30,40,50,60,70,80,90,100,200,300.. etc to be defined too with that 2 grammar case, maybe its enough to stay below 1000, i don't think that would come up in any hungarian highway names. |
An SSML-aware text-to-speech engine might already handle optimizing numbers or alphanumeric words for speech. (The Mapbox Directions API wraps
That’s reasonable. The only reason I brought up the grammar file was because of “a(z)”, which a text-to-speech engine is unlikely to pronounce correctly under any circumstance, unfortunately. That said, this PR doesn’t have to be the end of Hungarian localization work. If you think optimizing articles and numbers would be enough of a rabbit hole, we can merge this PR and would welcome any additional contributions regarding grammar. |
If we're talking about articles change only, it's not too hard to implement here 😉 BTW how But adding 2 grammatical cases support will be not too hard too if we need to change street status name only. @kiskunk, am I understand properly that "turn right onto the street" instruction with
And we don't need to change the |
I think a(z) is ok for now. As for jumbers it would be "megérkezett az 1. célponthoz" but "megérkezett a 2. célponthoz" so its atain an another level of complication of what letter the spoken number would start with. And for the street name it would be: Forduljon jobbra a Dohány utcára. without the "felé" and yes Dohány would not change |
The image looks correct. My list was from Wikipedia, so i will have to try to update that too with alagút. thanks. Felé and Irányába are interchange-able, they both represent one of the 2 cases, szakaszon represents the other. I just added the other 2 for a little bit of word diversity, I used felé for way_names, and irányába for destination. |
One more question, @kiskunk: how to proceed foreign names - I suppose it's better to keep these felé/irányába/szakaszon words on their original place in instructions? |
yes, thats the best way i can imagine |
languages/translations/hu.json
Outdated
"straight": { | ||
"default": "Hajtson egyenesen", | ||
"name": "Hajtson egyenesen {way_name:article} felé", | ||
"destination": "Hajtson egyenesen {destination} irányba" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kiskunk, maybe there should be irányába
not irányba
? And article a(z)
is missing here?
languages/translations/hu.json
Outdated
"straight": { | ||
"default": "Hajtson tovább egyenesen", | ||
"name": "Hajtson tovább egyenesen {way_name:article} felé", | ||
"destination": "Hajtson tovább egyenesen {destination} irányba" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kiskunk, maybe there should be irányába
not irányba
? And article a(z)
is missing here?
languages/translations/hu.json
Outdated
"uturn": { | ||
"default": "Forduljon vissza az út végén", | ||
"name": "Forduljon vissza {way_name:article} végén", | ||
"destination": "Forduljon vissza {destination} irányban az út végén" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kiskunk, maybe article a(z)
is missing here?
Yes, I corrected the irányába, and added the missing a(z) |
Looks good to me, great job, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙇
- Added a Hungarian localization and grammar. [#274](#274) - Added a Japanese localization. [#277](#277) - Added an Arabic localization. [#267](#267) - Added a Slovenian localization. [#264](#264) - Updated Russian arrive instructions. [#278](#278) - Updated French grammar with 'chaussée' status street name and better articles matching. [#268](https://github.com/Project-OSRM/osrm-text-instructions/pull/268)[#279](https://github.com/Project-OSRM/osrm-text-instructions/pull/279)
Issue
Initial Hungarian translation contributed by @kiskunk initally discussed in #271.
Thanks a lot!
Tasklist