Skip to content
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

fix double underline, upwards #4648

Merged
merged 2 commits into from
Aug 9, 2023
Merged

Conversation

tisilent
Copy link
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
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
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
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
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
8 checks passed
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