Exclude deactivated realm emoji from autocomplete #1114
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Very useful when these objects appear in test failures.
It's a bit of a trap for the unwary (... like me last month, in ecd2cb5, causing zulip#1113), because it includes deactivated realm emoji as well as active ones.
This way the binding encapsulates an important fact about this bit of the API -- namely that these values are the same as the "emoji code" values seen elsewhere in the API -- so that we don't have to re-verify it when working elsewhere in the codebase and using this class. Also edit a doc comment in the model code to take advantage of that understanding.
I spent some time reverse-engineering in full detail the behavior web has for building its list of possible emoji to present in the UI (and so for the order that emoji results have when they don't differ on any of the criteria used at the ranking step after applying a query). The first output of that study was discovering zulip#1113, fixed earlier in this branch. This records the rest.
|
Thanks! LGTM, merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1113.
The first commit (toString on some emoji-related data classes) is shared with #1112.
When combining with #1112, the three new test cases will each need a tweak like this one:
check(store.allEmojiCandidates()).deepEquals([ + ...arePopularCandidates,just like #1112 itself does (in its last commit) for several existing test cases.
Selected commit messages
2c276b8 emoji [nfc]: Make realmEmoji internal to EmojiStoreImpl
It's a bit of a trap for the unwary (... like me last month, in
ecd2cb5, causing #1113), because it includes deactivated realm emoji
as well as active ones.
e564806 api [nfc]: Rename RealmEmojiItem.id to emojiCode
This way the binding encapsulates an important fact about this
bit of the API -- namely that these values are the same as the
"emoji code" values seen elsewhere in the API -- so that we don't
have to re-verify it when working elsewhere in the codebase and
using this class.
Also edit a doc comment in the model code to take advantage of
that understanding.
f09aab1 emoji: Exclude deactivated realm emoji from autocomplete
Fixes #1113.