Issue 932: implement fixed line height for StyledText #1002

SyntevoAlex · 2024-01-24T23:01:11Z

Please see #932 and included testing snippet. In it, create two StyledText: basic and with Use fixed line height option.

SyntevoAlex · 2024-01-24T23:01:35Z

I intend to merge this in a few days, unless someone wants to review.

github-actions · 2024-01-24T23:09:20Z

Test Results

299 files ±0 299 suites ±0 6m 37s ⏱️ +31s
4 100 tests ±0 4 092 ✅ ±0 8 💤 ±0 0 ❌ ±0
12 212 runs ±0 12 137 ✅ ±0 75 💤 ±0 0 ❌ ±0

Results for commit c9449b7. ± Comparison against base commit 15d3065.

♻️ This comment has been updated with latest results.

iloveeclipse · 2024-01-25T09:47:27Z

bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/TextLayout.java

+ * layouts, including those caused by word wrapping. StyledText uses one
+ * TextLayout per line and is only affected by word wrap restriction.
+ *
+ * @since 4.31


This is wrong version with 99.9% probability (nust be 3.125 as we don't use SDK version but bundle version to track API changes), but unfortunately due a bug in PDE API tooling (eclipse-pde/eclipse.pde#1073) I'm unable to validate API on this PR, and I also assume the API checks are broken on SWT master build either.

Thanks for taking you time to have a look!
Yes, sorry, since tags are quite confusing to me. There are 3 different version numbers in SWT (Eclipse, those mentioned in automatic commit messages, and fragment). I'll fix that.

iloveeclipse · 2024-01-25T09:50:44Z

...les/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/StyledText.java

+/**
+ * See {@link TextLayout#setFixedLineMetrics}
+ */
+public void setFixedLineMetrics(FontMetrics metrics) {


I believe with 99.9% probability this new method needs @since 3.125 comment and it would be an API error if API tooling would work properly.

Confirmed, it is needed.

iloveeclipse · 2024-01-25T09:51:29Z

...eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/StyledTextRenderer.java

@@ -180,6 +183,15 @@ public LineInfo(LineInfo info) {
 			}
 		}
 	}
+
+	private static class LineDrawInfo {


I wonder if that should be a record (so implicitly final fields)

Thanks, I'll try that. I learned about record just a few days ago (my excuse is that I'm not a Java programmer)

...eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/StyledTextRenderer.java

iloveeclipse · 2024-01-25T09:53:00Z

bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/FontMetrics.java

@@ -31,6 +31,16 @@ public final class FontMetrics {
 FontMetrics() {
 }

+public FontMetrics clone () {


Same here, I believe @since 3.125 is missing

Confirmed, it is needed.

iloveeclipse · 2024-01-25T09:54:17Z

bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/graphics/TextLayout.java

+ * layouts, including those caused by word wrapping. StyledText uses one
+ * TextLayout per line and is only affected by word wrap restriction.
+ *
+ * @since 4.31


@since 4.31 is wrong and should be @since 3.125

iloveeclipse · 2024-01-25T09:54:58Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/TextLayout.java

+ * layouts, including those caused by word wrapping. StyledText uses one
+ * TextLayout per line and is only affected by word wrap restriction.
+ *
+ * @since 4.31


@since 4.31 is wrong and should be @since 3.125

iloveeclipse · 2024-01-25T09:57:33Z

@merks : on #995 you've mentioned, that you are able to get API tooling work on SWT. May I ask you to checkout this PR and check if there will be any API errors reported?

If yes, we have also another issue with build not properly validating that on Jenkins...

merks · 2024-01-25T12:46:12Z

When I check out this PR I see these two errors (on Windows):

iloveeclipse · 2024-01-25T12:56:19Z

When I check out this PR I see these two errors

Thanks Ed, I've created #1003.

iloveeclipse · 2024-01-31T08:26:42Z

@SyntevoAlex : please rebase your PR on master, there were quite some changes in the SWT project infrastructure, so switching between your PR and master is not easy.

Also with the fix for eclipse-pde/eclipse.pde#1073 I can finally see "proper" API errors, so those I've mentioned before are confirmed to be valid.

iloveeclipse · 2024-01-31T12:09:42Z

@SyntevoAlex : thanks for rebase. I still see API errors, did you forget to fix them?

SyntevoAlex · 2024-01-31T12:31:00Z

I will fix them later, just rebased quickly on your request

SyntevoAlex · 2024-01-31T22:46:09Z

I applied code review suggestions, thanks for having a look!

SyntevoAlex · 2024-01-31T22:46:27Z

If there are no further issues, I intend to merge in a few days.

lshanmug · 2024-02-02T04:54:32Z

bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/FontMetrics.java

+ *
+ * @since 3.125
+ */
+public FontMetrics clone () {


Is this a new API in FontMetrics? In that case shouldn't it be added to all platforms?

I had no intention to add a cross-platform API.
Just wanted a convenience method to avoid copy&pasting code.

Does public part have to match on all platforms?
If yes, would you rather have the method on other platforms, or removed from macOS via moving it into TextLayout as a private helper?

Does public part have to match on all platforms?

Yes, otherwise there is a strong risk that the code written on a particular platform starts using those methods, and that as a result such code becomes non-portable, which is the opposite reason of why one would use SWT.

If yes, would you rather have the method on other platforms, or removed from macOS via moving it into TextLayout as a private helper?

Can't the method be only package visible here?

Ah right, I was confused because StyledText is in a different package. Will make it package-private then.

Yes, otherwise there is a strong risk that the code written on a particular platform starts using those methods, and that as a result such code becomes non-portable, which is the opposite reason of why one would use SWT.

+1

Yes, otherwise there is a strong risk that the code written on a particular platform starts using those methods, and that as a result such code becomes non-portable, which is the opposite reason of why one would use SWT.

To be fair we have this situation already with Windows OLE API .... in the end it might be better to throw unsupported operation and have the API everywhere as this is really a nightmare to develop.

To be fair we have this situation already with Windows OLE API ....

Win OLE api is in a platform specific package org.eclipse.swt.ole.win32, so all the classes in the package are platform specific - https://help.eclipse.org/latest/index.jsp?topic=/org.eclipse.platform.doc.isv/reference/api/org/eclipse/swt/ole/win32/package-summary.html

SyntevoAlex · 2024-02-06T03:03:55Z

@iloveeclipse automatic GitHub Compiler and API Tools verification now reports 99 new issues, and these look unrelated to my change. Should I just ignore it for now?

laeubi · 2024-02-06T05:32:17Z

@SyntevoAlex I have reset the quality gate and triggered a new master build, so next build should show "green" here after that.

No time, sorry

iloveeclipse · 2024-02-08T10:49:56Z

@SyntevoAlex : next week is M3 and then no changes of such kind will be allowed for 4.31 (usually).
Please rebase and merge if everything will be green (github indikates there were some changes on SWT in the meantime).

I personally have no time for a review, so it would be nice if someone would do that now.

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

To be implemented in next commits Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

…xedLineMetrics Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

…FixedLineMetrics Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

…awLineForeground() Needed to paint entire background in next commit. Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

…fore content Needed to avoid clipping tall unicode characters in fixedLineMetrics mode. Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

…FixedLineMetrics Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

SyntevoAlex · 2024-02-08T17:57:50Z

Thanks for the heads up.
All green, merged.

deepika-u · 2024-02-21T12:16:00Z

Verified this via #691 way of recreate.

Tried with below environment
Eclipse SDK
Version: 2024-03 (4.31)
Build id: I20240220-1800
OS: Windows 11, v.10.0, x86_64 / win32
Java vendor: Eclipse Adoptium
Java runtime version: 17.0.6+10
Java version: 17.0.6

SyntevoAlex requested review from lshanmug, niraj-modi, iloveeclipse and sravanlakkimsetti as code owners January 24, 2024 23:01

iloveeclipse previously requested changes Jan 25, 2024

View reviewed changes

iloveeclipse mentioned this pull request Jan 25, 2024

API errors not reported for PR's #1003

Closed

SyntevoAlex force-pushed the z_alexandr.miloslavskiy/#0113_SWT_Bug527917_StyledText_IncreasesRowHeightOnHieroglyph branch from d4dea60 to 8edce25 Compare January 31, 2024 12:05

SyntevoAlex force-pushed the z_alexandr.miloslavskiy/#0113_SWT_Bug527917_StyledText_IncreasesRowHeightOnHieroglyph branch 2 times, most recently from 70e7624 to 2967ed1 Compare January 31, 2024 22:44

lshanmug reviewed Feb 2, 2024

View reviewed changes

SyntevoAlex force-pushed the z_alexandr.miloslavskiy/#0113_SWT_Bug527917_StyledText_IncreasesRowHeightOnHieroglyph branch 2 times, most recently from 0ad2d77 to 4994559 Compare February 6, 2024 02:51

SyntevoAlex force-pushed the z_alexandr.miloslavskiy/#0113_SWT_Bug527917_StyledText_IncreasesRowHeightOnHieroglyph branch from 4994559 to 6278bc6 Compare February 6, 2024 11:40

SyntevoAlex added 3 commits February 8, 2024 20:47

Issue eclipse-platform#932: testing snippet

859e664

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

Issue eclipse-platform#932: new api: StyledText.setFixedLineMetrics

f3120f6

To be implemented in next commits Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

Issue eclipse-platform#932: [GTK] implementation for StyledText.setFi…

6d717bf

…xedLineMetrics Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

SyntevoAlex added 4 commits February 8, 2024 20:47

Issue eclipse-platform#932: [win32] implementation for StyledText.set…

e57ade8

…FixedLineMetrics Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

Issue eclipse-platform#932: Splitting out drawLineBackground() and dr…

6350180

…awLineForeground() Needed to paint entire background in next commit. Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

Issue eclipse-platform#932: StyledText: painting entire background be…

4d0f73b

…fore content Needed to avoid clipping tall unicode characters in fixedLineMetrics mode. Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

Issue eclipse-platform#932: [cocoa] implementation for StyledText.set…

c9449b7

…FixedLineMetrics Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

SyntevoAlex force-pushed the z_alexandr.miloslavskiy/#0113_SWT_Bug527917_StyledText_IncreasesRowHeightOnHieroglyph branch from 6278bc6 to c9449b7 Compare February 8, 2024 17:47

SyntevoAlex merged commit 0f7a1c6 into eclipse-platform:master Feb 8, 2024
13 checks passed

SyntevoAlex deleted the z_alexandr.miloslavskiy/#0113_SWT_Bug527917_StyledText_IncreasesRowHeightOnHieroglyph branch February 12, 2024 21:24

deepika-u mentioned this pull request Feb 21, 2024

StyledText increases row height (depending on text) #691

Closed

Issue 932: implement fixed line height for StyledText #1002

Issue 932: implement fixed line height for StyledText #1002

Conversation

SyntevoAlex commented Jan 24, 2024

SyntevoAlex commented Jan 24, 2024

github-actions bot commented Jan 24, 2024 • edited Loading

Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iloveeclipse commented Jan 25, 2024

merks commented Jan 25, 2024

iloveeclipse commented Jan 25, 2024

iloveeclipse commented Jan 31, 2024

iloveeclipse commented Jan 31, 2024

SyntevoAlex commented Jan 31, 2024

SyntevoAlex commented Jan 31, 2024

SyntevoAlex commented Jan 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyntevoAlex commented Feb 6, 2024

laeubi commented Feb 6, 2024

iloveeclipse commented Feb 8, 2024

SyntevoAlex commented Feb 8, 2024

deepika-u commented Feb 21, 2024

github-actions bot commented Jan 24, 2024 •

edited

Loading