Skip to content

fix double underline, upwards#4648

Merged
Tyriar merged 2 commits intoxtermjs:masterfrom
tisilent:fix-double-underline
Aug 9, 2023
Merged

fix double underline, upwards#4648
Tyriar merged 2 commits intoxtermjs:masterfrom
tisilent:fix-double-underline

Conversation

@tisilent
Copy link
Copy Markdown
Contributor

@tisilent tisilent commented Aug 8, 2023

fix #4476

Comment on lines 1022 to 1030
if (imageData.data[offset] === r &&
imageData.data[offset + 1] === g &&
imageData.data[offset + 2] === b) {
imageData.data[offset + 1] === g &&
imageData.data[offset + 2] === b) {
imageData.data[offset + 3] = 0;
} else {
// Check the threshold based difference
if (enableThresholdCheck &&
(Math.abs(imageData.data[offset] - r) +
(Math.abs(imageData.data[offset] - r) +
Math.abs(imageData.data[offset + 1] - g) +
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you revert these? The intent here is to double-indent wrapped lines to easily differentiate them from the lines inside the block. You might need to disable prettier if you're using that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ou.....I restore it. 😶‍🌫️

Comment on lines 546 to 549
this._tmpCtx.moveTo(xChLeft, yTop - ySpace);
this._tmpCtx.lineTo(xChRight, yTop - ySpace);
this._tmpCtx.moveTo(xChLeft, yTop);
this._tmpCtx.lineTo(xChRight, yTop);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're fixing a limitation of the canvas renderer, we still want the old ideal behavior on the webgl renderer. So this should be based on a flag passed into _drawToCache, something like restrictToCellHeight: boolean

Copy link
Copy Markdown
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great 👍

I created #4653 as we want a similar fix to curly too, ideally we would have a generic solution for this problem that ensures all underlines are within the cell for that.

@Tyriar Tyriar added this to the 5.3.0 milestone Aug 9, 2023
@Tyriar Tyriar self-assigned this Aug 9, 2023
@Tyriar Tyriar merged commit 0224317 into xtermjs:master Aug 9, 2023
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.

canvas renderer double underline is a single underline

2 participants