-
Notifications
You must be signed in to change notification settings - Fork 24
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
More graceful translations startup handling #630
Conversation
It's late, and this is still causing startup errors now. Now I'm getting errors like:
@StandingPadAnimations need your input here, since you're more familiar with the translation startup sequence. Whatever fix we try, we should please try to do an install over a previously prod v3.6.0 release to sanity check it's working, that's where I keep hitting a wall. You have my permission to take over this branch if you have ideas of a more suitable fix. In general, I do think it's not ideal to only conditionally define a property to exist, which was the reason for the original bug above. I'm also not sure why it's in the branch where content isn't loaded, since I've confirmed my local build's zip does indeed contain the .mo and .po files for each of the languages above. |
FYI I think I have a decent fix locally, will push in a bit. Doing some testing |
Test is NOT yet passing/working, might need some peer assistance
self.assertTrue( | ||
os.path.isfile( | ||
lang_folder / lang / "LC_MESSAGES" / "mcprep.mo"), | ||
f"Missing {lang}'s mo file") |
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.
If nothing else, we could reduce the test down to just this, which already goes a long way to prove that translations are present when they should be.
Likewise made the gitignore a little more strict for what we want
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.
That was a doozy, one thing after another - but, we're there. All local tests pass, as do the ones on github actions above thankfully
-------------------------------------------------------------------------------
bversion ran_tests ran skips failed errors
-------------------------------------------------------------------------------
(4.2.0) all_tests 70 2 0 No errors
(4.0.2) all_tests 70 2 0 No errors
(3.6.2) all_tests 70 2 0 No errors
(3.5.1) all_tests 70 2 0 No errors
(3.4.0) all_tests 70 2 0 No errors
(3.3.1) all_tests 70 2 0 No errors
(3.2.1) all_tests 70 2 0 No errors
(3.1.0) all_tests 70 2 0 No errors
(3.0.0) all_tests 70 2 0 No errors
(2.93.0) all_tests 69 3 0 No errors
(2.90.1) all_tests 69 3 0 No errors
(2.80.75) all_tests 68 4 0 No errors
Compiled in 30.6s + tests ran in 269.9s
Total of 300.5s with exit code 0
test_translations = [ | ||
("ru_RU", "Restart blender", "Перезапустите блендер"), | ||
# Blender 4.0+ only has 'zh_HANS', 'zh_HANT' | ||
("zh_HANS" if bpy.app.version > (4, 0) else "zh_CN", "Texture pack folder", "材质包文件夹"), |
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.
Explanation here, and also why I've added a new PO file (simplest option I could think of without further consequences/code changes)
Thanks to this test, I discovered we weren't supporting Chinese pre 4.0 due to Blender splitting into more sub regional names. The enum key zh_HANT only exists as os 4.0 (thus this test was failing pre blender 4.0), and meanwhile zh_CN only exists before. I understand from some reading that zh_HANT is more regional version of simplified Chinese, so I feel it's appropriate as a stopgap for now.
@@ -19,5 +19,12 @@ venv/ | |||
*.sublime-* | |||
MCprep_addon/import_bridge/conf | |||
MCprep_addon/import_bridge/nbt | |||
MCprep_addon/MCprep_resources/ | |||
MCprep_addon/MCprep_resources/*.blend |
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.
Before this change, we were masking .po files, felt it was worth making sure we had it recognized explicitly.
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.
An as-is copy of the zh_HANS.
I live-tested in both blender 3.x and 4.x to make sure that translations work fine in both cases (I'd say, it feels odd that the translations actually were working in blender 4.0 before since the contents of the po reference zh_CN and not zh_HANS, but I'm not going to dive into understanding that right now)
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.
If you're curious, zn_CN
is the old code for Simplified Chinese. It was changed to zn_HANS
since many countries outside of China also use Simplified Chinese (at least that's what I was able to gather when I looked into it a while back)
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.
Looks good to me. Just a minor stylistic nitpick
Closes #627 hopefully for real.
Was to fix this error that was seen on install/startup with the initial v3.6.1.1 release which has now been pulled:
Working on another patch for you to review.