odudex / krux

Open-source, airgapped hardware signer for Bitcoin
https://selfcustody.github.io/krux/
Other
10 stars 2 forks source link

Missed translation entry #22

Closed aglkm closed 1 year ago

aglkm commented 1 year ago

Missed translation for the following message from "src/krux/pages/home.py". No entries in xx-XX.json files.

t( """Warning:\nWallet output descriptor not found.\n\n Some checks cannot be performed.""" )

jdlcdl commented 1 year ago

Nice catch!

I think it's a bug, in that python i18n.py fill doesn't even see this code as needing translation.

Is it black that prettifies "t('some very long string')" onto multiple lines causing problems? Is it the multi-line triple quoted string that i18n.py isn't finding?

Changing the code to a single line, since the newline chars are already there, even after being prettified by black, solves this so that python i18n.py fill will know it needs to be translated.

More importantly, how many more triple-quoted multi-line strings needing translation are in the code base? and is this a convention that is new, or wanted, or maybe something to avoid?

Maybe taking a look at i18n.py is the more correct answer, since the code is valid.

odudex commented 1 year ago

@jdlcdl is right, the line break inserted by black makes it stealth to i18n.py validate and fill. I think a good practice would be avoid translating long sentences with line breaks at once, instead, segment it to shorter translations. Like:

self.ctx.display.draw_centered_text(
    t("Warning:")
    + "\n"
    + t("Wallet output descriptor not found.")
    + "\n\n"
    + t("Some checks cannot be performed.")
)

This way there also would be more translation re-use, as "Warning:" and "Wallet output descriptor not found." occur more than once. What do you think?

tadeubas commented 1 year ago

Totally agree @odudex

jdlcdl commented 1 year ago

I'm from the camp that thinks of these translation slugs as keys into a dictionary. All the dictionaries have the same keys but their values are translated for different users. But what these slugs are NOT, at least for me, are messages directed at the user (even if that's how many do it), so that code can be separated from how it's presented.

An extreme example in this case might be: "WARN:pages.home.sign_psbt:multisig_without_descriptor" and then the code would never again be changed. Instead, we'd update the english translation files and all others. This way, messaging can change without ever affecting the codebase. Translators would work from the translation files that they understand... and might need a tool to join on message slug keys.

A draw back would be that it would be hard to chase down user reported bugs... would have to go to translation files to find the key and into the code to see where that message came from.

If instead you split strings like this into different slugs, the translators will be all over the place in a sorted translation file trying to find how a message is composed. "Warning:" in one context might not be the same in another but they'll all translate the same.

Until we're sure. I sort of like:

t(
   "Warning:\nWallet output descriptor not found.\n\nSome checks cannot be performed."
)
jdlcdl commented 1 year ago

But I also agree with anything that serves "useful" so that translators can keep pushing forward!

Whatever makes it work so that the slugs end up in translation files is progress.

jdlcdl commented 1 year ago

I will give this more thought because multi-language is something I feel strongly about. Since english messages change over time, we stand to lose lots of translations whenever we change a slug in the code. For a few translations and well known translators, it's not a big deal, we can chase them down and have them take a look. But once there are many languages, we'll lose message translations that were "good enough" whenever we make the slightest english change in code if we're not careful to pull those translated messages back onto the new slug before we ditch unused slugs and commit changes.

tadeubas commented 1 year ago

The way Krux does follows the i18n standards, I don't think we need to change that, we would be "reinventing the well" because we use i18n as a lib... the problem occurred only because of the code prettify, it is somewhat common to have this issues, sometimes there isn't a tool that handle everything, we need to do manual adjustments on the code or create exceptions to prettify, so not a big issue as @odudex pointed out how to fix this. Translators would not need to find how a message is composed because it is something direct to read the text on the screen or direct on the code... there are other tools to handle translations separated from the code, people tend to use transifex, we could study this possibility and do the code to match the transifex translations values to the i18n we current use. But this would be a new feature or a major change on how Krux works.

jdlcdl commented 1 year ago

Thanks @tadeubas,

I'm not trying to change how krux does i18n immediately, just trying to plant some seeds to consider as the code approaches perfection and continues to get edits because messaging can never hope to be "Done."; if english is just another language, the messages in the code might be as clean/tight as the rest of the code. There would be no hesitation to improving messaging in english because it would be done in en-EN.json instead of the codebase -- avoiding the hassle of updating all other locales or losing translations, just as is the case for all other locales. We'll get a better feel over time.

I'm ok with @odudex's approach because it fixes this so that @aglkm can continue with translations. I still prefer the one-liner without triple-double-quotes, which survives black, for the reasons stated above. Both are better than a missing translation.

odudex commented 1 year ago

We recently removed the en-EN.json in favor of just using what's in the code directly. Maybe we should have discussed this more. But now it's done I suggest we move on. I recommend to use the slugs with moderation, don't break sentences unless there is punctuation, and I think we won't fall into context or grammar issues.

odudex commented 1 year ago

Fixed in c009b8a3a08fcb4ac6de994380e89c880dbe547a