-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Selection should not change text color #720
Comments
I think we may be able to do that without splitting the dom into three layers - by using a css trick 🤓. I've created a simple example here: The idea is to wrap the text inside an inner span which has I believe this will also only have a minimal impact on the performance, compared to rendering and keeping separate background / foreground layers. |
@mofux I didn't think about this trick 😄 we would need to wait for #654 if we wanted to try this out. On the performance side, I'm thinking making them separate layers might actually end up being faster. The spans wrapping the text would need to be recreated every time the line changes, whereas for the new layer approach we could have x background elements that move/hide depending on the state. They would need to be present and have the layer trick even when there is no selection as when the renderer refreshes the viewport it does so without any knowledge of the selection. Another perk of separation is that we could delay background layer positioning/rendering like how the evaluating links is delayed (say 50ms, ~3 frames?). This means that for a command that spits out a bunch of output (with/without bg colors) we would only need to recalc and render the background when there is a small break in rendering, meaning frames are rendered faster and therefore output is processed faster. |
I may be overthinking things though 😉 |
I think the biggest advantage with the If I read the code correctly, we probably would have to optimise the Anyway, awesome work on the virtual selection! Just wanted to point out this alternate approach 😊 |
The background being created/drawn is less of a concern here, the bigger concern is creating these additional text layers for each
Yeah currently the object pool only goes 1 level deep, this needs some work and benchmarking to see if there are gains from going deeper 🙂 |
I'm slowly starting to understand how the screen buffer works, and why your approach might be smarter 😅 |
@mofux yeah it became a little complicated to get perf so good 😉. You can probably get some good insights by reading through some of the links here microsoft/vscode#17875 |
Regresses in #991 |
I was in zeit/hyper (issues/2934) wanting to change the color of selected text (like in the editor in VS Code) but have been looking into it all the afternoon and now that I see your struggle (since almost 2 years) it kind of stopped my enthusiasm I was about to fix it naively and send a pull request but now I feel like a child with a solution to adults problems ... I wanted just to send it and wait for your feedback but now that I know it could add 0.3 milliseconds [sic] to the rendering I feel like I am not qualified enough to be the one to find the solution ... |
@Luxcium this is actually one of the issues that should be fixed by #1790, it's been a while since I looked but I think once that's ready we should be able to change the DOM renderer to also show the selection under the text as the only reason we regressed here was because the "default" renderer became the canvas (2d) renderer. |
This issue was closed off as it's fixed in the experimental WebGL renderer addon. It's not ready yet but I'm closing this off so I don't forget about it. See this query for current webgl addon issues https://github.com/xtermjs/xterm.js/issues?q=is%3Aissue+is%3Aopen+label%3Aarea%2Faddon%2Fwebgl |
#719 made it so that the selection is on top in order to be able to show the selection on characters with their background set. Ideally we shouldn't use a transparent selection though and instead draw the characters on top of the selection. Perhaps we should pull out the rendering of the background colors so that rendering occurs like this:
The text was updated successfully, but these errors were encountered: