-
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
DOM: Render selection color instead of cell bg #4673
Conversation
Summary: - Selection cell reuse has been added to the DOM renderer - Selection bg override is now correctly set correctly - Selection elements 'pretend' to be a decoration in order to render bg Fixes xtermjs#4097
Merging so I can get some testing in VS Code, @jerch let me know if something looks off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh well, that looks complicated. Please see my comments below.
&& ( | ||
(isInSelection && oldIsInSelection) | ||
|| (!isInSelection && cell.bg === oldBg) | ||
) | ||
&& ( | ||
(isInSelection && oldIsInSelection && colors.selectionForeground) | ||
|| (!(isInSelection && oldIsInSelection && colors.selectionForeground) && cell.fg === oldFg) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm this looks quite complicated - I guess you have tested it, so I assume it good as it is (though I wonder if it could be simplified). Which selection state tried you to cover here, which was not by the old conditions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we would just never merge cells if it was a selection which is easy but that means select all would all be the old slow rendering.
Outside of pulling things into named variables or expanding the comment I'm not sure how to simplify this, these cases are described in the comment above:
- bg did not change (or both are in selection)
- fg did not change (or both are in selection and selection fg is set)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... but that means select all would all be the old slow rendering.
Yes very true. Btw the same applies to isDecorated
. Yeah, I kinda skipped dealing with inner works of selections and decorations, as it is more on the rare side of things. But indeed, you can actually feel the difference in responsiveness when selecting the full viewport and try to scroll then - so its good you address that.
@@ -417,7 +431,7 @@ export class DomRendererRowFactory { | |||
} | |||
|
|||
// exclude conditions for cell merging - never merge these | |||
if (!isCursorCell && !isInSelection && !isJoined && !isDecorated) { | |||
if (!isCursorCell && !isJoined && !isDecorated && isInSelection === oldIsInSelection) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, if isInSelection === oldIsInSelection
makes much sense here - conditions here are late evals for the first span, and whether it qualifies for merging with follow-up chars.
I lazily hacked !isInSelection
in here, which is technically too strict. Normally selections can also merge, if the edge conditions are all met, which I was not sure about, thus the shortcut.
Now your isInSelection === oldIsInSelection
matches the same value with itself, because of line:206?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you're right this does nothing 🙈
We may just be able to remove that condition completely then as this seemed to work well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary:
Fixes #4097