On Thu, 29 Feb 2024 13:23:40 GMT, John Hendrikx <jhendr...@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
