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.

Sign up for GitHub

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

Corrected TextLayout to produce right number of lines when '\r\n' sequence comes. #1320

Merged
merged 1 commit into from
Jan 2, 2025
Merged

Corrected TextLayout to produce right number of lines when '\r\n' sequence comes. #1320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 33 additions & 11 deletions bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/TextLayout.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
*/
public final class TextLayout extends Resource {
Font font;
String text, segmentsText;
String text, segmentsText, originalText;
int lineSpacingInPoints;
int ascent, descent;
int alignment;
Expand Down Expand Up @@ -309,7 +309,7 @@ public TextLayout (Device device) {
styles[0] = new StyleItem();
styles[1] = new StyleItem();
stylesCount = 2;
text = ""; //$NON-NLS-1$
text = originalText = ""; //$NON-NLS-1$
long[] ppv = new long[1];
OS.OleInitialize(0);
if (COM.CoCreateInstance(COM.CLSID_CMultiLanguage, 0, COM.CLSCTX_INPROC_SERVER, COM.IID_IMLangFontLink2, ppv) == OS.S_OK) {
Expand Down Expand Up @@ -2724,8 +2724,16 @@ private int getScaledVerticalIndent() {
*/
public TextStyle getStyle (int offset) {
checkLayout();
int length = text.length();
int length = originalText.length();
if (!(0 <= offset && offset < length)) SWT.error(SWT.ERROR_INVALID_RANGE);
int crCount = 0, subStringLength = originalText.subSequence(0, offset+1).length();
for (int crIndex = 0; crIndex < subStringLength; crIndex++) {
if (originalText.charAt(crIndex) == '\r') {
crCount++;
}
}
offset = offset - crCount;

for (int i=1; i<stylesCount; i++) {
if (styles[i].start > offset) {
return styles[i - 1].style;
Expand Down Expand Up @@ -2956,15 +2964,11 @@ StyleItem[] merge (long items, int itemCount) {
linkBefore = false;
}
char ch = segmentsText.charAt(start);
switch (ch) {
case '\r':
case '\n':
item.lineBreak = true;
break;
case '\t':
item.tab = true;
break;
if (ch == '\r' && start + 1 < end) {
ch = segmentsText.charAt(start + 1);
}
item.lineBreak = ch == '\n';
item.tab = ch == '\t';
if (itemLimit == -1) {
nextItemIndex = itemIndex + 1;
OS.MoveMemory(scriptItem, items + nextItemIndex * SCRIPT_ITEM.sizeof, SCRIPT_ITEM.sizeof);
Expand Down Expand Up @@ -3455,6 +3459,22 @@ public void setStyle (TextStyle style, int start, int end) {
int length = text.length();
if (length == 0) return;
if (start > end) return;

int startCount = 0;
int endCount = 0;
for (int crIndex = originalText.indexOf('\r', 0); crIndex >= 0; crIndex = originalText.indexOf('\r', crIndex + 1)) {
if (crIndex < start) {
++startCount;
} else if (crIndex <= end) {
++endCount;
} else {
break;
}
}
endCount = endCount + startCount;
start -= startCount;
end -= endCount;

start = Math.min(Math.max(0, start), length - 1);
end = Math.min(Math.max(0, end), length - 1);
int low = -1;
Expand Down Expand Up @@ -3568,6 +3588,8 @@ public void setTabs (int[] tabs) {
*/
public void setText (String text) {
checkLayout();
this.originalText = text;
text = text.replace("\r", "");
if (text == null) SWT.error(SWT.ERROR_NULL_ARGUMENT);
if (text.equals(this.text)) return;
freeRuns();
Expand Down
81 changes: 81 additions & 0 deletions ...sts/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_TextLayout.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,60 @@ public void test_setStyle() {
assertNull(layout.getStyle(4));
assertNull(layout.getStyle(5));
layout.dispose();

layout = new TextLayout (display);
Copy link
Contributor

Choose a reason for hiding this comment

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

The test with these changes succeeds on master, so it's not a regression test for the changes made in this PR. Can you adapt the test so that it serves as a regression test for the addressed issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we have not changed the behavior of the api's. The original behavior of the api's still work the same. We are internally detecting, correcting and passing the corrected content in specific cases only. We dint even add new api's. It is only the look and feel by which you can make out if some extra lines are expected or not expected varies. - is my understanding.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I mistakenly expected the added test to be a regression test for the fix because of #1320 (comment)

Still, this change is no refactoring but affects the behavior of public APIs (e.g., TextLayout#draw(...) behaves differently). I guess that, e.g., also TextLayout#getBounds() now behaves differently, so wouldn't it be possible to have a regression test comparing the bounds of two configurations that should actually result in the bounds but did not do before this change (or vice versa)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HeikoKlare
As per your suggestion i have added new junit tests with TextLayout#getLineCount() and TextLayout#getBounds().
Since this change is only with respect to windows, i have excluded mac, gtk for the tests to run.

I guess this makes it all complete as we discussed. If i missed out any, please do let me know.

Can you review it and take it forward please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please hold on, let me cross check and update you again.

Copy link
Contributor Author

@deepika-u deepika-u Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @HeikoKlare
Sorry for the confusion caused.
I am successfull in adding only TextLayout#getLineCount() junit. I think we can go ahead with this pr now.

I have tried even TextLayout#getBounds() but had to withdraw as the behavior is not consistent across.
With the sample input taken,

  • On master, getBounds() is returning {0, 0, 30, 220} where width and height are 30, 220 respectively. [junit is failing]
  • On the branch where i committed my changes(fix_for_issue184), getBounds() is returning {0, 0, 43, 140}. [junit is passing]
  • To cross check, i have pulled the pr 1320 to local, getBounds() is returning {0, 0, 43, 140}. [junit is passing]
  • But on the build machine, getBounds() is returning {0, 0, 34, 105}. [junit is failing because it is getting 34 but expecting 43]. -> https://github.com/eclipse-platform/eclipse.platform.swt/actions/runs/12312890737/job/34365830603?pr=1320
    Neither able to understand the reason for the failure(to fix) nor able to replicate the problem locally to debug further.

Can you please check now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HeikoKlare : Need your time to take this forward please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HeikoKlare : Need your inputs here please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding the regression test. That one, testing getLineCount(), makes even more sense to me for testing the fix than testing the changed result of getBounds(). 👍

The PR is ready to go from my side.
Just a small final remark regarding the added test: I am just wondering whether it may not be even better to use different expected line count values for the different platforms in the test instead of excluding the test for GTK and Cocoa. Since the functionality is not broken or non-existent on those other platform but just produces expected different results, testing for those other results seems to be more comprehensible to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HeikoKlare
Thanks for your time. I have updated them now, can you take a look now. Assuming everything is done, can you take this forward please.

layout.setText("\rabc\ndef\r\rghi\n\njkl\r\nmno\r\n\r\npqr\r");
layout.setStyle(s1, 0, 5);//abcd
layout.setStyle (s2, 7, 10);//fg
layout.setStyle (s3, 11, 18);//hijkl
layout.setStyle (s4, 19, 24);//mno
assertEquals(s1, layout.getStyle(0));
assertEquals(s1, layout.getStyle(1));
assertEquals(s1, layout.getStyle(3));
assertEquals(s1, layout.getStyle(4));
assertEquals(s1, layout.getStyle(5));

assertEquals(s2, layout.getStyle(7));
assertEquals(s2, layout.getStyle(8));
assertEquals(s2, layout.getStyle(9));
assertEquals(s2, layout.getStyle(10));

assertEquals(s3, layout.getStyle(12));
assertEquals(s3, layout.getStyle(13));
assertEquals(s3, layout.getStyle(14));
assertEquals(s3, layout.getStyle(15));

assertEquals(s4, layout.getStyle(19));
assertEquals(s4, layout.getStyle(20));
assertEquals(s4, layout.getStyle(22));
assertEquals(s4, layout.getStyle(23));
assertEquals(s4, layout.getStyle(24));

layout.setStyle (null, 0, 3);//abc
assertNull(layout.getStyle(0));

layout.setStyle(s1, 0, 9);//abcdef
assertEquals(s1, layout.getStyle(8));

layout.setStyle (s2, 1, 24);//abcdefghijklmno
assertEquals(s2, layout.getStyle(7));
assertEquals(s2, layout.getStyle(8));
assertEquals(s2, layout.getStyle(14));
assertEquals(s2, layout.getStyle(18));
assertEquals(s2, layout.getStyle(22));
assertEquals(s2, layout.getStyle(23));
assertEquals(s2, layout.getStyle(24));

layout.setStyle (s3, 0, 30);//abcdefghijklmnopqr
assertEquals(s3, layout.getStyle(0));
assertEquals(s3, layout.getStyle(1));
assertEquals(s3, layout.getStyle(30));

layout.setStyle (s4, 1, 30);//abcdefghijklmnopqr
assertEquals(s4, layout.getStyle(1));
assertEquals(s4, layout.getStyle(29));
assertEquals(s4, layout.getStyle(30));
layout.dispose();
}

@Test
Expand Down Expand Up @@ -1219,4 +1273,31 @@ public void test_Bug579335_win32_StyledText_LongLine() {
font.dispose();
}
}

@Test
public void test_getLineCount() {
// Skipping this test for GTK and Cocoa as this pr #1320 applies only for
// windows platform.
// Without pr #1320(master) : Number of lines: 11 (for Cocoa it is : 10)
// Expected : Number of lines: 7 (for Cocoa it is : 10)
TextLayout layout = new TextLayout(display);
try {
String text = "\rabc\ndef\r\rghi\n\njkl\r\nmno\r\n\r\npqr\r";
layout.setText(text);
int lineCount = layout.getLineCount();

int expected = 0;
if (SwtTestUtil.isWindows) {
expected = 7;
} else if (SwtTestUtil.isCocoa) {
expected = 10;
} else if (SwtTestUtil.isGTK) {
expected = 11;
}

assertEquals(expected, lineCount);
} finally {
layout.dispose();
}
}
}
69 changes: 69 additions & 0 deletions ...se.swt.tests/ManualTests/org/eclipse/swt/tests/manual/Issue184_CarriageReturnHandled.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*******************************************************************************
* Copyright (c) 2024 IBM and others.
*
* This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
* which accompanies this distribution, and is available at
* https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*******************************************************************************/
package org.eclipse.swt.tests.manual;

import org.eclipse.swt.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.widgets.*;

public class Issue184_CarriageReturnHandled {

private static FontData[] getFontData(Font font, int style) {
FontData[] fontDatas = font.getFontData();
for (FontData fontData : fontDatas) {
fontData.setStyle(style);
}
return fontDatas;
}

public static void main(String[] args) {
Display display = new Display();
final Shell shell = new Shell(display, SWT.SHELL_TRIM | SWT.DOUBLE_BUFFERED);
shell.setText("Underline, Strike Out");
String text1 = "\r\nde\nep\rika\r\rudaya\n\ngiri\r";

FontData[] fontData = getFontData(shell.getFont(), SWT.BOLD);
Font font = new Font(shell.getDisplay(), fontData);

FontData[] fontData1 = getFontData(shell.getFont(), SWT.ITALIC | SWT.BOLD);
Font font1 = new Font(shell.getDisplay(), fontData1);

FontData[] fontData2 = getFontData(shell.getFont(), SWT.BOLD);
Font font2 = new Font(shell.getDisplay(), fontData2);

final TextLayout layout = new TextLayout(display);
layout.setText(text1);

TextStyle style1 = new TextStyle(font, null, null);
layout.setStyle(style1, 3, 7); // eep in bold

TextStyle style2 = new TextStyle(font1, null, null);
layout.setStyle(style2, 12, 18); // udaya in bold

TextStyle style3 = new TextStyle(font2, null, null);
layout.setStyle(style3, 21, 24); // iri in bold

shell.addListener(SWT.Paint, event -> {
Point point = new Point(10, 10);
int width = shell.getClientArea().width - 2 * point.x;
layout.setWidth(width);
layout.draw(event.gc, point.x, point.y);
});
shell.setSize(400, 300);
shell.open();
while (!shell.isDisposed()) {
if (!display.readAndDispatch())
display.sleep();
}
layout.dispose();
display.dispose();
}
}
Loading