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.

Sign up for GitHub

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 frequently used emojis list doesn't get updated when adding emojis by typing emoji code with colon #18396

Merged
merged 12 commits into from
May 12, 2023
Merged

Fix frequently used emojis list doesn't get updated when adding emojis by typing emoji code with colon #18396

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9448c83
remove frequent emoji list onyx connection
bernhardoj May 4, 2023
4f94c44
connect with frequent emojis list onyx and update the list when emoji…
bernhardoj May 4, 2023
b0eeb72
improve the logic
bernhardoj May 5, 2023
f1184b8
fix can't add new emoji to the list
bernhardoj May 6, 2023
a9e689e
add test
bernhardoj May 6, 2023
9b780a5
fix typo
bernhardoj May 6, 2023
3490d46
add comment
bernhardoj May 6, 2023
1efc145
Merge branch 'main' into fix/17599-no-frequent-emoji-update-by-typing…
bernhardoj May 8, 2023
a414ef0
use the old logic back but with looping
bernhardoj May 8, 2023
c277e87
using js native sort as lodash is much slower
bernhardoj May 8, 2023
9521a0f
Merge branch 'main' into fix/17599-no-frequent-emoji-update-by-typing…
bernhardoj May 11, 2023
4c8af67
prettier
bernhardoj May 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 3 additions & 13 deletions src/components/EmojiPicker/EmojiPickerMenu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,6 @@ const propTypes = {
/** Stores user's preferred skin tone */
preferredSkinTone: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),

/** User's frequently used emojis */
frequentlyUsedEmojis: PropTypes.arrayOf(PropTypes.shape({
code: PropTypes.string.isRequired,
keywords: PropTypes.arrayOf(PropTypes.string),
})),

/** Props related to the dimensions of the window */
...windowDimensionsPropTypes,

Expand All @@ -47,7 +41,6 @@ const propTypes = {
const defaultProps = {
forwardedRef: () => {},
preferredSkinTone: CONST.EMOJI_DEFAULT_SKIN_TONE,
frequentlyUsedEmojis: [],
};

class EmojiPickerMenu extends Component {
Expand All @@ -64,8 +57,8 @@ class EmojiPickerMenu extends Component {
// since Windows doesn't support them
const flagHeaderIndex = _.findIndex(emojis, emoji => emoji.header && emoji.code === 'flags');
this.emojis = getOperatingSystem() === CONST.OS.WINDOWS
? EmojiUtils.mergeEmojisWithFrequentlyUsedEmojis(emojis.slice(0, flagHeaderIndex), this.props.frequentlyUsedEmojis)
: EmojiUtils.mergeEmojisWithFrequentlyUsedEmojis(emojis, this.props.frequentlyUsedEmojis);
? EmojiUtils.mergeEmojisWithFrequentlyUsedEmojis(emojis.slice(0, flagHeaderIndex))
: EmojiUtils.mergeEmojisWithFrequentlyUsedEmojis(emojis);

// Get the header emojis along with the code, index and icon.
// index is the actual header index starting at the first emoji and counting each one
Expand Down Expand Up @@ -234,7 +227,7 @@ class EmojiPickerMenu extends Component {
* @param {Object} emojiObject
*/
addToFrequentAndSelectEmoji(emoji, emojiObject) {
EmojiUtils.addToFrequentlyUsedEmojis(this.props.frequentlyUsedEmojis, emojiObject);
EmojiUtils.addToFrequentlyUsedEmojis(emojiObject);
this.props.onEmojiSelected(emoji, emojiObject);
}

Expand Down Expand Up @@ -569,9 +562,6 @@ export default compose(
preferredSkinTone: {
key: ONYXKEYS.PREFERRED_EMOJI_SKIN_TONE,
},
frequentlyUsedEmojis: {
key: ONYXKEYS.FREQUENTLY_USED_EMOJIS,
},
}),
)(React.forwardRef((props, ref) => (
// eslint-disable-next-line react/jsx-props-no-spreading
Expand Down
14 changes: 2 additions & 12 deletions src/components/EmojiPicker/EmojiPickerMenu/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,6 @@ const propTypes = {
/** Stores user's preferred skin tone */
preferredSkinTone: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),

/** User's frequently used emojis */
frequentlyUsedEmojis: PropTypes.arrayOf(PropTypes.shape({
code: PropTypes.string.isRequired,
keywords: PropTypes.arrayOf(PropTypes.string),
})),

/** Props related to the dimensions of the window */
...windowDimensionsPropTypes,

Expand All @@ -40,7 +34,6 @@ const propTypes = {

const defaultProps = {
preferredSkinTone: CONST.EMOJI_DEFAULT_SKIN_TONE,
frequentlyUsedEmojis: [],
};

class EmojiPickerMenu extends Component {
Expand All @@ -50,7 +43,7 @@ class EmojiPickerMenu extends Component {
// Ref for emoji FlatList
this.emojiList = undefined;

this.emojis = EmojiUtils.mergeEmojisWithFrequentlyUsedEmojis(emojis, this.props.frequentlyUsedEmojis);
this.emojis = EmojiUtils.mergeEmojisWithFrequentlyUsedEmojis(emojis);

// Get the header emojis along with the code, index and icon.
// index is the actual header index starting at the first emoji and counting each one
Expand All @@ -77,7 +70,7 @@ class EmojiPickerMenu extends Component {
* @param {Object} emojiObject
*/
addToFrequentAndSelectEmoji(emoji, emojiObject) {
EmojiUtils.addToFrequentlyUsedEmojis(this.props.frequentlyUsedEmojis, emojiObject);
EmojiUtils.addToFrequentlyUsedEmojis(emojiObject);
this.props.onEmojiSelected(emoji, emojiObject);
}

Expand Down Expand Up @@ -190,9 +183,6 @@ export default compose(
preferredSkinTone: {
key: ONYXKEYS.PREFERRED_EMOJI_SKIN_TONE,
},
frequentlyUsedEmojis: {
key: ONYXKEYS.FREQUENTLY_USED_EMOJIS,
},
}),
)(React.forwardRef((props, ref) => (
// eslint-disable-next-line react/jsx-props-no-spreading
Expand Down
78 changes: 55 additions & 23 deletions src/libs/EmojiUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,22 @@
import _ from 'underscore';
import lodashSumBy from 'lodash/sumBy';
import lodashMaxBy from 'lodash/maxBy';
import lodashOrderBy from 'lodash/orderBy';
import moment from 'moment';
import Str from 'expensify-common/lib/str';
import Onyx from 'react-native-onyx';
import ONYXKEYS from '../ONYXKEYS';
import CONST from '../CONST';
import * as User from './actions/User';
import emojisTrie from './EmojiTrie';
import FrequentlyUsed from '../../assets/images/history.svg';

let frequentlyUsedEmojis = [];
Onyx.connect({
key: ONYXKEYS.FREQUENTLY_USED_EMOJIS,
callback: val => frequentlyUsedEmojis = val,
bernhardoj marked this conversation as resolved.
Show resolved Hide resolved
});

/**
* Get the unicode code of an emoji in base 16.
* @param {String} input
Expand Down Expand Up @@ -139,10 +149,9 @@ function addSpacesToEmojiCategories(emojis) {
/**
* Get a merged array with frequently used emojis
* @param {Object[]} emojis
* @param {Object[]} frequentlyUsedEmojis
* @returns {Object[]}
*/
function mergeEmojisWithFrequentlyUsedEmojis(emojis, frequentlyUsedEmojis = []) {
function mergeEmojisWithFrequentlyUsedEmojis(emojis) {
if (frequentlyUsedEmojis.length === 0) {
return addSpacesToEmojiCategories(emojis);
}
Expand All @@ -159,30 +168,38 @@ function mergeEmojisWithFrequentlyUsedEmojis(emojis, frequentlyUsedEmojis = [])

/**
* Update the frequently used emojis list by usage and sync with API
* @param {Object[]} frequentlyUsedEmojis
* @param {Object} newEmoji
* @param {Object|Object[]} newEmoji
*/
function addToFrequentlyUsedEmojis(frequentlyUsedEmojis, newEmoji) {
let frequentEmojiList = frequentlyUsedEmojis;
let currentEmojiCount = 1;
function addToFrequentlyUsedEmojis(newEmoji) {
bernhardoj marked this conversation as resolved.
Show resolved Hide resolved
const currentTimestamp = moment().unix();
const emojiIndex = _.findIndex(frequentEmojiList, e => e.code === newEmoji.code);
if (emojiIndex >= 0) {
currentEmojiCount = frequentEmojiList[emojiIndex].count + 1;
frequentEmojiList.splice(emojiIndex, 1);
}
const updatedEmoji = {...newEmoji, ...{count: currentEmojiCount, lastUpdatedAt: currentTimestamp}};
const maxFrequentEmojiCount = (CONST.EMOJI_FREQUENT_ROW_COUNT * CONST.EMOJI_NUM_PER_ROW) - 1;
const maxFrequentEmojiCount = (CONST.EMOJI_FREQUENT_ROW_COUNT * CONST.EMOJI_NUM_PER_ROW);

// Get unique emojis array with counts for every emoji, these emojis are usually extracted from copy/pasted comments
const uniqueEmojisWithCounts = _.chain([].concat(newEmoji))
.groupBy('name')
.map(values => ({
...values[0],
count: values.length,
lastUpdatedAt: currentTimestamp,
}))
.value();

// Concat uniqueEmojisWithCounts with the frequentlyUsedEmojis where we sum the count and update the lastUpdatedAt
const mergedEmojisWithFrequent = _.chain(uniqueEmojisWithCounts)
.concat(frequentlyUsedEmojis)
.groupBy('name')
.map(groupedEmojiList => ({
...groupedEmojiList[0],
count: lodashSumBy(groupedEmojiList, 'count'),
lastUpdatedAt: lodashMaxBy(groupedEmojiList, 'lastUpdatedAt').lastUpdatedAt,
}))
.value();

// We want to make sure the current emoji is added to the list
// Hence, we take one less than the current high frequent used emojis and if same then sorted by lastUpdatedAt
frequentEmojiList = lodashOrderBy(frequentEmojiList, ['count', 'lastUpdatedAt'], ['desc', 'desc']);
frequentEmojiList = frequentEmojiList.slice(0, maxFrequentEmojiCount);
frequentEmojiList.push(updatedEmoji);
// Sort the list and take the first maxFrequentEmojiCount items
const frequentEmojiListOrdered = lodashOrderBy(mergedEmojisWithFrequent, ['count', 'lastUpdatedAt'], ['desc', 'desc'])
.slice(0, maxFrequentEmojiCount);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bernhardoj Thanks for the update. While testing, I noticed that we missed the case where we select a new emoji from the emoji picker. The newly selected emoji should appear on the frequent list. However, the above changes do not account for that case. Therefore, we need to fix it. Here is a simple fix:

Suggested change
const frequentEmojiListOrdered = lodashOrderBy(mergedEmojisWithFrequent, ['count', 'lastUpdatedAt'], ['desc', 'desc'])
.slice(0, maxFrequentEmojiCount);
let frequentEmojiListOrdered = lodashOrderBy(mergedEmojisWithFrequent, ['count', 'lastUpdatedAt'], ['desc', 'desc']);
if (uniqueEmojisWithCounts.length === 1 && _.last(frequentEmojiListOrdered).name === uniqueEmojisWithCounts[0].name) {
frequentEmojiListOrdered.splice(maxFrequentEmojiCount, 1);
} else {
frequentEmojiListOrdered = frequentEmojiListOrdered.slice(0, maxFrequentEmojiCount);
}

Copy link
Contributor Author

@bernhardoj bernhardoj May 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works fine for me though. Can you share your repro steps?

Screen.Recording.2023-05-06.at.09.00.57.mov

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bernhardoj Your emojis should have count > 1 , because we are ordering the emojis by count, if the newly added is the last in the list , it will be removed , check explanation

emoji.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not aware the video I attached is a link. updated.

The emoji I add is a new emoji (the monkey emoji), not an emoji with count > 1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think I understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. That's what will happen with the new behavior. Updated my comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which means with the new behavior, no matter how many times we insert the new emoji, it won't be added to the list (because the count is always 0).

I'm not sure if we're on the same page. Which count are you referring to? The current behavior is to extract emojis from the copy/pasted comment, group them by emoji name, and assign each emoji a count that corresponds to the number of occurrences in the comment. Then, we merge this list with the old frequently used emojis list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The count on the frequent emoji list. Let say all 24 emoji has count of 2. Then, we add a new emoji. Because it doesn't exist in the list, it will have a count of 1. With our new logic, we won't take the new emoji. The next time we add the same emoji, the count will still be 1 because it doesn't exist in the list.

Copy link
Contributor

@fedirjh fedirjh May 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bernhardoj That’s why I suggested these changes here , If the last emoji in the list is the inserted emoji then We shouldn’t remove it , and we remove the previous emoji.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to solve the issue. Also added a test.


// Second sorting is required so that new emoji is properly placed at sort-ordered location
frequentEmojiList = lodashOrderBy(frequentEmojiList, ['count', 'lastUpdatedAt'], ['desc', 'desc']);
User.updateFrequentlyUsedEmojis(frequentEmojiList);
User.updateFrequentlyUsedEmojis(frequentEmojiListOrdered);
}

/**
Expand All @@ -204,6 +221,9 @@ const getEmojiCodeWithSkinColor = (item, preferredSkinToneIndex) => {
/**
* Replace any emoji name in a text with the emoji icon.
* If we're on mobile, we also add a space after the emoji granted there's no text after it.
*
* All replaced emojis will be added to the frequently used emojis list.
*
* @param {String} text
* @param {Boolean} isSmallScreenWidth
* @param {Number} preferredSkinTone
Expand All @@ -215,10 +235,17 @@ function replaceEmojis(text, isSmallScreenWidth = false, preferredSkinTone = CON
if (!emojiData || emojiData.length === 0) {
bernhardoj marked this conversation as resolved.
Show resolved Hide resolved
return text;
}
const emojis = [];
for (let i = 0; i < emojiData.length; i++) {
const checkEmoji = emojisTrie.search(emojiData[i].slice(1, -1));
const name = emojiData[i].slice(1, -1);
const checkEmoji = emojisTrie.search(name);
if (checkEmoji && checkEmoji.metaData.code) {
let emojiReplacement = getEmojiCodeWithSkinColor(checkEmoji.metaData, preferredSkinTone);
emojis.push({
name,
code: checkEmoji.metaData.code,
types: checkEmoji.metaData.types,
});

// If this is the last emoji in the message and it's the end of the message so far,
// add a space after it so the user can keep typing easily.
Expand All @@ -228,6 +255,11 @@ function replaceEmojis(text, isSmallScreenWidth = false, preferredSkinTone = CON
newText = newText.replace(emojiData[i], emojiReplacement);
}
}

// Add all replaced emojis to the frequently used emojis list
if (!_.isEmpty(emojis)) {
addToFrequentlyUsedEmojis(emojis);
}
return newText;
}

Expand Down
12 changes: 1 addition & 11 deletions src/pages/home/report/ReportActionCompose.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,6 @@ const propTypes = {
/** Stores user's preferred skin tone */
preferredSkinTone: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),

/** User's frequently used emojis */
frequentlyUsedEmojis: PropTypes.arrayOf(PropTypes.shape({
code: PropTypes.string.isRequired,
keywords: PropTypes.arrayOf(PropTypes.string),
})),

/** The type of action that's pending */
pendingAction: PropTypes.oneOf(['add', 'update', 'delete']),

Expand All @@ -137,7 +131,6 @@ const defaultProps = {
blockedFromConcierge: {},
personalDetails: {},
preferredSkinTone: CONST.EMOJI_DEFAULT_SKIN_TONE,
frequentlyUsedEmojis: [],
isComposerFullSize: false,
pendingAction: null,
...withCurrentUserPersonalDetailsDefaultProps,
Expand Down Expand Up @@ -519,7 +512,7 @@ class ReportActionCompose extends React.Component {
},
suggestedEmojis: [],
}));
EmojiUtils.addToFrequentlyUsedEmojis(this.props.frequentlyUsedEmojis, emojiObject);
EmojiUtils.addToFrequentlyUsedEmojis(emojiObject);
}

isEmptyChat() {
Expand Down Expand Up @@ -1045,9 +1038,6 @@ export default compose(
blockedFromConcierge: {
key: ONYXKEYS.NVP_BLOCKED_FROM_CONCIERGE,
},
frequentlyUsedEmojis: {
key: ONYXKEYS.FREQUENTLY_USED_EMOJIS,
},
preferredSkinTone: {
key: ONYXKEYS.PREFERRED_EMOJI_SKIN_TONE,
},
Expand Down