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

Implement initial support for RTL languages #3958

Merged
merged 23 commits into from Nov 10, 2022

Conversation

mohad12211
Copy link
Contributor

@mohad12211 mohad12211 commented Sep 8, 2022

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

image

This is a fix #720 for RTL languages.

  • line breaks work.
  • Neutral words (words that contains numbers or non-alphabetical characters like & $ # etc...) are ordered properly
  • punctuation marks are displayed properly.
  • mixing RTL and LTR works (the correct way, not Twitch's way)
  • reply messages are handled properly. will be another PR, we need to reorder words when creating the reply element due to Make reply thread subtext easier to click #4067
  • Selection do not work yet. won't fix for now, use (right click -> copy message) as a workaround.

Selection do not work yet.
punctuation marks do not work for some reason.
mixing RTL and LTR does seem to work.
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
@mohad12211
Copy link
Contributor Author

The last commit should show proper mixing between RTL and LTR, Twitch is doing it the wrong way actually.
If the text starts with an RTL word (e.g. Arabic), with LTR in the middle, it will only swap the RTL sequences, which is wrong, and a frustrating issue on Twitch... Should be working here. And it makes emotes show in the correct way as well.
Example:
image

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
@Felanbird
Copy link
Collaborator

The artifacts to test these changes can be found here, incase any issues can be found by native RTL speakers.

image

@EndlessJest
Copy link

thank you so much! Everything seems to work correctly so far.

@Felanbird
Copy link
Collaborator

Felanbird commented Sep 16, 2022

From a functionality standpoint, it appears these changes push the "space" when using large emotes to the bottom, instead of the top of the message. resolved

before:
image

after:
image

mohad12211 and others added 3 commits September 17, 2022 00:09
Since we are using LTR layout, non-RTL characters (for some reason
Arabic punctuation marks are considered non-RTL) will render in wrong
place (behind the word instead of in front of it, and vice versa)
The solution is to add an invisible Arabic letter mark before and after
the word.
Now Compact-Emotes and Zero-Width-Emotes work after the RTL ordering
@mohad12211
Copy link
Contributor Author

@Felanbird yes that was due to a weird thing I did, fixed after this refactor.

@mohad12211
Copy link
Contributor Author

mohad12211 commented Sep 16, 2022

I refactored the fix a bit.

What is done is basically this:
we call _addElement normally, if we detect an RTL word in the container, we reorder the elements.
we call addElement on each element again, without adding the element to this->elements_, if the text starts with an RTL word, we render from right to left (so currentX_ will decrease instead of increase), otherwise render normally since we are calling _addElement in the correct order. since we need the previous element for Compact-Emotes, I pass the index for the previous element, I pass -2 by default, so that in the first call we just do it the old way, the second call it will start from -1.
I think this is better than making two _addElement functions, one for RTL and one for LTR with 90% similar code.
let me know if I can do it in a better way.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
@Felanbird
Copy link
Collaborator

Felanbird commented Sep 17, 2022

image
Top portion is this PR - bottom is normal chatterino incorrect rendering resolved

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
src/util/Helpers.cpp Outdated Show resolved Hide resolved
@mohad12211
Copy link
Contributor Author

Everything should be working now except selection, copying the entire message works for now as an alternative to selection.
I will see what I can do about selection soon.
Please test this as much as possible, preferably with other RTL languages other than Arabic.
To download the app with this changes. go to Checks, at the top, then build, then at the bottom you will find binaries for all operating systems:
image
image

@mohad12211 mohad12211 marked this pull request as ready for review November 5, 2022 12:11
src/messages/layouts/MessageLayoutContainer.cpp Outdated Show resolved Hide resolved
src/util/Helpers.cpp Outdated Show resolved Hide resolved
mohad12211 and others added 6 commits November 10, 2022 19:50
due to Chatterino#4067 we should reorder the words when creating the element
neutral words which are between RTL words are treated like RTL
and need the U+061C mark for proper orientation
make variable static

Co-authored-by: pajlada <rasmus.karlsson@pajlada.com>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@mohad12211
Copy link
Contributor Author

ready for final review & merge.

@pajlada
Copy link
Member

pajlada commented Nov 10, 2022

Looks like this for me (left = your PR, right = master)
image

Testing with this text: a یک دو سه b

@pajlada
Copy link
Member

pajlada commented Nov 10, 2022

When switching to Segoe UI it looks correct to me

image

@pajlada
Copy link
Member

pajlada commented Nov 10, 2022

I attribute the previous errors I ran into to bad fonts on Linux

Copy link
Collaborator

@Felanbird Felanbird left a comment

Choose a reason for hiding this comment

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

As far as I believe I can test this as someone who does not speak/write an RTL language, I believe it works.
image

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Thank you!!

@pajlada pajlada enabled auto-merge (squash) November 10, 2022 20:13
@pajlada pajlada merged commit 3fcb7e1 into Chatterino:master Nov 10, 2022
@Felanbird
Copy link
Collaborator

@mohad12211 Thanks for your contribution! As a first-time contributor, you can now add yourself to the contributors list that's shown inside the application.

To do so, open a new PR where you modify the resources/contributors.txt file and add yourself to the list. Make sure to read the comments at a top for instructions.

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.

Problem with Languages using the Arabic Script - Hebrew and Arabic are reversed
5 participants