Skip to content

#5310 Fixes for attachments which contain emojis in their names#5382

Merged
ioanmo226 merged 33 commits intomasterfrom
5310-attachment-emoji-live-test
Sep 8, 2023
Merged

#5310 Fixes for attachments which contain emojis in their names#5382
ioanmo226 merged 33 commits intomasterfrom
5310-attachment-emoji-live-test

Conversation

@sosnovsky
Copy link
Collaborator

@sosnovsky sosnovsky commented Aug 24, 2023

This PR fixes issues for attachments which contain emojis in their filenames

close #5310
close #5345


Tests (delete all except exactly one):

  • Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

Comment on lines +711 to +714
} else {
// regex from https://stackoverflow.com/a/69661174/3091318
encodedStr = encodedStr.replace(/(?![*#0-9]+)[\p{Emoji}\p{Emoji_Modifier}\p{Emoji_Component}\p{Emoji_Modifier_Base}\p{Emoji_Presentation}]/gu, '')
}
Copy link
Collaborator

@martgil martgil Aug 26, 2023

Choose a reason for hiding this comment

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

This is nice - I tried to find a reference for regex in Kotlin but I do not see any relevant documentation/reference material that Kotlin supports \p{Emoji_Presentation} or similar, unlike javascript which has rich support with it. So with that, we might end up using emoji-unicode ranges which again may contain an incomplete range of emojis.

@sosnovsky Do you allow me to fix both #5308 and #5309 by reusing the regex on line 713?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I tried to use the same regex on FES for allowing emojis, but unfortunately Kotlin doesn't support these \p keywords.

Let me finish this PR, then I'll check possible solution for #5308 and #5309 on FES side, and if there won't be a simple way to allow emojis and other symbols in recipient names - I'll notify you that you can work on stripping emojis from recipient names on browser extension side.

@sosnovsky sosnovsky marked this pull request as ready for review September 1, 2023 09:20
@sosnovsky sosnovsky requested a review from ioanmo226 September 1, 2023 09:20
@sosnovsky sosnovsky marked this pull request as draft September 1, 2023 11:30
@sosnovsky sosnovsky marked this pull request as ready for review September 8, 2023 11:36
@sosnovsky sosnovsky requested a review from ioanmo226 September 8, 2023 11:36
@ioanmo226 ioanmo226 merged commit b1db551 into master Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Attachment blocks are duplicated when filename contains non-alphanumeric symbols Unable to send attachment when filename contains emoji

2 participants