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

Jump to bottom

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+156 −11
Open

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
54 changes: 54 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)?

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
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