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

Allow unsetting extending ansi colors #3912

Merged
merged 2 commits into from
Jul 19, 2022
Merged

Conversation

silamon
Copy link
Contributor

@silamon silamon commented Jul 19, 2022

Following up PR to address review comments in #3905.

@Tyriar Tyriar requested a review from jerch July 19, 2022 17:51
@Tyriar
Copy link
Member

Tyriar commented Jul 19, 2022

@jerch in case I missed anything 😛

@Tyriar Tyriar added this to the 4.20.0 milestone Jul 19, 2022
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.

LGTM 😺

There is just one thing idk for sure atm - when I was doing the OSC 4 implementation with hot color changes, there was an awkward color object reference thingy going on (not sufficiently re-grabbing colors from changes on individual slots). I am pretty sure I fixed it, still the issue kept me scratching my head back then.

So to be on the safe side - it might be good idea to explicitly check, whether a setTheme call works as before, and also that extended colors shift properly. The old issue only occured with canvas and webgl renderer.

@Tyriar
Copy link
Member

Tyriar commented Jul 19, 2022

Tested running these on each renderer and it worked as expected:

term.options.theme = { extendedAnsi: ['#ffffff']};

term.options.theme = { };

@Tyriar Tyriar merged commit 9997f26 into xtermjs:master Jul 19, 2022
@Tyriar Tyriar modified the milestones: 4.20.0, 5.0.0 Jul 28, 2022
@silamon silamon deleted the extendedcolors2 branch October 16, 2022 09:51
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.

3 participants