-
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
Underline styles and colors - core part #2751
Conversation
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.
Looks good after the minor issues are resolved 👍, next PR would be adding rendering?
isUnderlineColorRGB(): boolean; | ||
isUnderlineColorPalette(): boolean; | ||
isUnderlineColorDefault(): boolean; | ||
getUnderlineStyle(): number; |
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.
Just to clarify, all underline styles will still result in the isUnderline
public API being non-zero?
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.
The idea here for renderer side is to first test for isUnderline
, if that evals to true
, you'd have to request the style and the color to finally draw it. This way it is compatible to our current code, which only relies on a isUnderline
test and always draws a single line.
The color methods cannot be used to determine whether underline is in place, as they would always report a color (fg color). The style method can be used for that, yes.
public content = 0; | ||
public fg = 0; | ||
public bg = 0; | ||
public extended: IExtendedAttrs = new ExtendedAttrs(); |
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.
So the approximate memory increase per cell will be 1 pointer for this, and then the standard ExtendedAttrs
object will get shared between all cells?
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 only for cells, that actually have extended attribs set. All other (thus most cells) dont use more memory in the buffer. To get this working I used another bit in bg
as indicator whether a cell holds extended attrs (HAS_EXTENDED
flag).
Yepp. But thats non trivial, as you already might have guessed from the comments in #1145. Gonna fix the issues above in the next couple of days, but prolly wont make it to the renderer parts 😞 |
@jerch no worries, we should still merge to avoid conflicts piling up |
Adds support for colon `:` separated sub parameters in parser. Technically, after this PR, nothing should change except, now sub parameters are parsed, stored safely and we don't invalidate the whole sequence when a `:` is received within a parameter substring. In this PR: - If sub parameters are detected with a parameter, but the usage is unrecognised, we simply *skip* the parameter in `adaptDispatch`. - A separate store for sub parameters is used to avoid too many changes to the codebase. - We currently allow up to `6` sub parameters for each parameter, extra sub parameters are *ignored*. - Introduced `VTSubParameters` for easy access to underlying sub parameters. > **Info**: We don't use sub parameters for any feature yet, this is just the core implementation to support newer usecases. ## Validation Steps Performed - [x] Use of sub parameters must not have any effect on the output. - [x] Skip parameters with unexpected set of sub parameters. - [x] Skip sequences with unexpected set of sub parameters. References #4321 References #7228 References #15599 References xtermjs/xterm.js#2751 Closes #4321
PR to support additional underline styles and colors in
AttributeData
. Implements core parts of #1145.Note that this PR extends
AttributeData
in a generic way, which can be use to store other data later on (like #1134).Changes to the internal interface:
The color and the color mode accessors transparently map to the FG variants, if no independent color was set or underline is unset. Thus its important to test beforehand for
isUnderline
.TODO:
AttributeData
to expose extended attributesBufferLine
to hold and export extended attributes