Use variants for underline dotted.#4703
Conversation
|
hi @Tyriar , The effect should be better this time. |
Tyriar
left a comment
Tyriar
left a comment
There was a problem hiding this comment.
Thanks! I'll try get to a proper review in a week or 2
|
I need to clean up the code. |
|
Thanks. |
| this._cellColorResolver.result.ext &= ~ExtFlags.VARIANT_OFFSET; | ||
| this._cellColorResolver.result.ext |= (variantOffset << 29) & ExtFlags.VARIANT_OFFSET; |
There was a problem hiding this comment.
This should live inside CellColorResolver.result. It would be ideal if calculating variantOffset was inside that function as well.
| 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; |
There was a problem hiding this comment.
Can all this logic live inside CellColorResolver?
|
|
||
| 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(); |
There was a problem hiding this comment.
Same; can this be shared inside CellColorResolver?
| const lineWidth = Math.max(1, Math.floor(fontSize * drp / 15)); | ||
| const deviceCellWidth = this._charAtlas?.getDeviceCellWidth(); | ||
|
|
There was a problem hiding this comment.
You should be able to pull device cell width off `this.dimensions``
|
ok.I have discovered an issue now. I will solve it first |
| let chWidth: number; | ||
| if (typeof code === 'number') { | ||
| chWidth = this._unicodeService.wcwidth(code); | ||
| } else { | ||
| chWidth = this._unicodeService.getStringCellWidth(code); | ||
| } | ||
| $variantOffset = computeVarinatOffset(deviceCellWidth * chWidth, lineWidth, $variantOffset); |
There was a problem hiding this comment.
I was expecting the variant to be based on the left-most pixel of the glyph, so chWidth wouldn't be needed?
There was a problem hiding this comment.
I need to take a look at the process here.....
| } else { | ||
| chWidth = this._unicodeService.getStringCellWidth(code); | ||
| } |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
If you pass in IOptionsService you can use rawOptions.fontSize and avoid the unsafe !
| if (offsetWidth === 0) { | ||
| this._tmpCtx.setLineDash([Math.round(lineWidth), Math.round(lineWidth)]); | ||
| this._tmpCtx.moveTo(xChLeft, yTop); | ||
| this._tmpCtx.lineTo(xChRight, yTop); | ||
| } else { |
There was a problem hiding this comment.
I don't think you need a special case for this as the main effect is + offsetWidth = + 0?
Co-authored-by: Daniel Imms <[email protected]>
Co-authored-by: Daniel Imms <[email protected]>
|
Next. Curve. |
|
@tisilent that would be great, I found it a bit tricky to get the curve to look good at various zoom levels. |
2 participants
fix #4349