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

some text emoji not converting to emoji #9910

Closed
progserega opened this issue May 31, 2019 · 2 comments · Fixed by matrix-org/matrix-react-sdk#3070
Closed

some text emoji not converting to emoji #9910

progserega opened this issue May 31, 2019 · 2 comments · Fixed by matrix-org/matrix-react-sdk#3070
Labels
A-Emoji P2 S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect X-Regression

Comments

@progserega
Copy link

:) - success converted to emoji
:-) - not converted to emoji, but converted to space
:D - converted
:-D - not converted to emoji, but converted to space

https://riot.im/app
Version 1.2.0

In old version of Riot - all text emoji success converted to emoji.

@jryans jryans added S-Minor Impairs non-critical functionality or suitable workarounds exist P2 X-Regression A-Emoji labels May 31, 2019
@jryans
Copy link
Collaborator

jryans commented May 31, 2019

I believe this is also a difference caused by the new emoji data set we're now using after matrix-org/matrix-react-sdk#2995.

@npny
Copy link

npny commented Jun 5, 2019

I had a quick look into this:

  • The way the auto-replace works now is by looking inside node_modules/emojibase-data/en/compact.json (c.f. raw.json), for an exact match on the .emoticon field. (Relevant line)
    Looking for :) finds :pleased:, looking for :-) finds nothing.

  • Interestingly, there's also prioritisation of ascii emoji suggestions in the autocomplete (e.g. suggest :blissful: when you type :D). It is similar but works slightly differently, looking inside src/stripped-emoji.json for .aliases_ascii matches instead. (Relevant line)

  • It all works out in the end, because stripped-emoji.json is actually generated from compact.json by the emoji-data-strip.js script, so they end up with the same emoticon mapping

  • compact.json's .emoticon is a single string while stripped-emoji.json's .aliases_ascii is an array of strings. The latter makes more sense IMO, as you can have multiple ascii strings resolve to the same emoji, e.g. :) and :-) (mentioned in this issue), or :p and :P (mentioned in Missing :P emoji #9921). Perhaps even :c and :(, or :o and :0. But probably not :d and :D, or :'( and :(.

  • data/en/compact.json's path also seems to imply that different locales can have different ascii strings for an emoji. I'm not sure whether this really is that important, but then again I'm not an expert in emoji lexicography. The import is hardcoded to en/, so not an issue for now.

  • Ignoring the philosophical question of what ascii string should count as what emoji, it seems a solution for this particular issue would be to simply append a bunch of extra aliases_ascii inside stripped-emoji.json.
    Then one would need to get auto-replace to look at stripped-emoji.json (which we have control over) instead of compact.json (which we don't).
    Finally, we need to make sure emoji-data-strip.js works nicely with our customised stripped-emoji.json and doesn't overwrite it in the future.

  • Alternatively, we can decide that all of this is out of scope and that the ascii string for an emoji will be whatever emojibase says it is, custom mappings be damned.

At this point I don't know enough about how the whole system was intended to fit together (and about #9780) to make a call, so I just stopped there and decided to leave this here for reference.

Note: Switching the emoji picker/autocomplete to something else (#1107) may come with a more exhaustive mapping as a bonus, which the auto-replace can then use.

Note: Letting the user specify their own ascii->emoji mappings (https://github.com/vector-im/riot-web/issues/7505) may also be a more flexible solution to an imperfect default list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Emoji P2 S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect X-Regression
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants