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 workaround for combined emoji #3469

Merged
merged 16 commits into from Jan 11, 2022

Conversation

Mm2PL
Copy link
Collaborator

@Mm2PL Mm2PL commented Jan 5, 2022

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Closes #3384

@Mm2PL Mm2PL requested a review from pajlada January 5, 2022 17:40
CHANGELOG.md Outdated Show resolved Hide resolved
@pajlada
Copy link
Member

pajlada commented Jan 6, 2022

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

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.

it works on windows now 👍

@Mm2PL Mm2PL requested a review from pajlada January 8, 2022 22:05
@Felanbird
Copy link
Collaborator

Incoming whispers doesn't work on windows so nobody randomly merge this :)

image
🤏 windows

@LosFarmosCTL
Copy link
Contributor

Was this tested on macOS or should I take a look at it when I am back home tomorrow?

@Felanbird
Copy link
Collaborator

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 🙂

Copy link
Contributor

@LosFarmosCTL LosFarmosCTL left a 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

@LosFarmosCTL
Copy link
Contributor

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:

void sendWhisperMessage(const QString &text)
{
// (hemirt) pajlada: "we should not be sending whispers through jtv, but
// rather to your own username"
auto app = getApp();
app->twitch.server->sendMessage("jtv", text.simplified());
}

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.

@Mm2PL
Copy link
Collaborator Author

Mm2PL commented Jan 10, 2022

@Felanbird I made sending it work in sent whispers. Feel free to merge whenever! miniDank

Incoming whispers work too.
2022-01-10_21-34

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.

Whispers are fixed on windows, and since this has been tested on all 3 OS I'm going to go ahead and merge it 👍

@Felanbird
Copy link
Collaborator

Oh also since we had a minor discussion about it, this PR unfortunately did not accidentally fix Arabic rendering.

@Felanbird Felanbird enabled auto-merge (squash) January 10, 2022 23:45
@Felanbird Felanbird merged commit 8200998 into Chatterino:master Jan 11, 2022
@zneix zneix deleted the feature/fix_combined_emoji_rfc branch January 12, 2022 00:59
zneix pushed a commit to SevenTV/chatterino7 that referenced this pull request Jan 12, 2022
zneix added a commit to SevenTV/chatterino7 that referenced this pull request Jan 12, 2022
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)
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.

Fixing combined emoji
5 participants