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
base: master
Are you sure you want to change the base?
Conversation
* 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
There was a problem hiding this 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), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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." |
There was a problem hiding this comment.
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",)) |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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 " |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)) + "]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you forget tr
?
There was a problem hiding this comment.
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, '-')] = {} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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. |
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. |
Yes - I know about POEditor. My Slovak translation was imported in there. |
uf2dialog
andcp_front