Skip to content

Fix rendering library thumbnail if it has an Arabic name - #8050

Closed
hadicharara wants to merge 5 commits into
jellyfin:masterfrom
hadicharara:hadicharara/fix-arabic-text-rendering
Closed

Fix rendering library thumbnail if it has an Arabic name#8050
hadicharara wants to merge 5 commits into
jellyfin:masterfrom
hadicharara:hadicharara/fix-arabic-text-rendering

Conversation

@hadicharara

@hadicharara hadicharara commented Jul 3, 2022

Copy link
Copy Markdown

Library thumbnail rendering were broken with Arabic names, due to a bug in the DrawText function of the canvas.
Example for the word أفلام:
لقطة شاشة 2022-07-02 في 20 55 50

Changes
I used CanvasExtensions.DrawShapedText to properly render Arabic fonts. However, this function does not consider TextAlign (and calculating text width for Arabic fonts is complicated). As such, I am only using it for Arabic library names, and have set a fixed position from the left for the library name.

لقطة شاشة 2022-07-02 في 20 57 53

Although not ideal, it is better than before. I am open to any feedback!

@hadicharara hadicharara changed the title Fixed rendering library thumbnail if it has an Arabic name Fix rendering library thumbnail if it has an Arabic name Jul 8, 2022
@jellyfin-bot jellyfin-bot added the merge conflict Merge conflicts should be resolved before a merge label Jul 20, 2022
@Bond-009

Copy link
Copy Markdown
Member

due to a bug in the DrawText function of the canvas.

Is this a Skia issue?

@hadicharara

Copy link
Copy Markdown
Author

due to a bug in the DrawText function of the canvas.

Is this a Skia issue?

Yes exactly! I had added a library that fixed this problem. Not sure if I should rebase though because of the lack of interest in this MR.

@crobibero

Copy link
Copy Markdown
Member

Please rebase (and update added deps)
Unsure if something like this will work, but seems better to me then using Regex.

https://stackoverflow.com/a/4331154

@hadicharara
hadicharara force-pushed the hadicharara/fix-arabic-text-rendering branch from 903189a to 994e0aa Compare September 18, 2022 00:35
@jellyfin-bot jellyfin-bot removed the merge conflict Merge conflicts should be resolved before a merge label Sep 18, 2022
@hadicharara

Copy link
Copy Markdown
Author

Unsure if something like this will work, but seems better to me then using Regex.

Considering this won't run often, I'm not sure if it's worth the performance improvement. However, if you insist, I'll look into it. Also, I just rebased and updated the dependencies.

@crobibero

Copy link
Copy Markdown
Member

Unsure if something like this will work, but seems better to me then using Regex.

Considering this won't run often, I'm not sure if it's worth the performance improvement. However, if you insist, I'll look into it. Also, I just rebased and updated the dependencies.

If my link works it should fix the image for all RTL languages, not just Arabic; so I think it should at least be tested

@hadicharara

hadicharara commented Sep 18, 2022

Copy link
Copy Markdown
Author

Alright I didn't think about other languages. I'll test it and report back.

@hadicharara

Copy link
Copy Markdown
Author

I did it. It should work with all RTL languages. Result is still sub-par but better than before.

@cvium

cvium commented Oct 7, 2022

Copy link
Copy Markdown
Member

I don't think this PR is entirely correct. Some of the values are outside char range. You need to convert them to utf32 like the SO post says

@jellyfin-bot jellyfin-bot added the merge conflict Merge conflicts should be resolved before a merge label Oct 10, 2022
@jellyfin-bot jellyfin-bot removed the merge conflict Merge conflicts should be resolved before a merge label Oct 13, 2022
@hadicharara

Copy link
Copy Markdown
Author

Converted characters to UTF-32 now.

@jellyfin-bot jellyfin-bot added the merge conflict Merge conflicts should be resolved before a merge label Jan 11, 2023
@crobibero crobibero linked an issue Apr 6, 2023 that may be closed by this pull request
1 task
@ghost

ghost commented Apr 7, 2023

Copy link
Copy Markdown

@barronpm

Copy link
Copy Markdown
Member

Solved by #9612

@barronpm barronpm closed this Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge conflict Merge conflicts should be resolved before a merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Issue]: RTL Issue with auto generated photos

6 participants