Emoji data storage improvements and custom emoji support #710

kaustubh-nair · 2020-07-06T18:47:33Z

The main points of this PR are:

Currently emoji data only has emoji names. This PR makes the emoji data more complete, by storing other properties like emoji code, and emoji type. They are necessary for sending emoji data while adding reactions to messages. Additionally, a distinction between default and custom emojis would be nice, in case we'd want this data in the future.
Adds support for custom emojis by fetching them from the server and storing them in the model. Since custom emojis are stored in the model, we might as well store the default ones too - instead of fetching emojis from two different places.
A bunch of variable/filename renaming commits to facilitate this change.
Adding zulip_extra_emoji to list of emojis

neiljp · 2020-07-08T23:56:57Z

@kaustubh-nair Based on comment in #zulip-terminal, ok to remove 'needs review', as other PRs are expected to come before this?

kaustubh-nair · 2020-07-09T02:25:10Z

@neiljp, this one of supposed to be merged before the others.

neiljp

@kaustubh-nair This is easy to read and review 👍 That makes it easier to give lots of feedback! This is mainly complex due to the types of emoji/reactions.

This PR has quite a bit of naming adjustment/improvement to allow for refactoring, which is good, but I wonder if we can improve those names - eg. emoji_data.py only has unicode emoji data in it, and model.emoji_data is only the active emoji.

One concern I have is that from rendering emoji (and/or reactions) we know that there is 'zulip extra' and 'custom' emoji (in addition to unicode emoji), yet you treat the server emoji as realm_emoji - the server doesn't appear to distinguish in the data response, so perhaps that's fine. Should we test/handle these differently, though? These don't seem to be exposed differently in the API call response, and maybe it doesn't matter, but the reactions data does contain this difference, so how should we be handling this?

tools/convert-emoji-data

tests/model/test_model.py

tests/conftest.py

tests/model/test_model.py

zulipterminal/model.py

neiljp

@kaustubh-nair Thanks for the update 👍

When I run the fetch/convert scripts on the branch, there seems to be a diff in unicode_emojis.py.

I didn't see the manual addition of the :zulip: emoji that needs to be added by each front-end yet?

Do you know if there are emoji events that should update the data we initially fetch?

For fetching the custom emoji, we could likely do this in parallel with the register and message-fetch at startup, but this seems fine as it is for now.

tests/conftest.py

zulipterminal/emoji_names.py

tools/convert-unicode-emoji-data

neiljp · 2020-08-10T20:36:05Z

tests/model/test_model.py

@@ -71,6 +75,7 @@ def test_init(self, model, initial_data, user_profile):
        assert model.unpinned_streams == []
        self.classify_unread_counts.assert_called_once_with(model)
        assert model.unread_counts == []
+        assert model.emoji_data == unicode_emojis


What is emoji_data? (you've changed to 'active_' below)

Fixed. Missed this between commits

tests/model/test_model.py

zulipterminal/model.py

kaustubh-nair · 2020-08-18T18:06:09Z

zulipterminal/model.py

        unicode_emoji_data = unicode_emojis.EMOJI_DATA
        for name, data in unicode_emoji_data.items():
            data['type'] = 'unicode_emoji'
+        typed_unicode_emoji_data = cast(NamedEmojiData, unicode_emoji_data)


This can't be done inline right?

You've modified the original, so this seems reasonable 👍

kaustubh-nair · 2020-08-18T21:31:56Z

@neiljp Thanks for the review! Updated PR according to comments.

When I run the fetch/convert scripts on the branch, there seems to be a diff in unicode_emojis.py.

I can't see a diff on the unicode data? Did you run this recently?

Do you know if there are emoji events that should update the data we initially fetch?

This has been discussed on czo before, the server needs to share emoji data using an endpoint - there aren't any events for updating this yet. EDIT - Are you talking about custom emojis?

neiljp · 2020-08-29T05:33:09Z

@kaustubh-nair This looks good and should be mergeable after a rebase :) As per my comments on czo, I was referring to using events from the server to ensure that the custom emojis are kept up to date. The only aspect we might consider prior to merging would be handling failing responses from the client call to fetch the emoji, which would fit into the same category as for get-messages and register.

The unicode data issue was fixed in one of your commits, from what I can tell :)

The emoji data in the server file has been updated, this commit syncs the emoji data saved from that file.

This makes the variable names more descriptive for when other emoji types, such as custom emojis, will have to be stored. * Rename emojis_fixture to unicode_emojis. * Rename emoji_names.py to unicode_emojis.py and rename the data stored in them to EMOJI_DATA. * Rename tools/convert-emoji-names to tools/convert-unicode-emoji-data and rename the I/O files used. * Rename tools/fetch-emoji-names to tools/fetch-unicode-emoji-data and rename output file created. * Rename test_emoji_names.py to test_emoji_data.py.

Save the data from unicode_emojis.py as a model attribute, instead of loading them directly from the file. This will make it easy, in the future, to use emoji data when it is fetched from different sources, for example - custom emojis that will be fetched using a server endpoint. Tests amended.

This commit stores the code and type of emojis in the emoji data stored in the model. This is important when other types of emojis are added. Emoji conversion script modified to store data in an OrderedDict.

* Fetch custom emojis from server which can be used by the user using typeahead. * Model tests amended. * New fixture and test added for custom emojis. Minor notes: * The emojis are being sorted twice here, once when unicode emojis are stored in the file using the script (which happens occasionally when it is manually updated) and again after custom emojis are fetched (which happens during every load). The first sorting seems pointless, at first glance, but it improves sorting time for the second one since sorted() uses Timsort that can perform better if subsequences of the data are already sorted. * A custom emoji will replace a unicode emoji if they have the same names.

This is a hard-coded emoji that replaces :zulip: emojis of other types. This emoji uses the zulip_extra_emoji type.

neiljp · 2020-10-16T17:41:59Z

@kaustubh-nair Thanks for the update! I just fixed some recent minor conflicts and merged manually 🎉

As I indicated in the stream, as part of the synchronizing of the realm emoji, we should likely move to treating the initial fetch as being from the initial data from the register, which I've filed as #809.

zulipbot added the size: XL [Automatic label added by zulipbot] label Jul 6, 2020

kaustubh-nair force-pushed the custom-emoji branch 2 times, most recently from 7e94ba0 to e8cda96 Compare July 6, 2020 19:19

kaustubh-nair added the PR needs review PR requires feedback to proceed label Jul 6, 2020

kaustubh-nair changed the title ~~Support custom emojis~~ Emoji data storage improvements and custom emoji support Jul 8, 2020

neiljp added this to the Release after upcoming milestone Jul 27, 2020

kaustubh-nair force-pushed the custom-emoji branch from e8cda96 to 3ed143e Compare July 29, 2020 16:54

kaustubh-nair force-pushed the custom-emoji branch 5 times, most recently from 1535d85 to a6813f4 Compare August 6, 2020 20:38

neiljp reviewed Aug 6, 2020

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label Aug 6, 2020

kaustubh-nair force-pushed the custom-emoji branch 4 times, most recently from a84ddfa to fee1a98 Compare August 9, 2020 20:48

kaustubh-nair added the PR needs review PR requires feedback to proceed label Aug 9, 2020

neiljp added the PR blocks other PR label Aug 9, 2020

zulipbot added the has conflicts label Aug 10, 2020

kaustubh-nair force-pushed the custom-emoji branch from 6fe2920 to af4e171 Compare August 10, 2020 18:19

zulipbot removed the has conflicts label Aug 10, 2020

kaustubh-nair commented Aug 10, 2020

View reviewed changes

zulipterminal/model.py Show resolved Hide resolved

kaustubh-nair force-pushed the custom-emoji branch 2 times, most recently from 9684f4a to 4008f26 Compare August 10, 2020 18:23

neiljp reviewed Aug 10, 2020

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label Aug 10, 2020

neiljp added the has conflicts label Aug 13, 2020

kaustubh-nair force-pushed the custom-emoji branch from 4008f26 to 166bb44 Compare August 17, 2020 18:34

zulipbot removed the has conflicts label Aug 17, 2020

kaustubh-nair force-pushed the custom-emoji branch from 166bb44 to 1afde76 Compare August 18, 2020 14:27

kaustubh-nair commented Aug 18, 2020

View reviewed changes

kaustubh-nair force-pushed the custom-emoji branch 2 times, most recently from df10ff7 to 4b2eb74 Compare August 18, 2020 21:27

kaustubh-nair added the PR needs review PR requires feedback to proceed label Aug 18, 2020

neiljp removed the PR needs review PR requires feedback to proceed label Aug 29, 2020

neiljp added has conflicts PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Sep 22, 2020

emoji_names: Sync emoji names with upstream.

9402272

The emoji data in the server file has been updated, this commit syncs the emoji data saved from that file.

kaustubh-nair force-pushed the custom-emoji branch from 4b2eb74 to 4896f2b Compare October 13, 2020 05:32

zulipbot removed the has conflicts label Oct 13, 2020

kaustubh-nair added 6 commits October 13, 2020 11:05

model/boxes/emojis: Store code and type for emojis.

1348ff7

This commit stores the code and type of emojis in the emoji data stored in the model. This is important when other types of emojis are added. Emoji conversion script modified to store data in an OrderedDict.

refactor: helper/model/emojis: Use TypedDict for emoji data.

ee19a8e

helper/model: Add :zulip: emoji to list of emojis.

644618c

This is a hard-coded emoji that replaces :zulip: emojis of other types. This emoji uses the zulip_extra_emoji type.

kaustubh-nair force-pushed the custom-emoji branch from 4896f2b to 644618c Compare October 13, 2020 05:39

kaustubh-nair added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Oct 13, 2020

neiljp added PR ready to be merged PR has been reviewed & is ready to be merged and removed PR needs review PR requires feedback to proceed labels Oct 14, 2020

neiljp mentioned this pull request Oct 16, 2020

Fetch realm emoji in initial register call & update using events #809

Closed

neiljp closed this Oct 16, 2020

Uh oh!

Emoji data storage improvements and custom emoji support #710

Emoji data storage improvements and custom emoji support #710

Uh oh!

Conversation

kaustubh-nair commented Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neiljp commented Jul 8, 2020

Uh oh!

kaustubh-nair commented Jul 9, 2020

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

neiljp Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

kaustubh-nair Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kaustubh-nair Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

neiljp Aug 29, 2020

Choose a reason for hiding this comment

Uh oh!

kaustubh-nair commented Aug 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neiljp commented Aug 29, 2020

Uh oh!

neiljp commented Oct 16, 2020

Uh oh!

Uh oh!

kaustubh-nair commented Jul 6, 2020 •

edited

Loading

kaustubh-nair commented Aug 18, 2020 •

edited

Loading