[FIX] Message_AllowedMaxSize fails for emoji sequences by c0dzilla · Pull Request #10431 · RocketChat/Rocket.Chat

c0dzilla · 2018-04-11T20:30:38Z

This pr ensures emojis are treated as single characters when determining if message size exceeds allowed limit, for better experience.

Taking same example as mentioned in issue, if max allowed size is 10

now works and has effective size 10.

cardoso · 2018-04-11T22:11:02Z

Is this working only for :shortname: ?

Or is it covering also the case where the user types the actual "👨‍👨‍👧‍👦" emoji instead of its :shortname: . From the iOS emoji keyboard for instance.

When I said emoji sequences I meant unicode sequences.

sampaiodiego

thanks @c0dzilla ..

looking at the code it is only taking care of :emojis: and not unicode ones.. can you please confirm that and make sure you have the same behavior for both (treating them as only one character)?

there is function called emojione.shortnameToUnicode that might help you.

c0dzilla · 2018-04-17T20:55:29Z

@sampaiodiego I've converted emoji unicodes to shortnames using toShort() while checking the length. I think it should handle that as well now.

cardoso · 2018-04-19T11:24:51Z

@c0dzilla @sampaiodiego we need a better solution than this. Converting back to shortname would imply that this solution only works for unicode graphemes with a corresponding shortname, which is very limiting. The iOS App for instance has many more emojis than in the Web App.

We need a generic solution for counting unicode graphemes. From what I've searched, it's non-trivial in JS and we may need a new dependency, but I think it's worth it.

rodrigok · 2018-05-18T14:24:24Z

Here is a possible solution https://stackoverflow.com/a/46085089

cardoso · 2018-05-22T22:28:19Z

+			// converting emoji unicodes to shortnames if present
+			let adjustedMessage = emojione.toShort(message.msg);
+
+			adjustedMessage = adjustedMessage.replace(/:\w+:/gm, (match) => {


Why is this part still needed? 🤔

graywolf336 · 2018-05-22T22:55:47Z

Why not use Array.from instead of underscore's?

cardoso · 2018-05-23T00:06:55Z

Also, I think Unit Tests will be a good idea here 👍

Testing cases like the one pointed out in the issue

"123456789👨‍👨‍👧‍👦" works with Message_AllowedMaxSize = 10

c0dzilla · 2018-05-23T08:19:27Z

After having a look at https://news.ycombinator.com/item?id=13830177, it seems that converting string to array will not always give the right length(like for ❤️ it gives length 2 instead of 1).

c0dzilla · 2018-05-31T14:22:28Z

@sampaiodiego @graywolf336 please have a look :)

ggazzo

I really liked your solution, but I got some points, please see if my questions make sense for you

ggazzo · 2018-06-12T01:16:15Z

+			}
+			return match;
+		});
+		return message && RocketChat.messageProperties(adjustedMessage).length > this.messageMaxSize;


this test on 'message' should not be done before?

Yeah, sorry about that 😅

ggazzo · 2018-06-12T01:18:13Z


 	isMessageTooLong(message) {
-		return message && message.length > this.messageMaxSize;
+		const adjustedMessage = message.replace(/:\w+:/gm, (match) => {


I think we have this replace in two places... maybe use an export/import? (:

…into max-msg-size-emoji

c0dzilla · 2018-06-12T17:49:30Z

@ggazzo I've made the changes ☺️

c0dzilla force-pushed the max-msg-size-emoji branch 2 times, most recently from 6a0d1fe to 0c9dfc2 Compare April 11, 2018 20:36

treats emojis as single characters for determining message size

d458112

c0dzilla force-pushed the max-msg-size-emoji branch from 0c9dfc2 to d458112 Compare April 11, 2018 20:43

theorenck added this to the 0.64.0 milestone Apr 11, 2018

theorenck requested a review from sampaiodiego April 11, 2018 22:31

sampaiodiego requested changes Apr 17, 2018

View reviewed changes

handles emoji unicodes while checking message size

e2c5d13

c0dzilla force-pushed the max-msg-size-emoji branch from 595f344 to e2c5d13 Compare April 17, 2018 20:18

RocketChat deleted a comment Apr 17, 2018

theorenck modified the milestones: 0.64.0, 0.65.0 Apr 23, 2018

rodrigok modified the milestones: 0.65.0, 0.66.0 May 18, 2018

rodrigok added the stat: board-postponed label May 21, 2018

c0dzilla added 2 commits May 23, 2018 02:24

Merge branch 'develop' into max-msg-size-emoji

1f0e634

uses underscore to calculate message length

160e45c

engelgabriel temporarily deployed to rocket-chat-pr-10431 May 22, 2018 22:25 Inactive

cardoso reviewed May 22, 2018

View reviewed changes

engelgabriel temporarily deployed to rocket-chat-pr-10431 May 23, 2018 08:19 Inactive

c0dzilla force-pushed the max-msg-size-emoji branch from d157709 to 61b6c28 Compare May 23, 2018 08:21

engelgabriel temporarily deployed to rocket-chat-pr-10431 May 23, 2018 08:21 Inactive

engelgabriel temporarily deployed to rocket-chat-pr-10431 May 30, 2018 13:20 Inactive

added tests

ba74537

c0dzilla force-pushed the max-msg-size-emoji branch from 99812f1 to ba74537 Compare May 30, 2018 13:40

engelgabriel temporarily deployed to rocket-chat-pr-10431 May 30, 2018 13:41 Inactive

Merge branch 'develop' into max-msg-size-emoji

67b68ca

engelgabriel temporarily deployed to rocket-chat-pr-10431 May 31, 2018 06:02 Inactive

engelgabriel temporarily deployed to rocket-chat-pr-10431 June 5, 2018 06:24 Inactive

Merge branch 'develop' into max-msg-size-emoji

1f3bd47

c0dzilla force-pushed the max-msg-size-emoji branch from 3f0119c to 1f3bd47 Compare June 5, 2018 14:05

engelgabriel temporarily deployed to rocket-chat-pr-10431 June 5, 2018 14:05 Inactive

Merge branch 'develop' into max-msg-size-emoji

0719032

engelgabriel temporarily deployed to rocket-chat-pr-10431 June 6, 2018 06:27 Inactive

Merge branch 'develop' into max-msg-size-emoji

1030c69

engelgabriel temporarily deployed to rocket-chat-pr-10431 June 11, 2018 20:49 Inactive

Merge branch 'develop' into max-msg-size-emoji

3f7c1d2

ggazzo temporarily deployed to rocket-chat-pr-10431 June 12, 2018 01:11 Inactive

ggazzo requested changes Jun 12, 2018

View reviewed changes

c0dzilla added 2 commits June 12, 2018 19:16

adds replace emoji shortnames to message properties

5d79284

Merge branch 'max-msg-size-emoji' of github.com:c0dzilla/Rocket.Chat …

ebf8b09

…into max-msg-size-emoji

engelgabriel temporarily deployed to rocket-chat-pr-10431 June 12, 2018 15:05 Inactive

engelgabriel temporarily deployed to rocket-chat-pr-10431 June 12, 2018 15:13 Inactive

engelgabriel temporarily deployed to rocket-chat-pr-10431 June 12, 2018 16:42 Inactive

type fix

2053ec1

c0dzilla force-pushed the max-msg-size-emoji branch from 728ca2f to 2053ec1 Compare June 12, 2018 16:51

engelgabriel deployed to rocket-chat-pr-10431 June 12, 2018 16:51 View deployment

ggazzo approved these changes Jun 12, 2018

View reviewed changes

ggazzo merged commit b681db6 into RocketChat:develop Jun 12, 2018

rodrigok mentioned this pull request Jun 28, 2018

Release 0.66.0 #11278

Merged

Uh oh!

Conversation

c0dzilla commented Apr 11, 2018

Uh oh!

cardoso commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sampaiodiego left a comment

Choose a reason for hiding this comment

Uh oh!

c0dzilla commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cardoso commented Apr 19, 2018

Uh oh!

rodrigok commented May 18, 2018

Uh oh!

cardoso May 22, 2018

Choose a reason for hiding this comment

Uh oh!

graywolf336 commented May 22, 2018

Uh oh!

cardoso commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

c0dzilla commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

c0dzilla commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggazzo left a comment

Choose a reason for hiding this comment

Uh oh!

ggazzo Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

c0dzilla Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

ggazzo Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

c0dzilla commented Jun 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

cardoso commented Apr 11, 2018 •

edited

Loading

c0dzilla commented Apr 17, 2018 •

edited

Loading

cardoso commented May 23, 2018 •

edited

Loading

c0dzilla commented May 23, 2018 •

edited

Loading

c0dzilla commented May 31, 2018 •

edited

Loading