Skip to content

Conversation

@meganrogge
Copy link
Member

@meganrogge meganrogge commented Apr 19, 2022

fixes #3739

Based the range on this documentation

@meganrogge meganrogge self-assigned this Apr 19, 2022
@meganrogge meganrogge added this to the 4.19.0 milestone Apr 19, 2022
@meganrogge meganrogge requested a review from jerch April 19, 2022 19:53
@meganrogge
Copy link
Member Author

still a wip - need to test more

Copy link
Member

@jerch jerch left a comment

Choose a reason for hiding this comment

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

Nonetheless a few early remarks from my side:

I'd suggest not to use getAsCharData for several reasons:

  • outdated and should be removed (CharData was the cell type of the old bufferline impl, those methods only existed for interim transition, but never got cleaned up afterwards because of reasons 😊) - ah right, too convenient in tests 😸
  • the method creates an interim array for every tested cell (creating GC pressure for no good reason)

Maybe use cell.getCode() directly instead?

A note on powerline glyphs - most should reside in the Unicode's PUA (private use area), as you correctly test for. But if I remember right there are powerline-like fonts that also use other areas or even single codepoints all over the place (was it nerdfont? idk). Not sure how you want to deal with that, it is a bit like chasing a rabbit, and if you overdo it, you might end up excluding too much ...

@meganrogge
Copy link
Member Author

A note on powerline glyphs - most should reside in the Unicode's PUA (private use area), as you correctly test for. But if I remember right there are powerline-like fonts that also use other areas or even single codepoints all over the place (was it nerdfont? idk). Not sure how you want to deal with that, it is a bit like chasing a rabbit, and if you overdo it, you might end up excluding too much ...

Yes good point and I considered covering additional symbols, but I think that arrows are the main problem case as they look pretty bad ATM.

Screen Shot 2022-04-19 at 1 41 30 PM

@meganrogge meganrogge merged commit 6d80131 into xtermjs:master Apr 19, 2022
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.

exclude powerline characters from contrast ratio change

2 participants