Fix emoji width #1951
Closed
Fix emoji width #1951
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
Closed
epicfaace
reviewed
Feb 25, 2019
| @@ -76,6 +76,20 @@ describe('getStringCellWidth', function(): void { | |||
| assert.equal(getStringCellWidth(s), sumWidths(terminal.buffer, 0, 1, '#')); | |||
| }); | |||
| // TODO: multiline tests once #1685 is resolved | |||
| it('emojis', function(): void { | |||
| const input = '😁😁😁😁😁😁#'; | |||
There was a problem hiding this comment.
I've never seen emojis in code before haha this is great
There was a problem hiding this comment.
I've never seen emojis in code before haha this is great
Actually there were a few and the failed test is one of them😂
|
Yes thats part of #1714. Unless we properly respect different Unicode versions we cannot make this work reliably. I have to vote against this patch here, as it only covers some chars while others are left out. Furthermore this will break with systems, that still use a lower Unicode version, like Ubuntu 16 or older OSX versions (before High Sierra). |
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.
Problem
As described in #469 (comment), emoji width is 1 when added then displayed in half, and delete width is 2 when it is deleted which caused character in the left is deleted by mistake.
Conditions
\u55357+\u56834\u10060.\u{200D}, so'👩❤️💋👩'.length === 11, when copy & paste in Chrome address, it becomes 👩 ❤️ 💋 👩 separated by\u8205This only fix emoji width, IME support is WIP here #1890
Solution
Single 'string' width is calculated by
wcwidthwhich doesn't consider emojis.By extending
getStringCellWidth, it can calculate both single 'string' and normal string width, containing eitherwcwidthstanding characters and unicode emojis. Terminal should usegetStringCellWidthother thanwcwidthto cover emoji width calculation.References
https://mathiasbynens.be/notes/javascript-unicode
https://blog.jonnew.com/posts/poo-dot-length-equals-two
Testing
Buffer stringIndexToBufferIndex fullwidth combining with emoji - match emoji cell. 'Lots of ￥\u0301 make me 😃.' is 3 lines interminal.buffer.lines.get(some_line_index)like: