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

Fix ReactionType::Unicode() to only accept valid emojis as types #953

Closed

Conversation

Prof-Bloodstone
Copy link
Contributor

Previously, to parse String into ReactionType, as long as the string wasn't empty and didn't start with '<', it was assumed to be valid ReactionType::Unicode emoji

Please note that I'm relatively new to Rust, so possible that I did it completely wrong. Feedback would be greatly appreciated.

I didn't want to break the API, but from my knowledge it's not possible to keep it as it was. Would love to be proved wrong.

TODO:

  • Fix tests
  • Add tests
  • Check formatting
  • Rebase onto next branch?

Previously, to parse String into ReactionType, as long as the string wasn't empty and didn't start with '<', it was assumed to be valid ReactionType::Unicode emoji

This causes Discord to return 400 and apparently also rate limits
@arqunis arqunis added enhancement An improvement to Serenity. model Related to the `model` module. labels Sep 6, 2020
@Prof-Bloodstone
Copy link
Contributor Author

What could be an alternative, which doesn't break API and provides new convenience methods, would be something like:

use serenity::model::channel::{ReactionType, ReactionConversionError};
use serenity::static_assertions::_core::convert::TryFrom;
use unic_emoji_char::is_emoji;


pub trait AsEmoji {
    fn as_emoji(&self) -> Result<ReactionType, ReactionConversionError>;
}

impl AsEmoji for &str {
    fn as_emoji(&self) -> Result<ReactionType, ReactionConversionError> {
        return self.to_string().as_emoji();
    }
}

impl AsEmoji for String {
    fn as_emoji(&self) -> Result<ReactionType, ReactionConversionError> {
        return ReactionType::try_from((*self).clone()).and_then( |reaction| {
            match &reaction {
                ReactionType::Custom { animated: _, id: _, name: _} => Ok(reaction),
                ReactionType::Unicode(string) => {
                    let chars = string.chars().collect::<Vec<char>>();
                    return if chars.len() != 1 || !is_emoji(chars[0].clone()) {
                        Err(ReactionConversionError)
                    } else {
                        Ok(reaction)
                    }
                },
                ReactionType::__Nonexhaustive => unreachable!(),
            }
        });
    }
}

Probably different naming, because I feel like as_<TYPE> should return that type, and not a Result

But I feel like it's just a rather temporary workaround, so not sure if it's worth to put it if the proper fix will need to break the API later.

@Prof-Bloodstone
Copy link
Contributor Author

We can't do that until unic_emoji_char crate supports Unicode 0.11 or newer (see open-i18n/rust-unic#259 and open-i18n/rust-unic#260)

This is because of a bug in the Unicode 0.5 Emoji standard itself, which is reflected in the crate.

So the 5.0 standard defines 1️⃣ as 0031 FE0F 20E3; Emoji_Keycap_Sequence ; keycap: 1 : http://unicode.org/Public/emoji/5.0/emoji-sequences.txt
But neither of the later 2 characters are deemed Components: http://unicode.org/Public/emoji/5.0/emoji-data.txt (At the bottom)
Despite being explicitely mentioned that they should be components: https://www.unicode.org/reports/tr51/tr51-12.html#Emoji_Properties

They are included in the data in the very next standard revision: https://www.unicode.org/Public/emoji//11.0/emoji-data.txt

@Prof-Bloodstone Prof-Bloodstone deleted the fix_unicode_emoji branch October 25, 2020 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement to Serenity. model Related to the `model` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants