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

Use variants for underline dotted. #4703

Merged
merged 27 commits into from
Nov 2, 2023
Merged

Conversation

tisilent
Copy link
Contributor

fix #4349

@tisilent
Copy link
Contributor Author

hi @Tyriar , The effect should be better this time.

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.

Thanks! I'll try get to a proper review in a week or 2

src/common/MultiKeyMap.ts Outdated Show resolved Hide resolved
@tisilent
Copy link
Contributor Author

I need to clean up the code.
I want to fix the dashed line as well.

@tisilent
Copy link
Contributor Author

Thanks.

Comment on lines 372 to 373
this._cellColorResolver.result.ext &= ~ExtFlags.VARIANT_OFFSET;
this._cellColorResolver.result.ext |= (variantOffset << 29) & ExtFlags.VARIANT_OFFSET;
Copy link
Member

Choose a reason for hiding this comment

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

This should live inside CellColorResolver.result. It would be ideal if calculating variantOffset was inside that function as well.

Comment on lines 80 to 84
const fontSize = this._optionsService.rawOptions.fontSize;
const drp = this._coreBrowserService.dpr;
const lineWidth = Math.max(1, Math.floor(fontSize * drp / 15));
const deviceCellWidth = this._charAtlas?.getDeviceCellWidth();
let variantOffset: number = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Can all this logic live inside CellColorResolver?

Comment on lines 412 to 416

const fontSize = this._optionsService.rawOptions.fontSize;
const drp = this._coreBrowserService.dpr;
const lineWidth = Math.max(1, Math.floor(fontSize * drp / 15));
const deviceCellWidth = this._charAtlas?.getDeviceCellWidth();
Copy link
Member

Choose a reason for hiding this comment

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

Same; can this be shared inside CellColorResolver?

Comment on lines 415 to 417
const lineWidth = Math.max(1, Math.floor(fontSize * drp / 15));
const deviceCellWidth = this._charAtlas?.getDeviceCellWidth();

Copy link
Member

Choose a reason for hiding this comment

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

You should be able to pull device cell width off `this.dimensions``

@tisilent
Copy link
Contributor Author

ok.I have discovered an issue now. I will solve it first

Comment on lines 161 to 167
let chWidth: number;
if (typeof code === 'number') {
chWidth = this._unicodeService.wcwidth(code);
} else {
chWidth = this._unicodeService.getStringCellWidth(code);
}
$variantOffset = computeVarinatOffset(deviceCellWidth * chWidth, lineWidth, $variantOffset);
Copy link
Member

Choose a reason for hiding this comment

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

I was expecting the variant to be based on the left-most pixel of the glyph, so chWidth wouldn't be needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to take a look at the process here.....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. 😄

Comment on lines 164 to 166
} else {
chWidth = this._unicodeService.getStringCellWidth(code);
}
Copy link
Member

Choose a reason for hiding this comment

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

code should always be a number so this isn't necessary. Did you mean to pass in chars?

if (code !== NULL_CELL_CODE) {
const fontSize = this._terminal.options.fontSize;
const drp = this._coreBrowserService.dpr;
const lineWidth = Math.max(1, Math.floor(fontSize! * drp / 15));
Copy link
Member

Choose a reason for hiding this comment

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

If you pass in IOptionsService you can use rawOptions.fontSize and avoid the unsafe !

src/browser/renderer/shared/CellColorResolver.ts Outdated Show resolved Hide resolved
src/browser/renderer/shared/CellColorResolver.ts Outdated Show resolved Hide resolved
Comment on lines 602 to 606
if (offsetWidth === 0) {
this._tmpCtx.setLineDash([Math.round(lineWidth), Math.round(lineWidth)]);
this._tmpCtx.moveTo(xChLeft, yTop);
this._tmpCtx.lineTo(xChRight, yTop);
} else {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need a special case for this as the main effect is + offsetWidth = + 0?

src/common/buffer/AttributeData.ts Outdated Show resolved Hide resolved
tisilent and others added 3 commits August 25, 2023 10:33
Co-authored-by: Daniel Imms <2193314+Tyriar@users.noreply.github.com>
Co-authored-by: Daniel Imms <2193314+Tyriar@users.noreply.github.com>
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.

This works really well! 👏

image

image

image

@Tyriar Tyriar added this to the 5.4.0 milestone Nov 2, 2023
@Tyriar Tyriar self-assigned this Nov 2, 2023
@Tyriar Tyriar enabled auto-merge November 2, 2023 17:48
@Tyriar Tyriar merged commit 86074c9 into xtermjs:master Nov 2, 2023
19 checks passed
@tisilent tisilent deleted the underline-dotted branch November 3, 2023 04:23
@tisilent
Copy link
Contributor Author

tisilent commented Nov 3, 2023

Next. Curve.

@Tyriar
Copy link
Member

Tyriar commented Nov 3, 2023

@tisilent that would be great, I found it a bit tricky to get the curve to look good at various zoom levels.

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.

Take column into account when rendering cells for perfect patterned underline rendering
2 participants