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

Add Simplified Chinese Support #772

Merged
merged 36 commits into from
Sep 21, 2023
Merged

Add Simplified Chinese Support #772

merged 36 commits into from
Sep 21, 2023

Conversation

smallg0at
Copy link
Contributor

@smallg0at smallg0at commented Sep 7, 2023

This pull request add Simplified Chinese as a language option.

The docs haven't been (and will not be) translated, and will redirect to the English version. Support is work in progress. Translated, but only in-app doc will be available.

This pull request also introduces the following changes:

  • Pipeline Window title now properly uses localization strings.
  • Tile MenuItem now have localization strings.
    • Due to my limited knowledge in Italian, its italian string is currently identical to the English one
  • Add Chinese documentation actions

@smallg0at
Copy link
Contributor Author

Something's going wrong with the code analysis here...

@lupino3
Copy link
Member

lupino3 commented Sep 7, 2023

Hi @smallg0at, and thanks for your contribution! I kicked off the CI pipelines, since PR by non-owners by default do not trigger them.

Codacy checks are failing because your changes trigger some tests that were anyway failing: https://app.codacy.com/gh/EduMIPS64/edumips64/pullRequest?prid=12573761. We'll see if I can just bypass it. Let's see if the other build and test pipelines succeed.

I'll be happy to assist you in getting this PR merged and releasing a new version with Simplified Chinese translation, thanks a lot for your work!

Copy link
Member

@lupino3 lupino3 left a comment

Choose a reason for hiding this comment

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

I think we need to fix one constant to make it compile. I provided suggestions, please accept them if they are correct. Thanks!

src/test/java/org/edumips64/CliRunnerTest.java Outdated Show resolved Hide resolved
src/test/java/org/edumips64/CliRunnerTest.java Outdated Show resolved Hide resolved
@smallg0at
Copy link
Contributor Author

I think we need to fix one constant to make it compile. I provided suggestions, please accept them if they are correct. Thanks!

They are indeed correct. Just fixed them.

@smallg0at
Copy link
Contributor Author

smallg0at commented Sep 7, 2023

I'm seeing this font fallback issue here. For some reason it doesn't fallback properly, and I am unable to fix :(

image

@lupino3
Copy link
Member

lupino3 commented Sep 7, 2023

I'm seeing this font fallback issue here. For some reason it doesn't fallback properly, and I am unable to fix :(

image

I'll have a look at this as soon as possible!

@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2023

Codecov Report

Merging #772 (3303eb9) into master (9281ed7) will increase coverage by 0.91%.
The diff coverage is 91.18%.

❗ Current head 3303eb9 differs from pull request most recent head 85b4dce. Consider uploading reports for the commit 85b4dce to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #772      +/-   ##
============================================
+ Coverage     51.14%   52.06%   +0.91%     
+ Complexity     1447     1444       -3     
============================================
  Files           249      249              
  Lines          9879    10138     +259     
  Branches       1092     1092              
============================================
+ Hits           5053     5278     +225     
- Misses         4507     4541      +34     
  Partials        319      319              
Files Changed Coverage Δ
src/main/java/org/edumips64/Main.java 0.00% <0.00%> (ø)
...c/main/java/org/edumips64/utils/CurrentLocale.java 99.30% <100.00%> (+0.34%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9281ed7...85b4dce. Read the comment docs.

@lupino3
Copy link
Member

lupino3 commented Sep 9, 2023

Ok, I think I am able to reproduce the error:

image

FWIW, I had to add the following line to build.gradle.kts, otherwise it would not compile on my Windows machine:

image

@lupino3
Copy link
Member

lupino3 commented Sep 9, 2023

Ok, I found the problem. We are explicitly setting the Verdana font in ErrorDialog, which cannot display Chinese characters, apparently. Removing the explicit font setting fixes the issue:

image image

This is only a work-around though. We need to manage fonts more consistently in the app.

I'll see if I can think about how to do it tomorrow, so I can unblock your PR, @smallg0at

@lupino3
Copy link
Member

lupino3 commented Sep 10, 2023

Hi @smallg0at, I just merged #776, which should fix the errors that caused you problems.

Could you please rebase your changes on top of the new master branch?

Then your PR should be good to go.
Let me know if you need any help, and thanks for your contribution!

Once your change is merged, I'll release a new EduMIPS64 version, so you can get the Simplified Chinese version from official packages.

@smallg0at
Copy link
Contributor Author

I'm currently working on translating the docs, so it might take some extra days to complete

@lupino3
Copy link
Member

lupino3 commented Sep 12, 2023

Thank you very much, @smallg0at! Appreciate your initiative and effort. Will wait for your work to be finished!

@smallg0at
Copy link
Contributor Author

smallg0at commented Sep 12, 2023

The translation work is complete, though it still has the following problems:

  • Due to the use of machine translation (DeepL), many warnings will appear.
  • Chinese docs PDF currently fail to build and will produce empty file
         [ERROR] pdfbuilder.py:161 Failed to build doc
         Traceback (most recent call last):
         Traceback (most recent call last):FAILED
           File "D:\Code\edumips64\.gradle\python\Lib\site-packages\rst2pdf\pdfbuilder.py", line 158, in write
             docwriter.write(doctree, destination)
           File "D:\Code\edumips64\.gradle\python\Lib\site-packages\docutils\writers\__init__.py", line 80, in write
             self.translate()
           File "D:\Code\edumips64\.gradle\python\Lib\site-packages\rst2pdf\pdfbuilder.py", line 621, in translate
             contents[0] += nodes.Text(langmod.labels['contents'])
                                       ^^^^^^^^^^^^^^
         AttributeError: module 'zh' has no attribute 'labels'
  • and due to this console encoding problem the gradle output I see is complete mess.

image

@lupino3
Copy link
Member

lupino3 commented Sep 13, 2023

It seems to be building correctly in Github Actions.

@lupino3
Copy link
Member

lupino3 commented Sep 13, 2023

image

in-application help works, but it doesn't render well for me

@smallg0at
Copy link
Contributor Author

smallg0at commented Sep 19, 2023

Thanks @smallg0at. I had tried the same fix here a few days ago (f32f1ff) but it didn't seem to work :/

Checked the GitHub Actions docs, might be due to the version getting action not actually outputting anything. Working on a fix.

EDIT: I was wrong. The source of this bug is due to the upload jar action reading a wrong value (should be version). SHOULD be fixed now.

More info can be seen here: https://github.com/christian-draeger/read-properties

@smallg0at
Copy link
Contributor Author

Good news: it has been confirmed fixed.

@smallg0at
Copy link
Contributor Author

There is some hope for rst2pdf however. Logs shows that it cannot locate the stylesheet files, which may be crucial for the implementation.

image

@smallg0at
Copy link
Contributor Author

Seems to need a token for Webapp distribution to work

https://github.com/smallg0at/edumips64/actions/runs/6237968146/job/16932897585#step:9:23

@lupino3
Copy link
Member

lupino3 commented Sep 19, 2023

Thanks for the read-properties fix, I'm actually merging it separately here once I see it works: #777 (I'll credit you in the PR)

@smallg0at
Copy link
Contributor Author

image

Found this issue, but in GBK this time. According to my findings this is probably what happened:

  • When changing locale, JDK will automatically change the encoding that it's running on. While it defaults to utf-8 in chinese when installed, the other languages do not get this treatment. HOWEVER, when changing locale to Chinese (China), it will instead modify the settings to be GBK, which adds up another level of mess.
  • When loading up a help file, javax.help.HelpBroker will NOT respect the HTML encoding and will use the default system-wide java encoding.

@smallg0at
Copy link
Contributor Author

Found this tool which has Russian help working with Javahelp, might be able to see how the localization works: https://github.com/logisim-evolution/logisim-evolution.

This project use JHelp and the implementation can be seen in src\main\java\com\cburch\logisim\gui\menu\MenuHelp.java. I can't really code Java so I'm unable to help.

@smallg0at
Copy link
Contributor Author

smallg0at commented Sep 20, 2023

@lupino3 Let's give it one last try to at least allow the release MSI has normal working Chinese docs. Please set your system variable JAVA_TOOL_OPTIONS to -Dfile.encoding=UTF8, then build the msi and test if the chinese in-app docs works properly in that distribution. If it doesn't work, I'll give up and use the English one.

@lupino3
Copy link
Member

lupino3 commented Sep 21, 2023

It works!
image
The environment variable was picked up by Gradle correctly, it echoed it during the build process.

I wonder if we can just set it in the Gradle file and have it work for all types of build?

@lupino3
Copy link
Member

lupino3 commented Sep 21, 2023

Ok, I verified that it doesn't seem to matter how it's built, but that option needs to be set as an environment variable when the JAR is executed. Let's see if we can work around it.

@lupino3
Copy link
Member

lupino3 commented Sep 21, 2023

I'm pretty sure the MSI works because we already pass -Dfile.encoding=utf-8 to the java command line that we build for the Windows shortcut:

image

What we can do is to add this option to the Ubuntu package as well and add a warning at the top of the Chinese docs saying (in English) to load the JAR with -Dfile.encoding=utf-8 if the help looks garbled.

According to https://stackoverflow.com/questions/361975/setting-the-default-java-character-encoding, there is no way of setting this option within the source code, as it's something used during initialization.

@smallg0at
Copy link
Contributor Author

smallg0at commented Sep 21, 2023

Thanks for testing. I'll do the following things and then it will be ready to merge:

@smallg0at
Copy link
Contributor Author

This pull request is ready for merging, as long as Codacy don't do anything funny.

@lupino3 lupino3 merged commit d12a759 into EduMIPS64:master Sep 21, 2023
9 of 11 checks passed
@lupino3
Copy link
Member

lupino3 commented Sep 21, 2023

Thanks for your contribution here @smallg0at! I'll see if I can take a look at the issues you filed in the next days, and then I'll create a new release with Simplified Chinese support!

Copy link
Member

@lupino3 lupino3 left a comment

Choose a reason for hiding this comment

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

Approved, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants