RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v13]
John Hendrikx
jhendrikx at openjdk.org
Thu Feb 29 13:26:58 UTC 2024
On Tue, 27 Feb 2024 10:00:41 GMT, Karthik P K <kpk at openjdk.org> wrote:
>> I'm not sure if we could write headless test for this. Could you please point me to a test which could be helpful for me?
>
> I believe the tests added in this PR are helpful in making sure that the HitInfo calculation does not give results like character index less than 0 or character index greater than total length of the text or out of bound values. Since each character width is different and we have to check multiple scenarios, it is difficult to write generalised test for each case. Hence a combination of manual test using MonkeyTester and the automated tests added in this PR would be helpful in validating the changes.
I disagree on this. The code is complicated and full of branches. Manual testing is very error prone. However, since you restored most of the code to its original (which wasn't tested either) I could live with it. Still, here's a small test that you could use to verify correct hits:
/*
* Copyright (c) 2012, 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
* particular file as subject to the "Classpath" exception as provided
* by Oracle in the LICENSE file that accompanied this code.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
package test.com.sun.javafx.text;
import static org.junit.Assume.assumeTrue;
import static org.junit.jupiter.api.Assertions.assertEquals;
import org.junit.jupiter.api.Test;
import com.sun.javafx.font.PGFont;
import com.sun.javafx.geom.RectBounds;
import com.sun.javafx.scene.text.FontHelper;
import com.sun.javafx.scene.text.TextLayout.Hit;
import com.sun.javafx.scene.text.TextSpan;
import com.sun.javafx.text.PrismTextLayout;
import javafx.scene.text.Font;
public class TextLayoutTest {
private final PrismTextLayout layout = new PrismTextLayout();
private final PGFont arialFont = (PGFont) FontHelper.getNativeFont(Font.font("Arial", 12));
record TestSpan(String text, Object font) implements TextSpan {
@Override
public String getText() {
return text;
}
@Override
public Object getFont() {
return font;
}
@Override
public RectBounds getBounds() {
return null;
}
}
@Test
void getHitInfoTest() {
assumeArialFontAvailable();
/*
* Empty line:
*/
layout.setContent("", arialFont);
// Checks that hits above the line results in first character:
assertEquals(new Hit(0, 0, true), layout.getHitInfo(0, -30));
// Checks before start of line:
assertEquals(new Hit(0, 0, true), layout.getHitInfo(-50, 0));
// Checks position of empty string:
assertEquals(new Hit(0, 0, true), layout.getHitInfo(0, 0));
// Checks past end of line:
assertEquals(new Hit(0, 0, true), layout.getHitInfo(250, 0));
// TODO is insertion index correct here?
// Checks that hits below the line results in last character + 1:
assertEquals(new Hit(0, 1, false), layout.getHitInfo(0, 30));
/*
* Single line:
*/
// 01234567890123456789012345678901234567890123
layout.setContent("The quick brown fox jumps over the lazy dog", arialFont);
// Checks that hits above the line results in first character:
assertEquals(new Hit(0, 0, true), layout.getHitInfo(0, -30));
// Checks before start of line:
assertEquals(new Hit(0, 0, true), layout.getHitInfo(-50, 0));
// Checks positions of a few characters:
assertEquals(new Hit(0, 0, true), layout.getHitInfo(0, 0)); // Start of "T"
assertEquals(new Hit(0, 1, false), layout.getHitInfo(5, 0)); // Past halfway of "T"
assertEquals(new Hit(1, 1, true), layout.getHitInfo(10, 0)); // Start of "h"
// Checks past end of line:
assertEquals(new Hit(42, 43, false), layout.getHitInfo(250, 0));
// TODO is insertion index correct here?
// Checks that hits below the line results in last character + 1:
assertEquals(new Hit(43, 44, false), layout.getHitInfo(0, 30));
/*
* Multi line:
*/
layout.setContent("The\nquick\nbrown\nfox\n", arialFont);
// Checks that hits above the first line results in first character:
assertEquals(new Hit(0, 0, true), layout.getHitInfo(0, -30));
// Checks before start of first line:
assertEquals(new Hit(0, 0, true), layout.getHitInfo(-50, 0));
// Checks positions of a few characters on first line:
assertEquals(new Hit(0, 0, true), layout.getHitInfo(0, 0)); // Start of "T"
assertEquals(new Hit(0, 1, false), layout.getHitInfo(5, 0)); // Halfway past "T"
assertEquals(new Hit(1, 1, true), layout.getHitInfo(10, 0)); // Start of "h"
// Checks past end of first line:
assertEquals(new Hit(2, 3, false), layout.getHitInfo(250, 0));
// Checks before start of second line:
assertEquals(new Hit(4, 4, true), layout.getHitInfo(-50, 15));
// Check second line:
assertEquals(new Hit(4, 4, true), layout.getHitInfo(0, 15)); // Start of "q"
// Checks past end of second line:
assertEquals(new Hit(8, 9, false), layout.getHitInfo(250, 15));
/*
* Test with two spans:
*/
layout.setContent(new TestSpan[] {new TestSpan("Two", arialFont), new TestSpan("Spans", arialFont)});
// Checks that hits above the line results in first character:
assertEquals(new Hit(0, 0, true), layout.getHitInfo(0, -30));
// Checks before start of line:
assertEquals(new Hit(0, 0, true), layout.getHitInfo(-50, 0));
// Checks positions of a few characters:
assertEquals(new Hit(0, 0, true), layout.getHitInfo(0, 0)); // Start of "T"
assertEquals(new Hit(0, 1, false), layout.getHitInfo(5, 0)); // Past halfway of "T"
assertEquals(new Hit(1, 1, true), layout.getHitInfo(10, 0)); // Start of "w"
assertEquals(new Hit(7, 8, false), layout.getHitInfo(60, 0)); // Past halfway of "s"
// Checks past end of line:
assertEquals(new Hit(7, 8, false), layout.getHitInfo(250, 0));
// TODO is insertion index correct here?
// Checks that hits below the line results in last character + 1:
assertEquals(new Hit(8, 9, false), layout.getHitInfo(0, 30));
/*
* Test with zero spans:
*/
layout.setContent(new TestSpan[] {});
// Checks that hits above the line results in first character:
assertEquals(new Hit(0, 0, true), layout.getHitInfo(0, -30));
// Checks before start of line:
assertEquals(new Hit(0, 1, false), layout.getHitInfo(-50, 0));
// Checks positions of center:
assertEquals(new Hit(0, 1, false), layout.getHitInfo(0, 0)); // Start of "T"
// Checks past end of line:
assertEquals(new Hit(0, 1, false), layout.getHitInfo(250, 0));
// TODO is insertion index correct here?
// Checks that hits below the line results in last character + 1:
assertEquals(new Hit(0, 1, false), layout.getHitInfo(0, 30));
}
private void assumeArialFontAvailable() {
assumeTrue("Arial font missing", arialFont.getName().equals("Arial"));
}
}
Also add equals/hashCode/toString to `Hit`:
public static class Hit {
int charIndex;
int insertionIndex;
boolean leading;
public Hit(int charIndex, int insertionIndex, boolean leading) {
this.charIndex = charIndex;
this.insertionIndex = insertionIndex;
this.leading = leading;
}
public int getCharIndex() { return charIndex; }
public int getInsertionIndex() { return insertionIndex; }
public boolean isLeading() { return leading; }
@Override
public int hashCode() {
return Objects.hash(charIndex, insertionIndex, leading);
}
@Override
public boolean equals(Object obj) {
if (this == obj)
return true;
if (obj == null)
return false;
if (getClass() != obj.getClass())
return false;
Hit other = (Hit) obj;
return charIndex == other.charIndex && insertionIndex == other.insertionIndex && leading == other.leading;
}
@Override
public String toString() {
return "Hit[charIndex=" + charIndex + ", insertionIndex=" + insertionIndex + ", leading=" + leading + "]";
}
}
Now, I noticed that there are several code paths in `getHitInfo` can't be covered. This probably is either a lack of imagination on my part with my test, or (since I did try) these code paths are dead and can't be hit. The use of `BreakIterator` with the default `Locale` is also worrying -- I don't see how that could possibly do the correct thing.
Here's the coverage I did manage to get. I'm pretty sure the bottom `else` is dead code. The `BreakIterator` may still work when given special characters.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1507567984
More information about the openjfx-dev
mailing list