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

Better i18n support in uf2dialog and cp_front #2595

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

bletvaska
Copy link

  • more strings are now available for translation in uf2dialog and cp_front

* surrounded more "visible" strings in the dialog with `tr()` function
* some f-strings were revewrited with `.format()` to make them translate with `tr()`
* fix issue of wrong filename in string - `pyveng.cfg`
    * should be `pyvenv.cfg`
* more strings are currently translatable
* updated slovak translation
Copy link
Member

@aivarannamaa aivarannamaa left a comment

Choose a reason for hiding this comment

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

Thank you!

Please see my comments.

(In the future I recommend to make a proposal before undertaking a nontrivial i18n effort, because I've left some strings untranslatable on purpose, eg. because I expect them to change and don't want to add extra work for dozens of translators just by invalidating it soon after)

"Bad encoding",
"Could not read as %s text.\nYou could try another encoding" % encoding,
tr("Bad encoding"),
tr("Could not read as {} text.\nYou could try another encoding").format(encoding),
Copy link
Member

Choose a reason for hiding this comment

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

Most of the translatable strings contain the old style formatting markers, so I suggest you leave %s here as well.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I saw it. My decision was based about my experiences. When I teach Python programming and we talk about string formatting in Python, this is something I am not showing to students, or at least I notice them about not to use this style of formatting. I've seen some articles mentioning about not using this style, and also in the official documentation is mentioned, that using such style can be tricky in some situations. Also, when I was translating Mu editor, they were not using "old style" but new one. Question can be about the mentioning the parameter inside of the brackets, so instead of translating string in the form:

"Could not read as {} text.\nYou could try another encoding"

will be much more "readable" and "explainable" to have string in format:

"Could not read as {encoding} text.\nYou could try another encoding"

But the final decision is up to you. So I can put it back to "old style".

@@ -241,7 +241,7 @@ def __init__(self, master):
"You can activate an existing virtual environment also"
+ " via the right-click context menu in the file navagation"
+ " when selecting a virtual environment folder,"
+ " or the 'pyveng.cfg' file inside."
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for noticing this typo!

"This dialog uses `pipkin`, a new command line tool for managing "
"MicroPython and CircuitPython packages. ",
tr("This dialog uses `pipkin`, a new command line tool for managing "
"MicroPython and CircuitPython packages."),
("right",),
)
self._append_info_text("See ", ("right",))
Copy link
Member

Choose a reason for hiding this comment

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

I suppose you have omitted "See .... for more info" because of the piecewise placement of the whole sentence. You could still make it translatable by translating and splitting like this

trans = tr("See %s for more info")
prefix = trans.split("%s")[0].strip()
suffix = trans.split("%s")[1].strip()

Copy link
Author

Choose a reason for hiding this comment

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

My original idea was just about making "visible strings" translatable without any big effort ;) I can take a closer look (also about how tkinter works).

self._append_info_text(
"This dialog uses `pipkin`, a new command line tool for managing "
"MicroPython and CircuitPython packages. ",
tr("This dialog uses `pipkin`, a new command line tool for managing "
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the word "new" from this text. It has to be removed at some point, but removing it later would invalidate all translations written so far.

Copy link
Author

Choose a reason for hiding this comment

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

makes sense

label = ""
else:
text = f"[found {len(self._target_combo.mapping)} targets, please select one]"
text = "[" + 'found {} targets, please select one'.format(len(self._target_combo.mapping)) + "]"
Copy link
Member

Choose a reason for hiding this comment

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

did you forget tr?

Copy link
Author

Choose a reason for hiding this comment

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

Hmm... It looks like...

for variant in populars.values():
popular_variant = variant.copy()
# need different title to distinguish it from the same item in ALL VARIANTS
popular_title = self._create_variant_description(variant) + " "
popular_variant["title"] = popular_title
enhanced_mapping[popular_title] = popular_variant

enhanced_mapping["--- ALL VARIANTS " + "-" * 100] = {}
enhanced_mapping["--- " + tr('ALL VARIANTS ').ljust(100, '-')] = {}
Copy link
Member

Choose a reason for hiding this comment

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

better move the trailing space outside of the tr argument

Copy link
Author

Choose a reason for hiding this comment

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

You are right. I was experimenting a bit to make it working. And I think, my decision was based on some strings, which also ends with trailing space. I'll fix it.

@bletvaska
Copy link
Author

ahoj

and thanks for the reply. initially my goal was to improve slovak translation, but then i found more strings, which are normally visible for the users, untranslatable, so i started digging in the code. meanwhile i've was digging again, but i decided to wait for your answer. so - thanks - i'll write my answers to your comments.

@aivarannamaa
Copy link
Member

Just to avoid confusion -- I hope you are aware of Thonny's POEditor project and you didn't enhance existing translatable strings directly in the po-file.

For the new strings it's understandable that you couldn't translate them in POEditor. After accepting the PR I'll update the pot file in the POEditor and try to import your new translations for Slovak as well.

@bletvaska
Copy link
Author

Yes - I know about POEditor. My Slovak translation was imported in there.

@aivarannamaa aivarannamaa added this to the 5.0.0 milestone Aug 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants