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

Localization support #1271

Closed
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@MaxLeiter
Member

MaxLeiter commented Jun 25, 2017

Major WIP, maybe more of a POC.

Added a few translations to act as examples, not going to add the rest until I'm / we're sure this is how to go about it.

Need to add server-side option for default language and language selector on the client. Also need to setup translations server-side for use in index.html and all the plugins/inputs (like what commands return).

Todo (missing stuff):

  • Client settings option to change language
  • Support switching language with page reload
  • Complete en-US translations
  • Server-side default language option
  • Tests
  • Finalize JSON layout

@MaxLeiter MaxLeiter changed the title from Translation support to Early work on translation support Jun 25, 2017

@@ -35,7 +35,8 @@ module.exports = function() {
.engine("html", expressHandlebars({
extname: ".html",
helpers: {
tojson: (c) => JSON.stringify(c)
tojson: (c) => JSON.stringify(c),
translate: (c) => i18n.t(c)

This comment has been minimized.

@MaxLeiter

MaxLeiter Jun 26, 2017

Member

doesn't currently do anything

This comment has been minimized.

@xPaw

xPaw Jun 26, 2017

Member

Client JS needs to perform most of the translations. Only initial Loading/enable js strings need to be translated.

This comment has been minimized.

@MaxLeiter

MaxLeiter Jun 26, 2017

Member

Agreed, but need to somehow translate index.html strings

This comment has been minimized.

@xPaw

xPaw Jun 26, 2017

Member

I'm guessing leave most of them untranslated tokens, add a class like translate, and iterate them in client JS on load?

This comment has been minimized.

@MaxLeiter

MaxLeiter Jun 28, 2017

Member

Is now used

@MaxLeiter MaxLeiter changed the title from Early work on translation support to Localization support Jun 26, 2017

@@ -72,6 +72,10 @@ function buildChatMessage(data) {
text.html(templates.actions[type](data.msg));
}
if (template === "msg" && data.msg.type !== "motd" && (data.msg.from === "undefined" || data.msg.from === "" || data.msg.type === "error")) {

This comment has been minimized.

@xPaw

xPaw Jun 26, 2017

Member

What messages have from undefined?

This comment has been minimized.

@MaxLeiter

MaxLeiter Jun 26, 2017

Member

None 👍

"now_known_as": "You're now known as {{new_nick}}"
}
}

This comment has been minimized.

@xPaw

xPaw Jun 26, 2017

Member

identation has gone wrong somewhere

"has_changed": "has changed the topic to",
"topic_is": "The topic is:"
},
"topic_set_by": {

This comment has been minimized.

@xPaw

xPaw Jun 26, 2017

Member

This probably needs to be a single string...

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Jun 27, 2017

This basically works now. Just need to:

  1. translate index.html before it renders
  2. translate server log() calls
  3. default language server setting (need to somehow access defaults in lounge.js)
  4. write tests 😢
@williamboman

This comment has been minimized.

Member

williamboman commented Jun 27, 2017

translate server log() calls

I don't think server logs should be localized, I don't think I've ever seen it done elsewhere. It also opens up for misinterpretations due to server logs being more low-level and detailed by nature. Server logs should be English only imo.

@MiniDigger

This comment has been minimized.

Contributor

MiniDigger commented Jun 27, 2017

translated messages are bad. when you google an error, you would find the most results in english.
also, imagine someone opening a issue here, only able to provide a log in chinese or smth.

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Jun 27, 2017

Makes my life easier 👍

"now_known_as": "You're now known as {{new_nick}}"
}
}

This comment has been minimized.

@astorije

astorije Jun 30, 2017

Member

Could you add an EOF line please?

This comment has been minimized.

@MaxLeiter
"search_among_user_list": "J'adore The Lounge",
"disconnect_from": "hopefully this works : {{server}}?"
}

This comment has been minimized.

@astorije

astorije Jun 30, 2017

Member

WAT 😂

This comment has been minimized.

@MaxLeiter

MaxLeiter Jul 5, 2017

Member

Needed to test and don't know French

{{else}}
{{> ../user_name nick=invited}}
{{/if}}
to
{{translate 'client.invited.to'}}
{{{parse channel}}}

This comment has been minimized.

@astorije

astorije Jun 30, 2017

Member

I don't think this is sustainable. Assuming sentence order being [nick] [invited] [nick] [to] [channel] is likely not going to be true in a bunch of languages. For example, in French, it would work when reporting about someone else: [bob] [a invité] [alice] [sur] [#thelounge], but when user gets invited, that would be: [bob] [vous] [a invité sur] [#thelounge].

I barely remember anything from my German years, but I believe the structure would be something like: [bob] [has] [you] [to] [#thelounge] [invited] or something like that.

This comment has been minimized.

@MaxLeiter

MaxLeiter Jul 5, 2017

Member

Yeah, not too sure how to solve this -- need to do some reading up on Handlebars. http://i18next.github.io/i18next/pages/doc_templates.html has an "extended handlebars helper" but for the life of me I can't figure out how it works

This comment has been minimized.

@PolarizedIons

PolarizedIons Sep 15, 2017

Contributor

I still very much don't like this, but I have not found a nice solution either.

The only thing I could come up with is having a client.invite.userFirst = true in the translation file, and using that here to decide if the user/'you' or the channel goes first, then using your approach and having client.invited.prefix, .middle, and .suffix. I don't fully like this either though, since it puts logic inside the translation file

@@ -1,3 +1,3 @@
{{> ../user_name nick=from}}
is now known as
{{translate 'client.now_known_as'}}
{{> ../user_name nick=new_nick}}

This comment has been minimized.

@astorije

astorije Jun 30, 2017

Member

Previous comment applies here as well, and most templates I believe. Could this be made so that the strings in the JSON files contain the other elements, like {{from}} is now known as {{new_nick}}?

This comment has been minimized.

@MaxLeiter

MaxLeiter Jun 30, 2017

Member

Yeah, the Handlebars template supports passing in options, just need to figure out how 😄

This comment has been minimized.

@MaxLeiter

MaxLeiter Jul 5, 2017

Member

Actually, not sure how to solve this exactly, because it requires passing a template into a template...

@astorije

This comment has been minimized.

Member

astorije commented Jun 30, 2017

I haven't reviewed but just took a random look through the PR.
This is very exciting, I can't wait! Pretty sure language files are going to flourish after that!!

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Jul 10, 2017

@astorije

This comment has been minimized.

Member

astorije commented Jul 17, 2017

Because of wycats/handlebars.js#1360 (comment) (there won't be new features to Handlebars at least for now), how do we want to handle this? :(

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Jul 17, 2017

There may be a solution we haven't thought of? @xPaw any ideas? Worst comes to worst we don't translate a few templates.

@astorije astorije referenced this pull request Aug 13, 2017

Merged

Initial status message condensing #759

15 of 15 tasks complete

@astorije astorije self-assigned this Aug 31, 2017

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Sep 6, 2017

Should i rebase this or not worth the effort until handlebars updates? (which may be never...)

Comes down to if we want half-baked or not

@astorije

This comment has been minimized.

Member

astorije commented Sep 6, 2017

If you can rebase, that would be awesome. I am still very much looking forward to get involved with this and would absolutely love for us to support multiple languages soon :)

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Sep 9, 2017

Rebased to master and updated to translate new strings.

<input class="input" name="user">
</label>
</div>
<div class="col-xs-12">
<label>
Password
<span data-translate="true">index.signin.password</span>
<input class="input" type="password" name="password">
</label>
</div>
<div class="col-xs-12 error" style="display: none;">

This comment has been minimized.

@PolarizedIons

PolarizedIons Sep 12, 2017

Contributor

Are you missing a data-translate="true" here?

This comment has been minimized.

@MaxLeiter

MaxLeiter Sep 12, 2017

Member

Nice catch

</div>
</div>
<div class="help-item">
<div class="subject">
<<<<<<< HEAD

This comment has been minimized.

@PolarizedIons

PolarizedIons Sep 15, 2017

Contributor

Looks like you forgot to deal with a merge conflict here

</div>
</div>
<div class="help-item">
<div class="subject">
<<<<<<< HEAD

This comment has been minimized.

@PolarizedIons

PolarizedIons Sep 15, 2017

Contributor

and here

</span>
{{/equal}}
<button class="menu" aria-label="Open the context menu"></button>
<button class="menu" aria-label="{{translate 'open_context_menu'}}"></button>

This comment has been minimized.

@PolarizedIons

PolarizedIons Sep 15, 2017

Contributor

open_context_menu -> client.open_context_menu

@@ -9,7 +9,8 @@ module.exports = function(irc, network) {
var lobby = network.channels[0];
var msg = new Msg({
text: "You're now known as " + data.nick
text: "now_known_as",

This comment has been minimized.

@PolarizedIons

PolarizedIons Sep 15, 2017

Contributor

now_known_as -> server.now_known_as

"change_fail":"Failed to update your password",
"change_fail_entered_not_match":"Both new password fields must match",
"change_fail_entered_not_match_account":"The current password field does not match your account password",
"change_success":"Successfully updated your password, all your other sessions were logged out"

This comment has been minimized.

@PolarizedIons

PolarizedIons Sep 15, 2017

Contributor

I believe since #1199, sessions are no longer logged out, so this should probably be updated

This comment has been minimized.

@MaxLeiter

MaxLeiter Sep 15, 2017

Member

I don't see any strings being changed in that PR, so I think this is fine

This comment has been minimized.

@PolarizedIons

PolarizedIons Sep 15, 2017

Contributor

fair enough

"deop":"Remove op ({{deopMode}}) from one or several user in the current channel.",
"devoice":"Remove voice ({{devoiceMode}}) from one or several users in the current channel.",
"disconnect":"Disconnect from the current network with an optionally-provided message.",
"invite":"Invite a user to the specified channel. If {{channel}} is ommitted, user will be invited to the current channel.",

This comment has been minimized.

@PolarizedIons

PolarizedIons Sep 15, 2017

Contributor

ommitted -> omitted

@PolarizedIons

This comment has been minimized.

Contributor

PolarizedIons commented Sep 15, 2017

In the console I see

i18next::backendConnector: loaded namespace translation for language en-US ...
i18next::backendConnector: loaded namespace translation for language fr ...

Does it load ALL languages? Shouldn't it just load the current or default language?


It also tries to load /locales/en.json and 404s. I remember you saying it will load the language.json then override that with language-COUNTRY.json, to allow for regional translations. I'm indifferent about this 404, but it would be nice to not have


The condense messages aren't translated at all


So much for a "quick check" on the pr 😛 So happy to see this PR mostly working, and looking forward to it eventually being merged!

@@ -101,6 +102,10 @@ function buildChatMessage(msg) {
renderPreview(preview, renderedMessage);
});
if (template === "msg" && type !== "motd" && (msg.from === "" || type === "error")) {

This comment has been minimized.

@xPaw

xPaw Sep 15, 2017

Member

Can you just send translate: true from the server where it sends tokens instead?

This comment has been minimized.

@MaxLeiter

@astorije astorije added this to the 2.6.0 milestone Sep 20, 2017

@xPaw xPaw modified the milestones: 2.6.0, 2.7.0 Oct 19, 2017

@astorije astorije modified the milestones: 2.7.0, 3.0.0 Dec 5, 2017

@astorije

This comment has been minimized.

Member

astorije commented Feb 18, 2018

@MaxLeiter, I know this is going to be a huge pain, but would you mind rebasing this? I'd like to start reviewing it.

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Feb 18, 2018

@astorije sure, but still need to decide how to handle different word ordering/insertion.

Should I go with

{{before}} {{middle}} {{end}}

?

@astorije

This comment has been minimized.

Member

astorije commented Feb 18, 2018

@astorije sure, but still need to decide how to handle different word ordering/insertion.

Yes, that's precisely why I want to start reviewing it :)

I can't answer that question before I take a serious look at it. Just rebase and ignore that issue for now.

@astorije

This comment has been minimized.

Member

astorije commented Feb 18, 2018

@MaxLeiter I really think we should avoid that as much as we can, which is why I want to spend some time on this PR and play with it, see what can be done. There are internationalization solutions for handlebars out there, I just need to experiment with them.

@xPaw xPaw removed this from the 3.0.0 milestone Jun 20, 2018

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Aug 29, 2018

Closing until Vue is more fleshed out and implemented. Will probably need to be totally rewritten at that point

@MaxLeiter MaxLeiter closed this Aug 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment