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

Support dynamic line heights using decorations #194609

Closed

Conversation

remcohaszing
Copy link
Contributor

@remcohaszing remcohaszing commented Oct 2, 2023

This adds support for dynamic line heights using decorations in Monaco editor. This is done by adding the lineHeight property to IModelDecorationOptions. This property represents the line height of the decoration all lines it touches. The line height can only be increased, not decreased.

For testing, I used the following JavaScript and CSS in the Monaco playground:

var jsCode = [
	'"use strict";',
	"function Person(age) {",
	"	if (age) {",
	"		this.age = age;",
	"	}",
	"}",
	"Person.prototype.getAge = function () {",
	"	return this.age;",
	"};",
].join("\n");

var editor = monaco.editor.create(document.getElementById("container"), {
	value: jsCode,
	language: "javascript",
});

var decorations = editor.createDecorationsCollection([
	{
		range: new monaco.Range(3, 1, 5, 1),
		options: {
			lineHeight: 30,
			isWholeLine: true,
			linesDecorationsClassName: "myLineDecoration",
		},
	},
	{
		range: new monaco.Range(7, 1, 7, 24),
		options: {
			lineHeight: 30,
			inlineClassName: "myInlineDecoration",
		},
	},
]);
.myInlineDecoration {
	color: red !important;
	cursor: pointer;
	text-decoration: underline;
	font-weight: bold;
	font-style: oblique;
	font-size: 30px;
}

.myLineDecoration {
	background: lightblue;
	width: 5px !important;
	margin-left: 3px;
}

Since text might now not fill the entire line height, I chose to align the text at the bottom of the line. This matches default behaviour of typical document processors such as LibreOffice Writer and Google Docs.

image

The following screenshot shows that the following works:

  • Line decorations are full height
  • Text is aligned at the bottom
  • The cursor is aligned correctly
  • The current line indicator is adjusted to the line height
  • Matching words highlighting is aligned correctly

image

The following screenshot shows matching brackets are aligned correctly.

image

The following shows that text selection works. It also shows that the height of the selection indicator is adjusted to the editor’s line height, not that of the decorator. This is different from for example how LibreOffice does it. The same applies to other types of highlights, such as matching words and search.

image

Text selection in LibreOffice

image

Things that still need to be done:

  • Things are still weird if I insert or remove new lines.
  • Discuss if highlights should use the editor line height or the line’s line height. I went with using the full line height, as my client requested this, and this is consistent with LibreOffice and Google Docs.
  • Fix handling of inserting new lines.
  • Fix handling of removing new lines.
  • The search box breaks layout.
  • Fix layout shifts height when scrolling.
  • Handle word wrap
  • Handle cursor size

cc @hediet

Closes #147067

@remcohaszing remcohaszing marked this pull request as draft October 2, 2023 14:14
@hediet
Copy link
Member

hediet commented Oct 2, 2023

Nice work so far! I didn't take a very close look, but keep in mind that if no line heights are set, there shouldn't be any performance hit by this feature.

@remcohaszing
Copy link
Contributor Author

I created another fun little test case:

const model = monaco.editor.createModel(`# Coming soon

## to VSCode

### (and Monaco editor)

#### Variable line heights

##### Allowing for fancy stuff

###### Such as bigger markdown headers

And this is regular text`, 'markdown');

// Hover on each property to see its docs!
const ed = monaco.editor.create(document.getElementById("container"), {
	model,
	automaticLayout: true,
});

var decorations = ed.createDecorationsCollection([
	{
		range: new monaco.Range(1, 1, 1, Infinity),
		options: { lineHeight: 50, inlineClassName: "m50" },
	},
	{
		range: new monaco.Range(3, 1, 3, Infinity),
		options: { lineHeight: 45, inlineClassName: "m45" },
	},
	{
		range: new monaco.Range(5, 1, 5, Infinity),
		options: { lineHeight: 40, inlineClassName: "m40" },
	},
	{
		range: new monaco.Range(7, 1, 7, Infinity),
		options: { lineHeight: 35, inlineClassName: "m35" },
	},
	{
		range: new monaco.Range(9, 1, 9, Infinity),
		options: { lineHeight: 30, inlineClassName: "m30" },
	},
	{
		range: new monaco.Range(11, 1, 11, Infinity),
		options: { lineHeight: 25, inlineClassName: "m25" },
	},
]);
.m50 { font-size: 50px; }
.m45 { font-size: 45px; }
.m40 { font-size: 40px; }
.m35 { font-size: 35px; }
.m30 { font-size: 30px; }
.m25 { font-size: 25px; }

@remcohaszing remcohaszing marked this pull request as ready for review October 4, 2023 19:19
@remcohaszing
Copy link
Contributor Author

As far as I know everything works now.

Tests are still missing (old ones commented out). I would like to fix that once it’s confirmed the rest looks good.

This applies to selected text, search highlight, and same word
highlight, and other forms of highlighting.
@kieferrm kieferrm mentioned this pull request Nov 6, 2023
55 tasks
@hediet
Copy link
Member

hediet commented Nov 10, 2023

This looks very nice

chrome_WUw9gbCdfO

Another demo:

chrome_LuQK9RaCdk

I think the minimap should also reflect the line height.

@@ -699,6 +705,7 @@ export class LinesLayout {
break;
}
}
linesOffsets[lineNumber - startLineNumber] = currentLineRelativeOffset;
Copy link
Member

Choose a reason for hiding this comment

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

I also don't understand this line here. Was this a bug before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes I need the top offset of the next line requested. So I need the line offsets of all lines in the view, plus one the next line.

@@ -46,6 +48,7 @@ export class ViewModelDecorations implements IDisposable {
this._decorationsCache = Object.create(null);
this._cachedModelDecorationsResolver = null;
this._cachedModelDecorationsResolverViewRange = null;
this._lineHeight = configuration.options.get(EditorOption.lineHeight);
Copy link
Member

Choose a reason for hiding this comment

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

We have to react on when the line height is changed through options.

@hediet
Copy link
Member

hediet commented Nov 10, 2023

Thank you for the PR, nice job!

Generally, this looks quite good! Just the actual computation in lineLayout and viewModelDecorations needs more thought - I'll do some changes there.

Can you look into cursor height and try different line heights in various features of vscode? Like the diff editor? How broken is the VS Code experience when some lines have a different height than others?

@remcohaszing
Copy link
Contributor Author

Re #194609 (comment):

I created the remcohaszing.markdown-decorations extension with the intention to leverage the changes in this PR for markdown / MDX headers.


Re minimap:

I’m personally not a fan of the minimap. I’m not sure how it should behave, so I’ll take your word for it. I would need to look into that.


Some feedback I for from my client is that I still need to look into line wrapping if a non-monospace font is used. I haven’t looked into that yet. This could mean I need to make changes, or that they need to leverage an existing Monaco setting


Some other feedback is that they would like to make the line height smaller. So essentially they want support for #66468. The actual implementation for that issue could be an extension. My biggest problem with that is that I don’t know how to deal with decorations and line numbers.

@remcohaszing
Copy link
Contributor Author

Re diff editor:

It’s not terrible, not not great eiter. Both editors look good invididually, but they aren’t properly aligned when decorations are applied.

I also haven’t implemented the interface for extensions yet, but that shouldn’t be too hard. I think they need 2 additional properties: lineHeight and fontSize.

If it’s ok, I would also like to add the fontFamily property while at it. I would like to leverage this in remcohaszing.markdown-decorations.

@Apetree100122
Copy link

var jsCode =
['"use strict";',
"function Person
(age
)
{","if (age){",this.age = age
;",}","}","Person.prototype.getAge =
function () {",
"return this.age;"
"};",].join("\n
");var editor = monaco.editor.create(document.getElementById("container"
),
{value: jsCode,
language: "javascript"});var decorations = editor.createDecorationsCollection
(
[{range: new monaco.Range(3, 1
5, 1),
opti
ons: {lineHeight:
30,isWhole
Line: true,linesDecorationsClass
Name:myLineDecoration"},
},{range: new monaco.Range(7, 1, 7
24),opt
ions: {lineHeight:
30,
inline
_Class
NamemyInlineDecoration},
}]);.m50 {
font-size: 50px
}.m45 { font-size:
45px; }m
40 { font-size: 40px
}m
35 {
font-size:
35px
}.m
30
{ font-size:
30px}.m
25 { font-size: 2
5px}
const model = monaco.editor.createModel
`# Coming soon ## to VSCode ### and Monaco editor
)#### Variable line heights ##### Allowing for fancy stuff

Such as bigger markdown headers And this is regular text`

'markdown');
\Hover on each property to see its docs
!const ed =monaco.editor.create
(
document.get
ElementBy
Id container, {model, automatic
Layout: "true})var decorations = ed.createDecorationsCollection
(
[{range: new monaco.Range(1
_______1, 1
[[Infinity]]]
________options
{ lineHeight: 50, inlineClassName: "m
50" }},
{range: new monaco.Range(3, 1, 3, Infin
options: { lineHeight: 45, inlineClassName: "m4
5" }},{range: new monaco.Range(5, 1, 5, Infinity),
options: { lineHeight: 40, inlineClassName:{
range: new monaco.Range(9, 1, 9, Infinity),{
range: new monaco.Range( 1 1 1 1 )options:
{ lineHeight: 25, inlineClassName: "m25
])

@remcohaszing
Copy link
Contributor Author

Just for the record, I’m still fixing some things on the cuescript branch every now and then. Some of these changes may be desirable as part of this PR, some may not.

It would help if someone could have a look at #194712 and #199021 first.

Copy link

@Apetree100122 Apetree100122 left a comment

Choose a reason for hiding this comment

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

signed Apetree100122

@kieferrm kieferrm mentioned this pull request Feb 4, 2024
61 tasks
return this._decorationsHeightMapCache;
}

public getDecorationsOffset(lineNumber: number = this._linesCollection.getViewLineCount()): number {
Copy link
Member

@hediet hediet Feb 9, 2024

Choose a reason for hiding this comment

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

lineNumber: number = this._linesCollection.getViewLineCount()

I think this should be = this._linesCollection.getViewLineCount() + 1.
I'm currently refactoring this a bit.

@@ -507,7 +509,7 @@ export class LinesLayout {
public getVerticalOffsetAfterLineNumber(lineNumber: number, includeViewZones = false): number {
this._checkPendingChanges();
lineNumber = lineNumber | 0;
const previousLinesHeight = this._lineHeight * lineNumber;
const previousLinesHeight = this._decorations.getDecorationsOffset(lineNumber);
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this should be:

const previousLinesHeight = this._decorations.getDecorationsOffset(lineNumber + 1);

const lineCount = this._linesCollection.getViewLineCount();
const lineHeights = new Uint8ClampedArray(lineCount + 1).fill(this._lineHeight);
const viewRange = new Range(1, this._linesCollection.getViewLineMinColumn(0), lineCount, this._linesCollection.getViewLineMaxColumn(lineCount));
const modelDecorations = this._linesCollection.getDecorationsInRange(viewRange, this.editorId, true, false, false);
Copy link
Member

Choose a reason for hiding this comment

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

This is not good, I just noticed that viewRange is the entire document range.
This means that setting a decoration flushes everything here and causes querying all decorations.

@TongtongZhua
Copy link

Is there any plan to merge this code? Really looking forward to such exciting feature.

@kieferrm kieferrm mentioned this pull request Mar 4, 2024
64 tasks
@hediet
Copy link
Member

hediet commented Mar 7, 2024

I noticed another problem - by encoding the line height as distance between two line offsets, whitespaces (e.g. caused by code-lens) cause buggy behavior:

aNUfkHPEPs

hediet added a commit that referenced this pull request Mar 8, 2024
This contains the parts of #194609 that we can already merge.
While it does not implement dynamic line heights, it is a first step in that direction.

Co-authored-by: Remco Haszing <[email protected]>
@hediet
Copy link
Member

hediet commented Mar 8, 2024

I extracted the parts that we can take in #207160.
I'll close the PR for now as we cannot take the other parts without bigger modifications.

@hediet hediet closed this Mar 8, 2024
hediet added a commit that referenced this pull request Mar 8, 2024
This contains the parts of #194609 that we can already merge.
While it does not implement dynamic line heights, it is a first step in that direction.

Co-authored-by: Remco Haszing <[email protected]>
@microsoft microsoft locked and limited conversation to collaborators Jun 11, 2024
@aiday-mar aiday-mar requested review from aiday-mar and removed request for aiday-mar November 5, 2024 14:28
@aiday-mar aiday-mar self-assigned this Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support variable line heights in the editor
5 participants