RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v13]

[Andy Goryachev]

On Thu, 29 Feb 2024 13:23:40 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> 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;
>         }
>   ...

@hjohn how do you get this coverage diagram?

The BreakIterator is a part of the existing code, we should probably have this discussion outside of this PR.  I agree, there might be a situation when the app wants to select a specific locale for the text component instead of relying on the default locale.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1507819707