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

Deferred translation class with optional context for option/effect/item-action names #26402

Merged
merged 27 commits into from
Nov 10, 2018

Conversation

Qrox
Copy link
Contributor

@Qrox Qrox commented Oct 25, 2018

Summary

SUMMARY: I18N "Deferred translation class with optional context for option/effect/item-action names"

Purpose of change

There are several places in both cpp and json code where a word with multiple meanings is used without translation context. This makes it hard to translate the given strings in some other languages. This PR adds the ability to specify a translation context in these cases.

Closes #21760.

Describe the solution

  1. A translation class is created which supports an optional translation context and deserialization from JSON;
  2. Code for options, effects, and item actions is adjusted to use the new translation class, and JSON strings that needed a context are updated to include one.
  3. Keywords for the xgettext call in update_pot.sh is updated so it also searches for the constructor of the translation class;
  4. The python scripts used for extracting json strings are updated so they work properly on Windows;
  5. The python scripts are updated to extract json strings with context;
  6. merge_po.sh is updated to disable fuzzy matching and remove obsolete strings; (I'm not sure about this one. The .po files in the repo does not have fuzzy matching nor obsolete strings, but maybe they are not removed in the script for a reason? @BrettDong should merge_po.sh be kept as is?)
  7. Updated documentations to reflect the new translation class and JSON syntax;
  8. Also fixed excess spaces in language names.

Additional context

I need comments on the changes to the shell and python scripts, as I'm not very familiar with the languages.

The following are some screenshots showing the changes with this PR. I've marked relevant text with English so it's easier to see what is changed.

Before:

Item action menu:
image
The translations for "play - handheld game machine" and "smoke - cigar" do not make sense here.

Options menu:
image
According to the translation, the option is "physically disabled".

Character info UI:
image
These translations are correct.

After (after generating, editing, and compiling .po files):

Item action menu:
image

Options menu:
image

Character info UI:
image

src/effect.cpp Outdated
new_etype.name.push_back( jsarr.next_string() );
translation name;
if( !jsarr.read_next( name ) ) {
debugmsg( "Error reading effect names" );
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to call jsarr.throw_error to generate an exception along with a proper JSON context.

@@ -19,6 +19,10 @@
#define translate_marker_context(c, x) x
#endif

#include <string>
#include "json.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't include "json.h" here, it's quite large and has more dependencies. You don't need to derive from JsonDeserializer, having a deserialze function with proper signature is enough.

@BevapDin
Copy link
Contributor

Have you considered to adding a translation::operator std::string() const { return translated(); } function? This would implicitly convert translation objects to std::string (applying the translation).

This means you don't need to add translated() explicitly so often.

@AMurkin
Copy link
Contributor

AMurkin commented Oct 25, 2018

Will close #24102?

@Qrox
Copy link
Contributor Author

Qrox commented Oct 26, 2018

@AMurkin Currently keybindings are not using the translation class, but that's definitely doable. On another hand this should close #21760 you referenced in that issue.

@BrettDong
Copy link
Member

merge_po.sh is a pre-Transifex era thing. The script is actually not used anymore.

@Qrox
Copy link
Contributor Author

Qrox commented Oct 26, 2018

@BrettDong I'll adapt the script as a tool for locally testing translations then.

@Qrox
Copy link
Contributor Author

Qrox commented Oct 26, 2018

Have you considered to adding a translation::operator std::string() const { return translated(); } function?

I'm not sure about it. It could of course make the code less verbose, but I'm concerned that if someone wants to use the class in more places, and happens to forget to change the variable from std::string to translation, then the string may get translated prematurely. I guess it's better to keep it explicit.

@ZhilkinSerg ZhilkinSerg added [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Translation I18n labels Oct 26, 2018
@Qrox
Copy link
Contributor Author

Qrox commented Oct 26, 2018

@AMurkin Actually the strings you mentioned in #24102 are fixable by simply using pgettext, so I'm not going to address it in this PR.

@jbytheway
Copy link
Contributor

I see you've changed the Python scripts to use some Python 3-isms, rather than Python 2.

This is not really about this PR, but the #!/usr/bin/env python lines should really be changed to specify either python2 or python3 specifically, because just python can end up using either one and makes it difficult to have the script work everywhere (which leads to things like you having to make these changes).

@Qrox
Copy link
Contributor Author

Qrox commented Oct 27, 2018

Thanks, I'll change them to python3 since extract_json_strings.py already uses python3.

@Qrox Qrox changed the title [WIP][CR] Deferred translation class with optional context for option/effect/item-action names Deferred translation class with optional context for option/effect/item-action names Oct 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Translation I18n
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disambiguation of "Smoke"
7 participants