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

Update wcwidth/CharWidth.ts #1707

Closed
wants to merge 10 commits into from
Closed

Update wcwidth/CharWidth.ts #1707

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fae8a19
Update wcwidth and add test
dnfield Sep 24, 2018
0f1955a
update tables
dnfield Sep 24, 2018
cfc1a58
Fix my test, add doc
dnfield Sep 24, 2018
48bd634
Fix wrapping
dnfield Sep 24, 2018
e807d85
remove unnecessary comment
dnfield Sep 24, 2018
a76907e
Revert "remove unnecessary comment"
dnfield Sep 24, 2018
1c33493
Revert "Fix wrapping"
dnfield Sep 24, 2018
acbca7a
update test
dnfield Sep 24, 2018
993b8c2
legacy option
dnfield Sep 24, 2018
2ae864b
redo exporting of wcwidth
dnfield Sep 25, 2018
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
2 changes: 1 addition & 1 deletion src/Buffer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,7 @@ describe('Buffer', () => {
// const s = terminal.buffer.contents(true).toArray()[0];
// assert.equal(input, s);
for (let i = 10; i < input.length; ++i) {
const bufferIndex = terminal.buffer.stringIndexToBufferIndex(0, i + 1); // TODO: remove +1 after fix
const bufferIndex = terminal.buffer.stringIndexToBufferIndex(0, i);
Copy link
Member

Choose a reason for hiding this comment

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

Please address this with a separate PR. The test case tested the wrong behavior by purpose to track it.

const j = (i - 0) << 1;
assert.deepEqual([(j / terminal.cols) | 0, j % terminal.cols], bufferIndex);
}
Expand Down
7 changes: 7 additions & 0 deletions src/CharWidth.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,5 +75,12 @@ describe('getStringCellWidth', function(): void {
assert.equal(input, s);
assert.equal(getStringCellWidth(s), sumWidths(terminal.buffer, 0, 1, '#'));
});
it('wide emojis', function(): void {
const input = '😀😀🌖🌖🍆🍆';
terminal.writeSync(input);
const s = terminal.buffer.iterator(true).next().content;
assert.equal(input, s);
assert.equal(getStringCellWidth(s), 12);
});
Copy link
Member

Choose a reason for hiding this comment

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

Due to #1685 you can temporarily fix your test case by placing a whitespace after every second emoji char.

Copy link
Author

Choose a reason for hiding this comment

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

This test wasn't breaking, it was another existing one ("Lots of makes me ." - smiley just happend to occur after a break in the buffer, leaving an extra space)

Copy link
Author

Choose a reason for hiding this comment

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

See https://github.com/xtermjs/xterm.js/pull/1707/files#diff-28532a8d9e0ba0fd03535deee418bce4R490

Copy link
Member

Choose a reason for hiding this comment

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

Yupp my bad, the line isnt wrapping (mixed cols and rows in my head).

// TODO: multiline tests once #1685 is resolved
});
Loading