patreon link shouldn't open in the main window

[Docteh]

Looks like every other external link has target="_blank" set.

I also noticed that Korean and Hrvatski (Croatian) translations have translated "Patreon"

ko: Patron
hr: Patreona

[McGiverGim]

If the Patreon word and URL must not be translated is better to take it out of the translations file. Some user can translate them without seeing the description and we will not be aware of that if we no look specifically at that element.
I think is better to have them in the js file and pass them as parameter to the original phrase.

[McGiverGim]

@mikeller what do you think? I have seen that you approved it, so I'm not sure if my suggestion is the right way.

[Docteh]

My knowledge of the localization stuff is quite limited, I saw that the string was tagged via an html tag, instead of the usual javascript call, and assumed I had to do everything within the json file.

betaflight-configurator/src/tabs/landing.html

Line 32 in 0b035c1

<div i18n="defaultDonateBottom"></div>

[McGiverGim]

True, in this case we don't have the possibility to pass parameters 😭. We can:

• Modify the i18n code to support it
• Move the translation from html to js
• Let as is

[Docteh]

I guess if we could add the resource before the i18n stuff fires off?

betaflight-configurator/src/js/LogoManager.js

Lines 102 to 106 in 0b035c1

	// inject logo size variables for dynamic translation strings
	i18n.addResources({
	logoWidthPx: "" + this.constraints.imageSize.expectedWidth,
	logoHeightPx: "" + this.constraints.imageSize.expectedHeight,
	});

betaflight-configurator/locales/en/messages.json

Lines 3751 to 3753 in 0b035c1

	"osdSetupCustomLogoInfoImageSize": {
	"message": "Size must be $t(logoWidthPx)×$t(logoHeightPx) pixels"
	},

[mikeller]

I'm not saying it is going to break anything - I am just not sure that this is a good way to solve this by adding complexity.

[McGiverGim]

What I understood in your comment, is that splitting the file into two, needs to change the way that we use to include translations/constants in the HTML/JS. That's not true. The i18next localization that we use will search in both files in a transparent manner for the user if configured correctly. So the developer will use the same instructions/code than before, not matter if it is a translation or a constant, so it does not increase the complexity in any way.

The only drawback, is that the developer must be aware that there are two files, and include the messages in the correct one when adding. Nothing more.

[mikeller]

That still adds complexity over what we currently have - my suggestion is to keep non-translated and translated strings in the same file, as we currently do, and use some form of indicator to mark not-to-be-translated strings as such, and make it part of the CI build job (or potentially even add hooks to crowdin) to check that these constraints are not violated.

[mikeller]

Mergeing this, we can still finish the discussion.

[McGiverGim]

I have hidden the Patreon link string in Crowdin, but this is not a perfect solution. The users can show the hidden strings (all the languages strings were hidden and we now some users ended translating some of them).

[McGiverGim]

About the discussion, I think is better to have two different files for several reasons:

• Is more visible and clear. The developers will se clearly the two files and will understand by the name of the file that one is for constants and the other not.
• If we add some kind of mark/suffix to the name of the translations, we have more than one thousand of translations (and growing), and only about twenty are constants at this moment, so maybe they will not see any of them while trying to contribute and will ignore they can make some of them constants with a little modification.
• Another benefit of having two files is that directly the translator will not see in any way the phrases that they don't need to translate. They will not be uploaded to Crowdin.
• The descriptions in Crowdin are in a field that can be hidden by the user, and I don't know why maybe this is the reason because some users maybe don't read it at all.
• In Crowdin I can't see a better way to block a translation. I thought that hidden them will be enough, but is clear that for some users is not.
• Adding a control to to the CI to me is late. We will need to search each phrase in Crowdin, delete it, and generate the languages files again. Is more work for us.
• The other solution, add some suffix to the name of the phrase, and modify the localization.js to ignore them, will work, but is a little inconsistent to me. We will have translations in the files that we will ignore. This can lead too to some kind of confusion for the translators, that will see in Crowdin the phrase translated but not in the Configurator.

This are my thoughts, but I will accept the final decision taken ;)

[mikeller]

@McGiverGim:

Is more visible and clear. The developers will se clearly the two files and will understand by the name of the file that one is for constants and the other not.

I disagree. A lot of our contributors are part-time contributors. Even the fact that user-visible strings have to be extracted into a file is something that needs to be told to a lot of them. As it is, how this is done is pretty much self-explanatory. If we want them to do triage of their strings, and then sort them into the correct list, then there will be a lot more explaining to do.

and only about twenty are constants at this moment

Makes me wonder if we aren't overthinking this a bit - maybe we should just stick them into a stringConstants or similar hash table, and then extend the translations file with them on the fly when loaded, before using it?

[McGiverGim]

Makes me wonder if we aren't overthinking this a bit - maybe we should just stick them into a stringConstants or similar hash table, and then extend the translations file with them on the fly when loaded, before using it?

Maybe. The problem with this is that we have very few constants because the major part of the strings are older than the translation system, and we haven't a way to use parameters in the HTML i18n tags. But to my understanding, all the URLs for example must go out of the translations file. We are letting the translators modify them pointing to any other place.

If we use a hash it will grow in size in the time when we start to use it. Is not a bad solution, but if we do it in this way to me the array must go in it's own file to let it grow, and I can't see the benefit then comparing to a new file with constant translations.

This is not a critical need, the code is working, but we started the discussion with this PR and I always overthink when we are talking about code refactoring 😄

[mikeller]

In a way I see things a bit differently - I guess I am just more trusty for translators to do their best to do good work and not break things, so to me it is not so much the case that things must be moved out of the translations file, more that they can, in order to help translators by making it less confusing.

Maybe part of this is also experience with translations in other, bigger projects. What I've learned there is that, no matter how much effort is made, translations always end up being 'messy' to some extent - languages are just not designed to be translated in rule based ways.

With respect to URLs, this is one thing where I'd be careful about removing all of them from the translations files - for some of them there might be localised pages available (now or in the future) at different URLs, and in the interest of supporting non-English-speaking users, translators might want to substitute the URL of the English version with one for a localised version.

[McGiverGim]

True, sometimes the URLs must be localized, but to me the URLs must be out of the initial phrase. If we don't want to have localized versions, they can go into the constants file/hash. If we want to translate them, then we can let it as independent phrase in the translations file.

I speak as Spanish translators. The texts to translate are bigger and is difficult to translate them when we have a long URL in the middle, with the HTML tags, etc. Is easier to have a marker to the link like in this PR.

The welcome texts of Betaflight have been a nightmare for the translators, they are very long, with a lot of URLs and from time to time they change and we need to translate them again.

[Docteh]

I should have put the example in front of the first call to i18n.localizePage(); but I wasn't paying attention. I think inside the startProcess function is the best place to load constants into the i18n thing, hopefully not involving $.getJSON from jQuery...

Just a matter of where and how

diff --git a/locales/en/messages.json b/locales/en/messages.json
index a0bf2d16..9e2f8b94 100644
--- a/locales/en/messages.json
+++ b/locales/en/messages.json
@@ -594,10 +594,6 @@
     "defaultDonateBottom": {
         "message": "<p>If you want to contribute financially on an ongoing basis, you should consider becoming a patron for us on $
t(patreonLink.message).</p>"
     },
-    "patreonLink": {
-        "message": "<a href=\"https://www.patreon.com/betaflight\" target=\"_blank\">Patreon</a>",
-        "description": "Patreon is name, and should not require translation"
-    },
     "defaultDonate": {
         "message": "Donate"
     },
diff --git a/src/js/main.js b/src/js/main.js
index 9216ccba..cc266613 100644
--- a/src/js/main.js
+++ b/src/js/main.js
@@ -105,6 +105,12 @@ function setupAnalytics(result) {
 function startProcess() {
     // translate to user-selected language
     i18n.localizePage();
+    i18n.addResources({
+        "patreonLink": {
+            "message": "<a href=\"https://www.patreon.com/betaflight\" target=\"_blank\">Patreon</a>",
+            "description": "Patreon is name, and should not require translation"
+        }
+    });

     // alternative - window.navigator.appVersion.match(/Chrome\/([0-9.]*)/)[1];
     GUI.log(i18n.getMessage('infoVersions',{operatingSystem: GUI.operating_system,

We could ditch the `.message` part

diff --git a/locales/en/messages.json b/locales/en/messages.json
index a0bf2d16..a4f9f9ea 100644
--- a/locales/en/messages.json
+++ b/locales/en/messages.json
@@ -592,11 +592,7 @@
         "message": "<p><strong>Betaflight</strong> is a flight controller software that is <strong>open source</strong> and is avai
lable free of charge <strong>without warranty</strong> to all users.</p><p>If you found the Betaflight or Betaflight configurator us
eful, please consider <strong>supporting</strong> its development by donating.</p>"
     },
     "defaultDonateBottom": {
-        "message": "<p>If you want to contribute financially on an ongoing basis, you should consider becoming a patron for us
on $t(patreonLink.message).</p>"
-    },
-    "patreonLink": {
-        "message": "<a href=\"https://www.patreon.com/betaflight\" target=\"_blank\">Patreon</a>",
-        "description": "Patreon is name, and should not require translation"
+        "message": "<p>If you want to contribute financially on an ongoing basis, you should consider becoming a patron f
or us on $t(patreonLink).</p>"
     },
     "defaultDonate": {
         "message": "Donate"
diff --git a/src/js/main.js b/src/js/main.js
index 9216ccba..d5ae2732 100644
--- a/src/js/main.js
+++ b/src/js/main.js
@@ -105,6 +105,9 @@ function setupAnalytics(result) {
 function startProcess() {
     // translate to user-selected language
     i18n.localizePage();
+    i18n.addResources({
+        "patreonLink": "<a href=\"https://www.patreon.com/betaflight\" target=\"_blank\">Patreon</a>"
+    });

     // alternative - window.navigator.appVersion.match(/Chrome\/([0-9.]*)/)[1];
     GUI.log(i18n.getMessage('infoVersions',{operatingSystem: GUI.operating_system,