fix: display chinese character avatar #51855
Conversation
Phreeman33
requested review from
ArtificialOwl,
skjnldsv and
yemkareems
and removed request for
a team
lib/private/Avatar/Avatar.php
Outdated
| public function __construct( | ||
| LoggerInterface $logger, | ||
| private User $user, | ||
| private IConfig $config, | ||
| ) { | ||
| $this->logger = $logger; | ||
| $this->user = $user; | ||
| $this->config = $config; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the user display name. | ||
| */ | ||
| abstract public function getDisplayName(): string; | ||
|
|
||
| /** | ||
| * Returns the first letter of the display name, or "?" if no name given. |
There was a problem hiding this comment.
This changes seem to be unrelated, can you revert or is there a good reason to change the way the displayname is fetched?
There was a problem hiding this comment.
These changes are indeed not necessary. The main reason is that when I examined the getDisplayName() function, I found that it simply wraps a single line: return $this->user->getDisplayName();—and I question how much benefit this approach actually provides. However, after further reflection, these modifications seem rather hasty, and I'm not entirely sure whether this function is used elsewhere.
There was a problem hiding this comment.
Considering the GuestAvatar construct is different, I'd like to keep the DI out of the abstract class :)
susnux
added
bug
3. to review
lib/private/Avatar/Avatar.php
Outdated
| @@ -95,6 +97,24 @@ protected function getAvatarVector(int $size, bool $darkTheme): string { | |||
| return str_replace($toReplace, [$size, $fill, $fgFill, $text], $this->svgTemplate); | |||
| } | |||
|
|
|||
| protected function getFont(string $userDisplayName): string { | |||
| if (preg_match('/\p{Han}/u', $userDisplayName) === 1) { | |||
| switch ($this->config->getUserValue($this->user->getUID(), 'core', 'lang', '')) { | |||
There was a problem hiding this comment.
That's a good starting point, but that still means that if I set my lang to English this will still generate a wrong avatar 🤔
There was a problem hiding this comment.
IIRC there is no other way to detect this, with the regex we only know if there is a CJK character, but you do not know which language. Meaning they all map to the same unicode character, but only differ in rendering defined by the language.
Maybe we should fallback to some of those if it is CJK
There was a problem hiding this comment.
The first fallback option is to determine the font based on location, and the second option is to reference information from the browser or operating system. Currently, I have only implemented the most basic solution.
No matter how many fallback options there may be, if all language reference targets are non-CJK languages but the user still wants to use Chinese character avatars, then the NotoSansSC font rendering is the only option available. As a manual workaround, the user can simply change the language setting to the desired CJK language for rendering, generate the avatar, and then switch back.
There was a problem hiding this comment.
@skjnldsv see https://en.wikipedia.org/wiki/Han_unification
The problem is that they share the same code point for different "glyphs", also if this would be possible this would result in a like 100MB font file which will lead to memory issues with out PHP config of 512MB (and other perf issues).
Also @Phreeman33 I think this is a good solution, but as I noted below please just test the $text that is rendered for the CJK characters. Also if it contains CJK characters just fallback to NotoSansSC if language is not set to any CJK. locale should not be used, this is only for the formatting of number, money, dates and times - it is unrelated to the language of the text or the writing.
There was a problem hiding this comment.
We could also use https://github.com/PhenX/php-font-lib to check if the font contains all the glyph of a string 🤔
Ideally I would also just check the first two letters from the getAvatarText method. That should avoid checking too many characters.
There was a problem hiding this comment.
Ideally I would also just check the first two letters from the getAvatarText method. That should avoid checking too many characters.
Already done below, it gets only passed the text that is rendered (the letter).
We could also use https://github.com/PhenX/php-font-lib to check if the font contains all the glyph of a string 🤔
As said this does not work as they are the same but only different design due to language (all of this are based on the same code point but differ in design depending on language):
https://en.wikipedia.org/wiki/Han_unification#/media/File:Source_Han_Sans_Version_Difference.svg
There was a problem hiding this comment.
Ok, I guess I'm lacking the knowledge for this :)
There was a problem hiding this comment.
Ok, I guess I'm lacking the knowledge for this :)
#25529 for a deep dive into the topic ;)
can't we merge the fonts?
Afaik there's a limit of glpyhs in a font file
There was a problem hiding this comment.
@Phreeman33 could you please revert all other changes to keep the abstract getDisplayName method?
The only changes needed in this PR are:
getFontgenerateAvatar- the regular fonts
Signed-off-by: Phreeman33 <[email protected]>
Co-authored-by: Ferdinand Thiessen <[email protected]> Signed-off-by: Phreeman33 <[email protected]>
Signed-off-by: Phreeman33 <[email protected]>
Signed-off-by: Phreeman33 <[email protected]>
|
@susnux |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
Summary
This PR builds upon #42534 for fixes an issue where Chinese character avatars were not displaying properly. It also applies fonts tailored to the regional variations in Chinese writing conventions. The code was modified by a noob and has only undergone basic testing, so further testing and review may be necessary.
Screenshots
\u89d2 in SC
\u89d2 in TC/HK/JP/KR
\u8005 in KR
\u8005 in SC/TC/HK/JP
\u8aaa in JP/KR
\u8aaa in SC/TC/HK
TODO
Checklist