-
-
Notifications
You must be signed in to change notification settings - Fork 458
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 workaround for combined emoji #3469
Implement workaround for combined emoji #3469
Conversation
Adding comment here since it's been kept in chat only for now: There's some issue with this on Windows only, some error with parsing the unicode regular expression or something |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it works on windows now 👍
Was this tested on macOS or should I take a look at it when I am back home tomorrow? |
I assume not so you can go ahead and test it 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works fine on macOS
Ok, I did some more testing and while the behaviour itself does not create any issues on macOS I found a general problem with outgoing whispers. Inside CommandController.cpp any message that is recognised as being a whisper command will be redirected and sent directly into sendMessage() of the underlying AbstractIrcServer, so the code for replacing \u200D inside of TwitchChannel.cpp will never get executed: chatterino2/src/controllers/commands/CommandController.cpp Lines 64 to 70 in dfa3818
That entire behaviour seems a bit odd to me, why even redirect the message in the first place, but no idea if there is an underlying reason for it. Anyhow, it breaks the implementation of this PR and needs to be addressed somehow. |
@Felanbird I made sending it work in sent whispers. Feel free to merge whenever! miniDank |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whispers are fixed on windows, and since this has been tested on all 3 OS I'm going to go ahead and merge it 👍
Oh also since we had a minor discussion about it, this PR unfortunately did not accidentally fix Arabic rendering. |
Now we're on commit 8200998; Changes from upstream we've pulled - Minor: Add workaround for multipart emoji as described in [the RFC](https://mm2pl.github.io/emoji_rfc.pdf). (Chatterino#3469)
Pull request checklist:
CHANGELOG.md
was updated, if applicableDescription
Closes #3384