-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add support for underline style and color in VT #15795
Merged
Merged
Changes from 7 commits
Commits
Show all changes
42 commits
Select commit
Hold shift + click to select a range
07cb660
add support for underline color & style in VT parser
tusharsnx c26b688
add underline color and style support to VT renderer
tusharsnx 0093d93
use const& when reading TextAttribute without mutation
tusharsnx ac2b786
more attributes read to const& refactoring
tusharsnx 87a43c0
respect underline style and color in DECRARA and DECCARA
tusharsnx 3dcb214
remaining refactor for const& for reading TextAttribute
tusharsnx 1027e1b
refactor _SetRgbColorsHelperFromSubParams
tusharsnx 1673c6e
let DECCARA apply underline color
tusharsnx a8bede3
store underline style within CharacterAttribute
tusharsnx 00833f9
use public apis to manipulate flags in CharacterAttributes
tusharsnx 87b3fa7
Merge remote-tracking branch 'upstream/main' into temp
tusharsnx 0b38f07
update DECCIR to use styles instead of boolean when setting the under…
tusharsnx 7d12a21
let's bid adieu to CharacterAttribute::DoublyUnderlined
tusharsnx 1163542
update routines for XTPUSHSGR / XTPOPSGR
tusharsnx a7d2437
update routines for DECRQSS
tusharsnx d18f4d7
check underline color is not Index16 based
tusharsnx 42f6066
update routines for DECCIR
tusharsnx 9660e2a
update the tests
tusharsnx fd165f2
format
tusharsnx 0a3e9ae
fix little things
tusharsnx 5350ac4
fix a wrong default'ing for drawing underlines
tusharsnx 6d4ac54
fix wordings
tusharsnx eeff6a5
write seq for doubly underlined directly
tusharsnx 21309d5
report singly and doubly underlined with sgr 4 and 21 in DECRQSS resp…
tusharsnx a117f30
update test and add ITU color test for the underline
tusharsnx ac62677
renderer: merge NoUnderline into the default case
tusharsnx 5bb3531
adaptDispatch: maintain rendition attrs' numeric order while sending …
tusharsnx da50289
revert the comment added to SetDefaultRenditionAttributes
tusharsnx e33130c
initialize underline color to INVALID_COLOR where it make sense to do so
tusharsnx 32bd78a
avoid false errors due to missing headers within editor
tusharsnx 820cbc0
update test to use CurlyUnderlined instead of SinglyUnderlined
tusharsnx f5ab09a
reset doubly underlined before sending the new style
tusharsnx be681de
pass a separate boolean reverseUnderline in ChangeOps to reverse unde…
tusharsnx 7fd1129
update restore logic for XTPUSHSGR/XTPOPSGR
tusharsnx 432d4df
Merge remote-tracking branch 'upstream/main' into feat-underline-styl…
tusharsnx 20755f3
inline mapping functions for mapping underline style to/from characte…
tusharsnx 2935818
Merge remote-tracking branch 'upstream/main' into feat-underline-styl…
tusharsnx 8812867
use util to generate subparams and ranges
tusharsnx 31455cd
test underline color and style in GraphicsSingleWithSubParamTests
tusharsnx ea7dcfe
revert changes to DECRARA
tusharsnx a67a1ab
update the DECRARA testcase to test for intense attr instead of under…
tusharsnx dc1b95f
treat invalid style values as singly underlined
tusharsnx File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Instead of storing the underline style in a separate attribute, which ends up requiring an additional two bytes, I would much prefer if we could just add it to the existing
CharacterAttributes
enum.I think all we really need is one more bit. Maybe move the
Faint
to 0x20, and then reserve 0x40, 0x80, and 0x100 for the underline styles. Something like this:Then you can access those attributes with something like this (pseudo code):
There are probably some
WI_
macros you can use that'll make the implementation cleaner - I just wanted to demonstrate the general idea.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.
Excellent! I think I can implement this, with little tweaks maybe.
Wondering if it's safe to assume only one of the underline style can be active at any point, including doublyUnderlined even if it's done using
SGR 21
. I guess we don't need to track doubly underline separately, do we?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.
I'm glad you asked this, because we do track them separately at the moment, but I don't think we need to retain that behavior.
At the time I added double underline support there was no consensus in how this was handled amongst other terminals, so I just followed what XTerm was doing. But in retrospect I think that was probably the wrong decision, and now that we're adding more underline styles it makes even less sense. So yeah, I'd say we don't need to track double underline separately.
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.
I took a look at the discussion over at #2916 (comment) where you mentioned:
I assume this needs to be taken care of?
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.
This shouldn't apply to SGR 4:x.
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.
No. That's what I was trying to explain above. It seemed like the right thing to do at the time, but in retrospect, copying XTerm was the wrong decision. I don't have a record of my testing from back then, but having just checked now, I can't find anyone that still matches XTerm other than us. And in our case, we only really match XTerm in the DX renderer - the Atlas renderer draws the single underline on top of the double underline when both are applied.
There is no reason for us to retain the current behavior. This is such a ridiculous edge case anyway that nobody is going to be expecting it to work in a particular way.