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.

Sign up for GitHub

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

Jump to bottom

[desktop] make fonts depend on language and platform #2011

Merged
merged 0 commits into from May 26, 2020
Merged

[desktop] make fonts depend on language and platform #2011

merged 0 commits into from May 26, 2020

Conversation

ganthern
Copy link
Contributor

@ganthern ganthern commented Apr 20, 2020 •
edited by charlag

adds a function getFonts() to main-styles.js that returns the
font-family string for the body style.

will add additional fonts depending on the platform and used language.

AppearanceSettingsViewer will now also cause a regeneration of the
style tags and a change of language on the native side when the language
is changed by the user.

fix #1909

@ganthern ganthern linked an issue Apr 20, 2020 that may be closed by this pull request
Japanese kanji are not Japanese #1909
Closed
2 tasks
@ganthern ganthern changed the title [desktop] provide option to override the default font, close #1909 [WIP/desktop] provide option to override the default font, close #1909 Apr 20, 2020
@ganthern
Copy link
Contributor Author

Still WIP because it's missing a config migration which could be merged with the one in #1928

@ganthern ganthern marked this pull request as draft April 20, 2020 23:37
@charlag charlag force-pushed the 1909-kanji-fonts branch 2 times, most recently from 1a9ed7b to 76ee1df Compare April 21, 2020 08:08
@ganthern ganthern marked this pull request as ready for review April 21, 2020 08:16
@ganthern ganthern changed the title [WIP/desktop] provide option to override the default font, close #1909 [desktop] provide option to override the default font, close #1909 Apr 21, 2020
@charlag
Copy link
Contributor

charlag commented Apr 28, 2020

we can't rely on locale to detect which font would be correct for the user

is it so? I think that's what Electron will (hopefully) do with electron/electron#23247

@ganthern
Copy link
Contributor Author

Well, at the moment it's not reliable, but they seem to be working on it.
Maybe we could use the LanguageViewModel language tag to choose a default font.
Users that sometimes need a chinese font and at other times a japanese one would need to switch languages then instead of changing the font setting.

@charlag
Copy link
Contributor

charlag commented Apr 28, 2020

Hm both sound bad but probably we can cover most cases if we base it on a language and we won't have additional confusing setting.

@ganthern
Copy link
Contributor Author

Another problem is that, as far as I can tell from my research, chinese fonts seem to be inconsistent in what they're called on windows at least, which is why SimHei and 黑体 both are in the list even though they're the same font. The default font setting for the electron browser window takes one (and only one) font name, which then is used as default for the sans-serif family.
If a system has the font we chose for the language, but written differently, it wouldn't work.

Changing the body selector in main-styles.js would be another option to get maximum coverage.
In known problematic platform/client/language combinations, we could add a list of fonts to the font-families key.

@charlag
Copy link
Contributor

charlag commented Apr 28, 2020

Yeah, probably adding fonts to the body is a good idea (esp. because we discard style blocks currently). Won't screw up all fonts, will fall back properly.

@ganthern ganthern marked this pull request as draft April 30, 2020 07:36
@ganthern ganthern changed the title [desktop] provide option to override the default font, close #1909 [desktop] make fonts depend on language and platform, close #1909 May 5, 2020
@ganthern ganthern marked this pull request as ready for review May 12, 2020 06:43
@ganthern ganthern force-pushed the 1909-kanji-fonts branch 3 times, most recently from 6a54dcf to 9a38084 Compare May 19, 2020 07:12
charlag
charlag reviewed May 26, 2020
View reviewed changes
Copy link
Contributor

@charlag charlag left a comment

Choose a reason for hiding this comment

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

I will probably modify fonts a little bit but otherwise looks good!

app-ios/tutanota/Sources/TUTViewController.m Outdated
@@ -183,6 +183,8 @@ - (void)userContentController:(nonnull WKUserContentController *)userContentCont
[_fileChooser openWithAnchorRect:rect completion: sendResponseBlock];
} else if ([@"getName" isEqualToString:type]) {
[_fileUtil getNameForPath:arguments[0] completion:sendResponseBlock];
} else if ([@"changeLanguage" isEqualToString:type]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

kudos for figuring this stuff out!

src/native/SystemApp.js Outdated Show resolved Hide resolved
@charlag charlag changed the title [desktop] make fonts depend on language and platform, close #1909 [desktop] make fonts depend on language and platform May 26, 2020
@charlag
Copy link
Contributor

charlag commented May 26, 2020

I was about to merge this but noticed that electron/electron#18829 is fixed. Maybe we can remove the workaround soon.

@charlag charlag merged commit 96c419d into master May 26, 2020
@charlag charlag deleted the 1909-kanji-fonts branch May 26, 2020 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charlag charlag charlag left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Japanese kanji are not Japanese
2 participants
@ganthern @charlag