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.

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

Emoji can leave artifacts in right cell #1502

Closed
Tyriar opened this issue Jun 8, 2018 · 9 comments
Closed

Emoji can leave artifacts in right cell #1502

Tyriar opened this issue Jun 8, 2018 · 9 comments
Labels
type/bug Something is misbehaving

Comments

@Tyriar
Copy link
Member

Tyriar commented Jun 8, 2018

See microsoft/vscode#51385 (comment)

image

@jerch
Copy link
Member

jerch commented Jun 9, 2018

Two possible solutions come to my mind:

  • mark cell as overdrawn by left cell and therefore redraw it when left cell gets new content
  • scale bogus chars to fit in wcwidth * cells, no overdrawing at all

@Tyriar
Copy link
Member Author

Tyriar commented Jun 10, 2018

mark cell as overdrawn by left cell and therefore redraw it when left cell gets new content

I thought this was already supported, might have been broken in some cases recently?

@jerch
Copy link
Member

jerch commented Jun 11, 2018

There is the _isOverlapping test in the code (https://github.com/xtermjs/xterm.js/blob/master/src/renderer/TextRenderLayer.ts#L84), but to me it seems it is only applied when an overlapping char gets rendered but not when it gets removed (commented out?) Maybe @bgw can have a look at it?

@Tyriar
Copy link
Member Author

Tyriar commented Jun 11, 2018

Yeah I think it used to, if a cell is overlapping it must be drawn at the same time as its neighbouring cell, and if it's removed the neighbouring cell needs to be redrawn as well.

@jerch
Copy link
Member

jerch commented Jun 12, 2018

Read the original issue - the example the OP gave with the aubergines looks like it is wcwidth related, where the system wcwidth gives 2 and xterm.js's 1. This would explain why the OP can move the cursor further back into the prompt. I dont have Ubuntu 18 here - can someone with Ubuntu 18 test the system wcwidth for 🍆?

Edit:
On Ubuntu 14 it works as expected, also the overlapping is removed, so I am not sure if it is related to it.

@bgw
Copy link
Member

bgw commented Jun 12, 2018

To elaborate on what I think @Tyriar was trying to get at, we clear entire rows at a time:

this.clearCells(0, firstRow, terminal.cols, lastRow - firstRow + 1);

There should be one clearRect or fillRect call (depending on alpha) per frame.

can someone with Ubuntu 18 test the system wcwidth for 🍆?

I'm not on Ubuntu 18, but I'm on Debian Stable, which is a lot closer than Ubuntu 14. On my machine, wcwidth for 🍆 is 2:

selection_064

The program if anyone wants it for testing:

#define _XOPEN_SOURCE
#include <stdio.h>
#include <stddef.h>
#include <wchar.h>
#include <locale.h>

int main() {
    wchar_t wc = L'\U0001F346';
    setlocale(LC_ALL, "en_US.UTF-8");
    wprintf(L"%d, %lc\n", wcwidth(wc), wc);
}

I think this has to be a bug in the parser (maybe the wcwidth stuff). I don't see how it could happen from looking at the renderer.

@Tyriar
Copy link
Member Author

Tyriar commented Jun 12, 2018

we clear entire rows at a time

Oh I forgot about that 😅. I'm quite puzzled as to how this is happening then as the emoji would need to be drawn, but it looks like the class of bugs where the right is not "cleared" (which can't happen because we always clear and redraw the cells now).

@jerch
Copy link
Member

jerch commented Jun 12, 2018

@bgw Thanks for testing - yupp this is a bug in wcwidth, which returns 1 here. This is a case where the backend wcwidth and our wcwidth return different values for a specific char (see #1467 (comment) for the background). We can fix it by applying custom settings once #1470 is done.

@Tyriar
Copy link
Member Author

Tyriar commented Oct 7, 2019

I don't think this happens anymore.

@Tyriar Tyriar closed this as completed Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Something is misbehaving
Projects
None yet
Development

No branches or pull requests

3 participants