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

Application: add Chinese shared font support #49

Closed
wants to merge 1 commit into from
Closed

Application: add Chinese shared font support #49

wants to merge 1 commit into from

Conversation

wwwwwwzx
Copy link

also works for traditional Chinese characters.
issue #26

@natinusala
Copy link
Owner

Thanks for the PR!

I have a concern about memory usage (that I already had for the Korean font at the time).

How large is the Chinese font? Does it contain latin characters as well?

My idea would be to only load Chinese font if the system language is Chinese (same for Korean). What do you think?

@wwwwwwzx
Copy link
Author

Agree with loading the font if the system language is so. (But I am not familiar with the API)
Have done this for Plutonium: zaksabeast/CaptureSight@aaccce5

@natinusala
Copy link
Owner

natinusala commented Jul 19, 2020 via email

@WerWolv
Copy link
Contributor

WerWolv commented Jul 19, 2020

Latin font should always be loaded. In EdiZon for example I always fall back to thr English translation if there's none in the currently selected language. That way you can have translations but don't make your app completely unusable if a few important translations are missing

@natinusala
Copy link
Owner

@WerWolv yeah but what if the Chinese font also contains [A-Za-z0-9]? We don't need to load both then, do we?

@WerWolv
Copy link
Contributor

WerWolv commented Jul 19, 2020

Does it have it? I never checked. If so then yeah.
A way to still force load the fonts if needed would be nice though. I have a custom language switcher in my settings which really depends on all of them

@natinusala
Copy link
Owner

Well then I guess we can start by only loading Chinese and Korean if the system language needs it.

@wwwwwwzx can you make the change in your PR?

@wwwwwwzx
Copy link
Author

Checked in CaptureSight.
iPad Screen Shot 7-19-20, 3 50 AM

@natinusala
Copy link
Owner

That looks nice! Can you ensure that all latin characters are present, including accentued ones? It's for @WerWolv

Also I'm going to ask you to run./scripts/format --fix (you will need clang-format >= 10) and rebase + squash your PR before I can merge it.

@wwwwwwzx
Copy link
Author

  1. Tested with string: "Å å Ǻ ǻ Ḁ ḁ ẚ Ă ă Ặ ặ Ắ ắ Ằ ằ Ẳ ẳ Ẵ ẵ Ȃ ȃ Â â Ậ ậ Ấ ấ Ầ ầ Ẫ ẫ Ẩ ẩ Ả ả Ǎ ǎ Ⱥ ⱥ Ȧ ȧ Ǡ ǡ Ạ ạ Ä ä Ǟ ǟ À à Ȁ ȁ Á á Ā ā Ā̀ ā̀ Ã ã Ą ą Ą́ ą́ Ą̃ ą̃ A̲ a̲ ᶏ" (copied from wiki)
    Only loaded Chinese font
    iPad Screen Shot 7-19-20, 4 10 PM
    Added regular font
    iPad Screen Shot 7-19-20, 4 09 PM
    So probably let's load the regular font in Chinese and Korean system as well.

  2. clang-format is automatically running in my IDE. I have 14 other changed files after running the script. Are you sure to squash them into this pull request?

@natinusala
Copy link
Owner

clang-format is automatically running in my IDE. I have 14 other changed files after running the script. Are you sure to squash them into this pull request?

Uh, no 😅 Please run it using the script we provide, it has our configuration with our code style in it.

Then we need to always load regular latin font as well, which is what you did in the commit.

However it seems like you catched junk from master while rebasing, can you fix that please?

@wwwwwwzx
Copy link
Author

I did run your script. And at first, the error is the following (I have 10.0.0 installed):
run-clang-format.py: error: Command 'clang-format-8 --version' failed to start: [Errno 2] No such file or directory
After changing the --clang-format-executable to clang-format, there are 14 changed files (see the second commit)

@natinusala
Copy link
Owner

Oh shoot I misunderstood the first time. Let me run clang-format on the entirety of master, that way it doesn't interfere with your changes. Sorry about that.

@wwwwwwzx
Copy link
Author

Uh, conflicts are still there. Sorry I am not familiar with git commands.

@natinusala
Copy link
Owner

You need to rebase on the master branch, they should resolve themselves

If not, git will prompt you to fix them and it should be fairly easy

Works for traditional Chinese characters

Load Asian font according to the system language setting
@natinusala
Copy link
Owner

Hi, sorry for the delay! I do not accept contributions for master anymore so I will be closing this PR. #76 aims to reimplement what you did in the rework branch, if you want to have a look.

@natinusala natinusala closed this Mar 7, 2021
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