Refactor translations #1608

pafcu · 2017-11-13T19:38:51Z

Get rid of _tJsx and offer all features in the same function:

Plurals
Replacement with React
%(foo)s AND <a>foo</a> replacements mixed together

Also, no need anymore to play around with regular expressions.

Before:	Now:
`_t('Hello %(var)s', { var: 'foo' })`	Same
`_tJsx('sent to %(emailAddress)s', /%$emailAddress$s/, (sub) => <i>{emailAddress}</i>)`	`_t('sent to %(emailAddress)s', { emailAddress: (sub) => <i>{emailAddress}</i> })`
`_tJsx('Click <a>here</a>', [/<a>(.*?)<\/a>/], [(sub) => <a>{ sub }</a>]`)	`_t('Click <a>here</a>', {}, {'a': (sub) => <a>{ sub }</a>`)
`_tJsx('Click <a>here</a> or <a>there</a>', [/<a>(.?)<\/a>/,/<a>(.?)<\/a>/], [(sub) => <a>{ sub }</a>,(sub) => <a>{ sub }</a>]`)	`_t('Click <hereLink>here</hereLink> or <thereLink>there</thereLink>', {}, {'hereLink': (sub) => <a>{ sub }</a>,'thereLink': (sub) => <a>{ sub }</a>}`)

If this looks OK I'll update riot-web as well, and update the documentation.

lukebarnard1

Largely coherent, yey for refactoring things 👍 😄

lukebarnard1 · 2017-11-14T15:54:20Z

src/components/views/dialogs/SetMxIdDialog.js

-                            ],
+                            {},
+                            {
+                                'span': () => <span>{ this.props.homeserverUrl }</span>,


This looks like it can be replaced with a normal substitution,

Good catch, but I think it might be better to save for later. Changing it to a normal variable would require changing it in every translation as well. which would make this PR balloon even more (or force every translator to re-translate the string).

Fair enough

lukebarnard1 · 2017-11-14T16:02:45Z

src/components/views/messages/SenderProfile.js

    }

    return (
        <div className="mx_SenderProfile" dir="auto" onClick={props.onClick}>
-            { content }
+            { content.props.children[0] ?


This is a little bit sad, especially because we're only doing this so that Flair can appear with a different opacity to the sender profile text and yet we don't show Flair (currently) when the aux text is actually used.

One way to do this is to not use opacity as a cheap way to get the text to be the colour we want. This feels out-of-scope, but maybe leave a "// XXX:" comment explaining this.

lukebarnard1 · 2017-11-14T16:06:51Z

src/languageHandler.js

-// instance.
-export function _t(...args) {
+// Wrapper for counterpart's translation function so that it handles nulls and undefineds properly
+//Takes the same arguments as counterpart.translate()


missing space at start of comment

lukebarnard1 · 2017-11-14T16:08:54Z

src/languageHandler.js

+ * @return a React <span> component if any non-strings were used in substitutions, otherwise a string
+ */
+export function _t(text, variables, tags) {
+    // Don't do subsitutions in counterpart. We hanle it ourselves so we can replace with React components


lukebarnard1 · 2017-11-14T16:29:06Z

src/languageHandler.js

-        if (!match) {
-            throw new Error(`_tJsx: translator error. expected translation to match regexp: ${patterns[i]}`);
+        const match = inputText.match(regexp);
+        if(!match) {


Generally, the style is if (

lukebarnard1 · 2017-11-14T16:36:28Z

src/languageHandler.js

+        const match = inputText.match(regexp);
+        if(!match) {
+            output.push(inputText); // Push back input
+            continue; // Missing matches is entirely possible, because translation might change things


I thought the point was that the translations should contain the things to be replaced? Should we not still be throwing an error here?

This is not true. You can pass in e.g. count, but not display it.

If count is the exception, then maybe we should handle it as one and treat missing matches as errors?

It's also possible for others, but probably less common. E.g. in the "plural branch" you might display other variables than in the "singular branch".

Might make sense to log a warning though?

Warning would be fine, but probably want to ignore missing count because otherwise we log pointlessly. In doing so we might lose some warnings we do want for plural branches, as you've described, but that feels like an okay tradeoff.

lukebarnard1 · 2017-11-14T16:44:11Z

src/languageHandler.js

+
+        output.push(toPush);
+
+        // Check if we need to wrap the output into a span at the end


I would change this comment to be along the lines of "wrap the output in a React component if any items are React components"

lukebarnard1 · 2017-11-14T16:48:16Z

test/i18n-test/languageHandler-test.js

+    it('variable substitution with React component', function() {
+        // Need an extra space at the end because the result of _t() has an extra empty node at the end
+        const text = 'You are now ignoring %(userId)s ';
+        expect(JSON.stringify(languageHandler._t(text, { userId: () => <i>foo</i> })))


What's the reason for JSON.stringify? This should probably be commented.

Is there a better way to compare objects by content rather than identity?

expect supports toEqual: https://github.com/mjackson/expect#toequal, which can be used in place of toBe (which uses ===).

lukebarnard1 · 2017-11-14T16:48:32Z

test/i18n-test/languageHandler-test.js

+    });
+
+    it('variable substitution with React component', function() {
+        // Need an extra space at the end because the result of _t() has an extra empty node at the end


Is there a way for it not to have an extra empty node? 😇

lukebarnard1 · 2017-11-14T16:53:32Z

src/languageHandler.js

+ */
+export function _t(text, variables, tags) {
+    // Don't do subsitutions in counterpart. We hanle it ourselves so we can replace with React components
+    const args = Object.assign({ interpolate: false }, variables);


So why is variables passed in here?

dbkr

A few comments (and ftr, have been chatting on riot dev too (https://matrix.to/#/!DdJkzRliezrwpNebLk:matrix.org/$1510674234381wUoHD:ldbco.de) about things like doing our own sprintf-ing rather than using counterparts, but I think this is probably fine.

Thanks for this, I appreciate it's a bit of a mission!

dbkr · 2017-11-14T16:34:25Z

src/languageHandler.js

+
+    let wrap = false; // Remember if the output needs to be wrapped later
+    for (const regexpString in mapping) {
+        const regexp = new RegExp(regexpString);


We might find we need to look at caching these regexes, but I guess premature optimisation...

I had the same thought. I concluded that seemingly our major perf issues are elsewhere, but could be wrong.

dbkr · 2017-11-14T16:41:25Z

src/components/views/login/InteractiveAuthEntryComponents.js

@@ -370,7 +373,10 @@ export const MsisdnAuthEntry = React.createClass({
            });
            return (
                <div>
-                    <p>{ _tJsx("A text message has been sent to %(msisdn)s", /%\(msisdn\)s/, (sub) => <i>{this._msisdn}</i>) }</p>
+                    <p>{ _t("A text message has been sent to %(msisdn)s",
+                        { msisdn: () => <i>this._msisdn</i> },


Can this not just be the value rather than a function?

dbkr · 2017-11-14T16:43:43Z

src/components/views/rooms/RoomList.js

+                        {},
+                        {
+                            'CreateRoomButton': () => <CreateRoomButton size="16" callout={true} />,
+                            'RoomDirectoryButton': () => <RoomDirectoryButton size="16" callout={true} />,


More bits that don't need to be functions?

dbkr · 2017-11-14T16:47:23Z

src/languageHandler.js

 *
- * @param {string} jsxText The untranslated stringified JSX e.g "click <a href=''>here</a> now".
- * This will be translated by passing the string through to _t(...)
+ * The values to substitute with can be either simple strings, or functions that return the value to use in


Looking at the code, looks like they could probably be react components too? So you could just pass them in rather than functions that return them if it's a node that doesn't contain text? In fact, presumably you could even do these by passing it into vars rather than tags?

dbkr · 2017-11-14T16:53:32Z

src/languageHandler.js

+ * Translates text and optionally also replaces XML-ish elements in the text with e.g. React components
+ * @param {string} text The untranslated text, e.g "click <a>here</a> now to %(foo)s".
+ * @param {object} variables Variable substitutions, e.g { foo: 'bar' }
+ * @param {object} tags Tag substitutions e.g. { 'a': (sub) => <a>{sub}</a> }


Might be worth spelling out when one would use vars vs tags here (ie. tags is necessary whenever you need i18ned text inside an HTML tag like <a> or <i>?) With _tJsx you also needed to use _tJsx if you wanted any custom markup in the i18ned text (eg. an img) but since your new _t uses the same code paths for both, presumably tags like <img> could equally be done with a regular vars substitution?

Signed-off-by: Stefan Parviainen <pafcu@iki.fi>

lukebarnard1

Tiny thing, but LGTM otherwise 😄

lukebarnard1 · 2017-11-16T11:07:01Z

src/languageHandler.js

+
+        let replaced;
+        // If substitution is a function, call it
+        if(mapping[regexpString] instanceof Function) {


The style we use is if (, not sure why we haven't added this to the linting.

pafcu added 4 commits November 13, 2017 20:19

Refactor i18n stuff a bit

3afbaf6

Validate tag replacements in gen-i18n

9cf7e1b

Add unit tests for translation

672d508

Make eslint happy

2acd42e

lukebarnard1 self-assigned this Nov 14, 2017

lukebarnard1 suggested changes Nov 14, 2017

View reviewed changes

dbkr reviewed Nov 14, 2017

View reviewed changes

pafcu added 6 commits November 14, 2017 19:33

Dont't add empty nodes

772550a

Use toEqual instead of toBe

cdd03dd

Clarifications

788be67

Pass plain components, rather than functions returning them

df6d5cc

Add note about alternative to opacity

f8660de

Log if no match is found

ae252f7

pafcu mentioned this pull request Nov 15, 2017

Refactor translations element-hq/element-web#5613

Merged

Fix conflict and update strings

2c1618b

Signed-off-by: Stefan Parviainen <pafcu@iki.fi>

pafcu changed the title ~~[WIP] Refactor translations~~ Refactor translations Nov 15, 2017

lukebarnard1 suggested changes Nov 16, 2017

View reviewed changes

Add space after if

342378f

lukebarnard1 approved these changes Nov 16, 2017

View reviewed changes

lukebarnard1 merged commit 29e6627 into matrix-org:develop Nov 16, 2017

pafcu deleted the simplify-translation branch November 29, 2017 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor translations #1608

Refactor translations #1608

pafcu commented Nov 13, 2017 •

edited

Loading

lukebarnard1 left a comment

lukebarnard1 Nov 14, 2017

pafcu Nov 14, 2017

lukebarnard1 Nov 14, 2017

lukebarnard1 Nov 14, 2017

lukebarnard1 Nov 14, 2017

lukebarnard1 Nov 14, 2017

lukebarnard1 Nov 14, 2017

lukebarnard1 Nov 14, 2017

pafcu Nov 14, 2017

lukebarnard1 Nov 14, 2017

pafcu Nov 14, 2017 •

edited

Loading

lukebarnard1 Nov 14, 2017

lukebarnard1 Nov 14, 2017

lukebarnard1 Nov 14, 2017

pafcu Nov 14, 2017

lukebarnard1 Nov 14, 2017

lukebarnard1 Nov 14, 2017

lukebarnard1 Nov 14, 2017

dbkr left a comment

dbkr Nov 14, 2017

lukebarnard1 Nov 14, 2017

dbkr Nov 14, 2017

dbkr Nov 14, 2017

dbkr Nov 14, 2017

dbkr Nov 14, 2017

lukebarnard1 left a comment

lukebarnard1 Nov 16, 2017

Before:	Now:
`_t('Hello %(var)s', { var: 'foo' })`	Same
`_tJsx('sent to %(emailAddress)s', /%\(emailAddress\)s/, (sub) => <i>{emailAddress}</i>)`	`_t('sent to %(emailAddress)s', { emailAddress: (sub) => <i>{emailAddress}</i> })`
`_tJsx('Click <a>here</a>', [/<a>(.*?)<\/a>/], [(sub) => <a>{ sub }</a>]`)	`_t('Click <a>here</a>', {}, {'a': (sub) => <a>{ sub }</a>`)
`_tJsx('Click <a>here</a> or <a>there</a>', [/<a>(.?)<\/a>/,/<a>(.?)<\/a>/], [(sub) => <a>{ sub }</a>,(sub) => <a>{ sub }</a>]`)	`_t('Click <hereLink>here</hereLink> or <thereLink>there</thereLink>', {}, {'hereLink': (sub) => <a>{ sub }</a>,'thereLink': (sub) => <a>{ sub }</a>}`)


		output.push(toPush);

		// Check if we need to wrap the output into a span at the end

Refactor translations #1608

Refactor translations #1608

Conversation

pafcu commented Nov 13, 2017 • edited Loading

lukebarnard1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pafcu Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pafcu commented Nov 13, 2017 •

edited

Loading

pafcu Nov 14, 2017 •

edited

Loading