Make TextLayout use consistent font size. Do not merge. For testing only.

[DenisUngemach]

Do not merge. For testing only.

[HeikoKlare]

Okay, that's interesting. Can you execute the test class Test_org_eclipse_swt_custom_StyledText on #99 without test failures due to stack overflows?

[DenisUngemach]

Test_org_eclipse_swt_custom_StyledText

Currently all tests work, except two fail, but not with the stack overflow. Some asserts fail:

java.lang.AssertionError: :k: expected:<0> but was:<1>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:647)
at org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_StyledText.test_getOffsetAtLocationLorg_eclipse_swt_graphics_Point(Test_org_eclipse_swt_custom_StyledText.java:1368)
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:93)
at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:40)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:529)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:757)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:452)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:210)
Suppressed: java.lang.Throwable: Screenshot written to C:\Users\D071174\AppData\Local\Temp\org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_StyledText.test_getOffsetAtLocationLorg_eclipse_swt_graphics_Point.png
at org.eclipse.test.Screenshots$ScreenshotOnFailure.failed(Screenshots.java:41)
at org.junit.rules.TestWatcher.failedQuietly(TestWatcher.java:90)
at org.junit.rules.TestWatcher.access$300(TestWatcher.java:52)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:68)
... 18 more

[DenisUngemach]

I will check the existing tests for TextLayout. Some are failing.

[HeikoKlare]

Strange. I am wondering what makes the tests behave differently on our systems.

For example the test case test_lineStyleListener_invalidStyles_render gives me:

java.lang.StackOverflowError
	at org.eclipse.swt.custom.StyledTextRenderer.getTextLayout(StyledTextRenderer.java:1044)
	at org.eclipse.swt.custom.StyledTextRenderer.getTextLayout(StyledTextRenderer.java:984)
	at org.eclipse.swt.custom.StyledText.getPointAtOffset(StyledText.java:5190)
	at java.base/java.util.stream.IntPipeline$1$1.accept(IntPipeline.java:180)
	at java.base/java.util.Spliterators$IntArraySpliterator.forEachRemaining(Spliterators.java:1076)
	at java.base/java.util.Spliterator$OfInt.forEachRemaining(Spliterator.java:711)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:575)
	at java.base/java.util.stream.AbstractPipeline.evaluateToArrayNode(AbstractPipeline.java:260)
	at java.base/java.util.stream.ReferencePipeline.toArray(ReferencePipeline.java:616)
	at org.eclipse.swt.custom.StyledText.setCaretLocations(StyledText.java:8440)
	at org.eclipse.swt.custom.StyledTextRenderer.getTextLayout(StyledTextRenderer.java:1383)
	at org.eclipse.swt.custom.StyledTextRenderer.getTextLayout(StyledTextRenderer.java:984)
	at org.eclipse.swt.custom.StyledText.getPointAtOffset(StyledText.java:5190)
	at java.base/java.util.stream.IntPipeline$1$1.accept(IntPipeline.java:180)
	at java.base/java.util.Spliterators$IntArraySpliterator.forEachRemaining(Spliterators.java:1076)
	at java.base/java.util.Spliterator$OfInt.forEachRemaining(Spliterator.java:711)
        ...

[DenisUngemach]

Strange. I am wondering what makes the tests behave differently on our systems.

For example the test case test_lineStyleListener_invalidStyles_render gives me:

java.lang.StackOverflowError
	at org.eclipse.swt.custom.StyledTextRenderer.getTextLayout(StyledTextRenderer.java:1044)
	at org.eclipse.swt.custom.StyledTextRenderer.getTextLayout(StyledTextRenderer.java:984)
	at org.eclipse.swt.custom.StyledText.getPointAtOffset(StyledText.java:5190)
	at java.base/java.util.stream.IntPipeline$1$1.accept(IntPipeline.java:180)
	at java.base/java.util.Spliterators$IntArraySpliterator.forEachRemaining(Spliterators.java:1076)
	at java.base/java.util.Spliterator$OfInt.forEachRemaining(Spliterator.java:711)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:575)
	at java.base/java.util.stream.AbstractPipeline.evaluateToArrayNode(AbstractPipeline.java:260)
	at java.base/java.util.stream.ReferencePipeline.toArray(ReferencePipeline.java:616)
	at org.eclipse.swt.custom.StyledText.setCaretLocations(StyledText.java:8440)
	at org.eclipse.swt.custom.StyledTextRenderer.getTextLayout(StyledTextRenderer.java:1383)
	at org.eclipse.swt.custom.StyledTextRenderer.getTextLayout(StyledTextRenderer.java:984)
	at org.eclipse.swt.custom.StyledText.getPointAtOffset(StyledText.java:5190)
	at java.base/java.util.stream.IntPipeline$1$1.accept(IntPipeline.java:180)
	at java.base/java.util.Spliterators$IntArraySpliterator.forEachRemaining(Spliterators.java:1076)
	at java.base/java.util.Spliterator$OfInt.forEachRemaining(Spliterator.java:711)
        ...

Can you debug the getTextLayout in StyledTextRenderer?
I would guess, that only these 4 stackframes are important:

at org.eclipse.swt.custom.StyledText.setCaretLocations(StyledText.java:8440)
at org.eclipse.swt.custom.StyledTextRenderer.getTextLayout(StyledTextRenderer.java:1383)
at org.eclipse.swt.custom.StyledTextRenderer.getTextLayout(StyledTextRenderer.java:984)
at org.eclipse.swt.custom.StyledText.getPointAtOffset(StyledText.java:5190)

The reason for this problem actually seems to be the new getFontSize() implementation.

Can you revert only this method back to my heuristic code. And then run the test?

[HeikoKlare]

Can you revert only this method back to my heuristic code. And then run the test?

Yes, sorry that I did not mention that explicitly: without the change to getFontSize(), the test runs fine. But then the font size used in StyledText is wrong :-) (that's what the PR is actually for).

[DenisUngemach]

Yes, there were some other minor changes in TextLayout. This was also the first thing i found out.

[DenisUngemach]

The bug comes up because of StyledTextRenderer in line 1346 - 1360.

The size reaction for setAscent and setDescent is not implemented in TextLayout. This means it expects that the TextLayout got bigger, but this won't happen.

and since the size update does not work it calls every time: styledText.setCaretLocations(); which calls the TextLayout size change once again.

[DenisUngemach]

The combination of FontMetrics, ascent and descent does not work.

[HeikoKlare]

The combination of FontMetrics, ascent and descent does not work.

Sounds reasonable. In particular, the TextLayout uses an internal GC for retrieving the font metrics which is based on a native GC rather than a SkijaGC. Does that make sense or can that produce inconsistencies?

The size reaction for setAscent and setDescent is not implemented in TextLayout. This means it expects that the TextLayout got bigger, but this won't happen.

Can you further explain this? What kind of reactions would the TextLayout need to do? While setting ascent/descent should of course have an effect on the layout, they should not have an effect on the font size.

One thing I also noticed is that the marker for the corresponding brace is not sized correctly. This may also be an indicator for an inconsistency, as everything else (used font size, line number ruler, caret) seem to be consistent.

[DenisUngemach]

StyledTextRenderer line 1345 - 1386:

Here it first checks the heights and it wants to find out whether there are lines with bigger height than its own height.

If it finds such a line, then it wants to modify all layouts with this fonts metrics ascent and descent parameter.
By that it expects, that now the first situation (line 5 - 11 in this code ) , which means that a line has higher height than the others does no longer occur.

With: styledText.setCaretLocations(); later the infinite loop is caused, if this won't be handled properly.

if (styledText != null && styledText.isFixedLineHeight()) {
	int index = -1;
	int lineCount = layout.getLineCount();
	int height = getLineHeight();
	for (int i = 0; i < lineCount; i++) {
		int lineHeight = layout.getLineBounds(i).height;
		if (lineHeight > height) {
			height = lineHeight;
			index = i;
		}
	}
	if (index != -1) {
		FontMetrics metrics = layout.getLineMetrics(index);
		ascent = metrics.getAscent() + metrics.getLeading();
		descent = metrics.getDescent();
		if (layouts != null) {
			for (TextLayout l : layouts) {
				if (l != null && l != layout) {
					l.setAscent(ascent);
					l.setDescent(descent);
				}
			}
		}
		styledText.calculateScrollBars();
		if (styledText.verticalScrollOffset != 0) {
			int topIndex = styledText.topIndex;
			int topIndexY = styledText.topIndexY;
			int lineHeight = getLineHeight();
			int newVerticalScrollOffset;
			if (topIndexY >= 0) {
				newVerticalScrollOffset = (topIndex - 1) * lineHeight + lineHeight - topIndexY;
			} else {
				newVerticalScrollOffset = topIndex * lineHeight - topIndexY;
			}
			styledText.scrollVertical(newVerticalScrollOffset - styledText.verticalScrollOffset, true);
		}
		if (styledText.isBidiCaret()) styledText.createCaretBitmaps();
		styledText.caretDirection = SWT.NULL;
		styledText.setCaretLocations();
		styledText.redraw();
	}
}

[HeikoKlare]

Thank you, @DenisUngemach! I finally got it: when computing the lines in computeRuns, the height value of the line metrics was taken instead of summing up the ascent, decent and linespacing (as used by the original TextLayout as well). Both all tests as well as manual testing inside an Eclipse product work quite fine now with #99.

[DenisUngemach]

The branch contains some Test modifications for TextLayout. This might be good to have....

[HeikoKlare]

The branch contains some Test modifications for TextLayout. This might be good to have....

Indeed. Do you want to rebase on current master and extract those changes or should I do that while you are away?

[DenisUngemach]

The branch contains some Test modifications for TextLayout. This might be good to have....

Indeed. Do you want to rebase on current master and extract those changes or should I do that while you are away?

I will be in vacation in 5 minutes.

I have to say, that the TextLayout code is messy, because i searched for the font size bug and also made some modifications for the tests.
Feel free to check this out.

Since the skija paragraph TextBox defines sizes in float and not in int, there are some problems with the positions, because of rounding.

[HeikoKlare]

I'll try to do it. Enjoy your vacation! 🙂