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

Translation Enablement - Phase 1A #7247

Closed
wants to merge 5 commits into from
Closed

Conversation

srl295
Copy link
Contributor

@srl295 srl295 commented May 19, 2016

  • documented in TRANSLATION.md
  • using angular-translate
  • English strings in src/plugins/kibana/public/settings/sections/indices/i18n/en.json

Fixes: #7255

Author: Scott Russell scott_russell@us.ibm.com @DTownSMR
Author: Martin Hickey martin.hickey@ie.ibm.com @hickeyma
Author: Steven R. Loomis srloomis@us.ibm.com @srl295

@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

@srl295
Copy link
Contributor Author

srl295 commented May 19, 2016

/cc and thanks @shikhasriva

@srl295
Copy link
Contributor Author

srl295 commented May 19, 2016

  • At this point, there isn't fallback language support. So if your browser requests es and it's not installed, you get raw strings.

@srl295
Copy link
Contributor Author

srl295 commented May 19, 2016

  • failures — not quite sure what they mean. Can someone help explain?

@DTownSMR
Copy link

Steven, wasn't Martin's change included that supported fallback to English?

@srl295
Copy link
Contributor Author

srl295 commented May 19, 2016

@DTownSMR yes - but it required a hard coded list of locales. Looking at how to avoid that.

@Bargs
Copy link
Contributor

Bargs commented May 20, 2016

The test that's failing is one of our browser based unit tests. You can run this locally with npm run test:dev which will launch a karma instance and a browser that you can use devtools in to debug the test. Haven't had a chance to look at it closely myself but I didn't see an obvious connection between the failing test and the changes here, so I'd be interested to know if the same test fails for you locally.

@Bargs Bargs mentioned this pull request May 20, 2016
@hickeyma
Copy link
Contributor

hickeyma commented May 20, 2016

@Bargs: Thanks for the tip. I ran the unit tests as you suggested and one of the tests is failing. It seems to be "debounce service -> cancel". I have added the error output as follows: http://dpaste.com/3PHSWRC

* documented in TRANSLATION.md
* using angular-translate
* English strings in src/plugins/kibana/public/settings/sections/indices/i18n/en.json

Fixes: elastic#7255

Author: Scott Russell <scott_russell@us.ibm.com>
Author: Martin Hickey <martin.hickey@ie.ibm.com>
Author: Steven R. Loomis <srloomis@us.ibm.com>
* Otherwise, the '$http GET' to fetch translation resources
shows up as a deferred task here.

* May have fixed elastic#5446
@srl295
Copy link
Contributor Author

srl295 commented May 20, 2016

Debounce failures fixed in 5d9959c (see #5446 )

@Bargs
Copy link
Contributor

Bargs commented May 20, 2016

jenkins, test it

@srl295
Copy link
Contributor Author

srl295 commented May 20, 2016

^ 2337/2337 passed, but the server exitted with failure.

@rashidkpc
Copy link
Contributor

jenkins, test it

@srl295
Copy link
Contributor Author

srl295 commented May 27, 2016

@rashidkpc @Bargs thanks- what's next?

@Bargs
Copy link
Contributor

Bargs commented May 27, 2016

@srl295 is the code ready for review?

@srl295
Copy link
Contributor Author

srl295 commented May 27, 2016

@Bargs I think the core code and approach is ready. But actually, I think the fallback support (noted above) should be in. @hickeyma is working on some tests as well.

  • tests
  • replace 'TODO' in docs


For this example, we will demonstrate translation into Maltese (Language code `mt`).

- Check out the `kibana` source code with `git`,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should instruct the user to fork the Kibana repo rather than just checking it out.

Copy link
Contributor Author

@srl295 srl295 May 28, 2016

Choose a reason for hiding this comment

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

Good point.

  • fork, not checkout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

docs updated

@Bargs
Copy link
Contributor

Bargs commented May 27, 2016

@srl295 I started looking at the code a bit today, I'll try to dig in more on Tuesday next week and provide some more feedback.

@Bargs Bargs self-assigned this May 27, 2016
@epixa
Copy link
Contributor

epixa commented May 27, 2016

I haven't given this a thorough review, but I do have one immediate concern: by translating the template strings via directives/filters during runtime rather than by a consistent process at build time, we're limiting translation support to only angular templates, and we make it much harder to develop tooling around the translation system.

We don't have a plan to move away from angular at the moment, but we definitely don't plan to be on angular 1.x forever. As for tooling, I think a bare minimum for a translation system would be the ability to programmatically find missing translations in a language file, which Kibana core CI builds would outright fail on if there were any missing english translations. I'm not sure how reliable that would be when translations are applied at runtime.

@srl295
Copy link
Contributor Author

srl295 commented May 28, 2016

@epixa Thanks for the feedback. Please see in #6515, we aren’t tying this solution to Angular in any way. The files are just JSON files, and we show examples of non-Angular components using them. We don't have any of these in the PR for this particular phase.

As for tooling, I think a bare minimum for a translation system would be the ability to programmatically find missing translations in a language file, which Kibana core CI builds would outright fail on if there were any missing english translations. I'm not sure how reliable that would be when translations are applied at runtime.

@hickeyma is working on exactly that- automated tooling to compare the translation file (e.g. de.json) to make sure it includes everything that is in the source file (e.g. en.json).

@epixa
Copy link
Contributor

epixa commented May 28, 2016

Glad to hear about not being coupled to angular! I should have reviewed the contents of the PR more thoroughly, but I really wanted to get you some initial feedback before I headed off yesterday.

My concern with tooling wasn't so much about ensuring that other languages had the same translations as en.json so much as identifying the scenario where a word identifier is used somewhere in a template and there is no corresponding translation in en.json. This could happen due to a variety of different mistakes, including a simple typo. For example:

// en.json
{
  "GREETING": "Hello"
}

// someview.html
<span translate>GREETIGN</span>

It's imperative that that scenario cause an error at build time so that it wouldn't even pass CI.

@srl295
Copy link
Contributor Author

srl295 commented May 28, 2016 via email

@hickeyma
Copy link
Contributor

@epixa Thanks for the feedback. I am wondering if testing of the identifiers in the code (JS, HTML etc.) comes under the current testing of Kibana? My thinking is that adding globalization support does not alter the functionality of the product, it added the identifiers and string substitutions for each language supported. If an identifier is therefore incorrectly added, should this not be picked up by current testing as the string will not be returned?

@epixa
Copy link
Contributor

epixa commented May 31, 2016

From a practical perspective, I don't think relying on the existing tests is going to be feasible. Getting complete test coverage on Kibana is admirable, but there are practical and technical obstacles to overcome to make that happen, so it's a long-term goal at this point. It could be years before that happens, and I'd love to get i18n support before then. Our approach for testing at this point is to continue increasing test coverage on existing code where possible and to ensure that any new functionality being added is done so in a way that is itself testable, and that would apply to something like translations as well.

Stray identifiers in the UI might not technically impact functionality in the UI, but they have a negative impact on users in a much different, but no less important way. The ambiguity in a stray identifier could make a user uncomfortable using an entire feature, or worse it could make users misunderstand what the feature is suppose to do. We're talking about the possibility of adding delete data functionality in the upcoming pipeline work - for stuff like this, it is absolutely imperative that a missing identifier doesn't make that feature any less clear.

But beyond all of that, I think it's important that we can test the translation system itself without running functional tests on Kibana.

- mention 'fork' rather than 'check out'
- remove TODOs
@srl295
Copy link
Contributor Author

srl295 commented Jun 1, 2016

@epixa @Bargs should testing be in this PR or in a subsequent one?

@srl295
Copy link
Contributor Author

srl295 commented Jun 1, 2016

OK Kibana people… I thought I had an idea for how this could work, but now I'm not sure.
Problem: Need to figure out which languages are installed, at some start-up time (build, optimize, serve…) to know what is available and when to fall back to the source language (English).
As this PR stands, if you don't have one of the supported languages chosen (i.e. your browser doesn't request English explicitly), you won't get the correct content.
Example: if you have English and German installed, the list will be [ 'en', 'de' ]. If you just have the English content, it will be [ 'en' ] ,etc. Probably this output would be a small .js file just exporting the array:

module.exports = ['en'];

Options:

  1. Have users manually edit an installedLanguages.json file with a static list. (Not recommended, but wanted to list here for completeness.)
  2. Have users run some task (npm run update-languages) whenever they add/remove .json files to detect the list of languages available.
  3. Run code inside of the src/optimize phase to scan for i18n/*.json files at optimize time. This might be good, because then at optimize time we can also build optimal language bundles ( planned as a later enhancement- see Kibana Globalization #6515 under Phase 1AB )
  4. Run something under the src/cli or src/plugins mechanisms that would run early and do the scan/update.

Any thoughts about the preferred approach?

@Bargs
Copy link
Contributor

Bargs commented Jun 1, 2016

Testing should definitely be in this PR if it's going to go into master. Really I'd like to see all of 1A(X) (as well as example usage inside and outside of angular) working since building, testing, and in app usage all impact each other. It seems like we're moving in a good direction, but this feature touches so many different things its hard to predict all of the ramifications without seeing at least a POC with all the moving pieces.

I don't have a good solution for creating the supported languages list yet. Optimize seems like a good place, except that the tests will also need to know which languages to test, and ideally they should run without needing the optimizer. A related question: the main GH issue says "Kibana users could "plug in" translation (language) bundles". How will this work?

@srl295
Copy link
Contributor Author

srl295 commented Jun 1, 2016

all of 1A(X)
Thanks, that's helpful.

[tests] should run without needing the optimizer
The tests can run the same file-discovery algorithm to locate the files.

Kibana users could "plug in" translation (language) bundles. How will this work?

As this PR stands now, to add German they can just add the appropriate files. That's why the discovery phase mentioned here is needed.

src/plugins/kibana/public/settings/sections/indices/i18n/de.json
…

@epixa
Copy link
Contributor

epixa commented Jun 1, 2016

I'd like to see the supported languages feature implemented through the plugin system. This would mean that installing a new language means installing a new plugin. That would give us all the tracking capabilities we need for what languages are available, it would allow us to make decisions about languages during Kibana start up, etc.

bin/kibana-plugin install i18n-de

@srl295
Copy link
Contributor Author

srl295 commented Jun 1, 2016

@epixa should (could) the built in English content also be a plugin? Is there any documentation on plugins besides the "PLEASE DON'T WRITE CUSTOM PLUGINS" message?

@hickeyma
Copy link
Contributor

hickeyma commented Jun 1, 2016

@epixa Thanks for feedback on testing of stray identifiers. It makes sense that the user usability should not be broken. I agree.

Potential test case could be: Loading the list of translation identifiers per localization directory and comparing with the code files (HTML and JS) for that directory for the existence of the identifiers. If identifier is not present then we have a stale or stray identifier.

@Bargs
Copy link
Contributor

Bargs commented Jun 1, 2016

@srl295 @hickeyma I realize our feedback is kinda all over the place right now, I apologize for that. Would you be up for a video chat with @epixa and I? A lot of our feedback is driven by our thoughts on the future of Kibana's architecture, so it would be easier to hash some ideas out on a call rather than back and forth on a PR thread. We think this feature is really important so we'd like to give you guys the support you need to implement something really rock solid that'll work well into the future.

I'll try to highlight a few of the high level things we'd like to discuss. Note that none of these future plans are totally concrete at the moment, which is why it's easier to talk about it rather than write about it.

  • Over time we'd like to slim down the Kibana core. Even things like Dashboard and Discover will ideally be implemented as isolated plugins. So the most important first step in implementing globalization is probably creating a solid, framework agnostic service that all Kibana plugins can consume and contribute to.
  • Angular should probably consume this service rather than the i18n files directly.
  • Testing and translation checks should work without needing to run/optimize Kibana. We'd like to get rid of the optimization step in production anyways.
  • All translation files should probably be added to the system in the same way, even the default english ones.

Testing covers:
  - Missing locale translation files in a localization directory
  - Missing translation identifiers in locale translation files
  - Missing translations in a locale translation file
The tests assume the english locale (en) in a localization directory is the
reference file for checking.
… are used

Check the translation identifiers in locale file against code files for
that localization directory to check for stray or stale identifiers.
@Bargs Bargs removed their assignment Jun 29, 2016
@epixa
Copy link
Contributor

epixa commented Oct 8, 2016

@srl295 Is this PR still relevant given the other work you've been doing on the i18n support?

@elasticmachine
Copy link
Contributor

Can one of the admins verify this patch?

@srl295
Copy link
Contributor Author

srl295 commented Dec 15, 2016

Superseded by #7545 (which has been merged)

@srl295 srl295 closed this Dec 15, 2016
@srl295 srl295 deleted the phase1a branch December 15, 2016 17:38
@liangrui1988
Copy link

How do you translate it into Chinese? Thank you

@epixa
Copy link
Contributor

epixa commented Jul 17, 2017

@liangrui1988 Kibana isn't translatable yet. This issue represented an internal initial effort to support a future translation feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants