Allowing to define variable line heights #233110
Conversation
Decorations may affect line wrapping. For example, they make make the text bold, or increase or decrease font size. This wasn’t originally taken into account to calculate the the break offsets.
|
Ey |
aiday-mar
changed the title
| /** | ||
| * If set, the decoration will override the line height of the lines it spans. This can only increase the line height, not decrease it. | ||
| */ | ||
| lineHeight?: number | null; |
There was a problem hiding this comment.
qq: why can the line height only be increased?
| @@ -97,6 +97,7 @@ export class LinesLayout { | |||
| private _lineHeight: number; | |||
| private _paddingTop: number; | |||
| private _paddingBottom: number; | |||
| private _specialLineHeights = new Map<number, number>(); | |||
There was a problem hiding this comment.
Let's implement this using a more performant data structure which makes things slow only when the feature is exercised. For example, for a file with 100K lines, getting the top position for line 99K will involve querying this _specialLineHeights map 99K times.
It also seems to me that we're not quite keeping this field up to date correctly. If I set a line height of 40 on a line 1000 and then press enter on the first line, will the _specialLineHeights be updated correctly to reflect the edit I did?
| @@ -62,6 +62,7 @@ export class RenderLineInput { | |||
| public readonly renderWhitespace: RenderWhitespace; | |||
| public readonly renderControlCharacters: boolean; | |||
| public readonly fontLigatures: boolean; | |||
| public readonly lineHeight: number; | |||
There was a problem hiding this comment.
Why is this field added if there are no usages?
| @@ -47,6 +47,7 @@ export class ViewportData { | |||
| private readonly _model: IViewModel; | |||
|
|
|||
| public readonly lineHeight: number; | |||
| public readonly specialLineHeights: Map<number, number>; | |||
There was a problem hiding this comment.
Please don't add this field. Do not spread out the logic of the special line heights. Let's use a single way to get to it via this._context.viewLayout.getLineHeightForLineNumber
| private _lineHeightForLineNumber(lineNumber: number): number { | ||
| if (this._viewportData.specialLineHeights.has(lineNumber)) { | ||
| return this._viewportData.specialLineHeights.get(lineNumber)!; | ||
| } | ||
| return this._viewportData.lineHeight; | ||
| } |
There was a problem hiding this comment.
Try to feed in a getter for this data via a new ctor field if necessary. Please don't add the line heights to the viewport data.
| /** | ||
| * The special line heights | ||
| */ | ||
| readonly specialLineHeights: Map<number, number>; |
|
|
||
| constructor(lineNumber: number, detail: string, injectedText: LineInjectedText[] | null) { | ||
| constructor(lineNumber: number, detail: string, injectedText: LineInjectedText[] | null, lineHeight: number | null) { |
There was a problem hiding this comment.
Let's please use a new event and not overload the meaning of this one.
| */ | ||
| export class ModelSpecialLineHeightChangedEvent { | ||
|
|
||
| public readonly changes: ModelRawLineChanged[]; |
There was a problem hiding this comment.
Let's transport the changes with a new data type, there is no need to reuse/enlarge ModelRawLineChanged and it will just confuse all users of the event.
| private _getLineHeightForLine(lineNumber: number): number | null { | ||
| const startOffset = this._buffer.getOffsetAt(lineNumber, 1); | ||
| const endOffset = startOffset + this._buffer.getLineLength(lineNumber); | ||
| return this._decorationsTree.getLineHeightInInterval(this, startOffset, endOffset, 0); | ||
| } |
There was a problem hiding this comment.
There is something not quite correct here. Line heights don't work quite like injected text, because we should allow for line heights of the same line to be different across different editors. For example, if a particular text model is shown in editor1 and editor2, we should allow that a particular line has a particular height in editor1 and a different height in editor2, since line height is a presentation option (which is arguably different than injected text). So I think the decoration ownerId should play a part in this method call, not quite sure who calls it.
fixes #147067