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

[Karthik P K]

On Thu, 15 Feb 2024 10:36:51 GMT, Karthik P K <kpk at openjdk.org> wrote:

>> I think we should simplify the `getHitInfo` method in the `TextLayout` interface.
>> 
>> The code currently seems to be making distinctions where there are none.  `TextFlow`s provide spans, while `Text`s provide a text. `getHitInfo` can take advantage of this to avoid the `text` and `forTextFlow` parameters altogether.  This will reduce confusion (as there is no distinction) and also avoids cases that are non-sensical (providing `text` while `forTextFlow` is `true` or vice versa).
>> 
>> Previous changes to this code (when the parameter `text` was introduced to `getHitInfo`) should probably be partially undone and simplified with this new knowledge.
>
>> I think we should simplify the `getHitInfo` method in the `TextLayout` interface.
>> 
>> The code currently seems to be making distinctions where there are none. `TextFlow`s provide spans, while `Text`s provide a text. `getHitInfo` can take advantage of this to avoid the `text` and `forTextFlow` parameters altogether. This will reduce confusion (as there is no distinction) and also avoids cases that are non-sensical (providing `text` while `forTextFlow` is `true` or vice versa).
>> 
>> Previous changes to this code (when the parameter `text` was introduced to `getHitInfo`) should probably be partially undone and simplified with this new knowledge.
> 
> Thanks for reviewing @hjohn 
> Certainly we could simplify the `getHitInfo` method but the complication starts when we have to support Text node embedded in TextFlow. 
> Just to differentiate Text node and TextFlow, spans can be used as you suggested. I had to introduce these parameters for the case of Text node embedded in TextFlow. On top of that we need to support emojis and RTL text. This is where it started getting complex and I had to use these new parameters. If you have any thoughts on this, please let me know.
> I'll got through all the comments and incorporate wherever possible.

> @karthikpandelu You mentioned there is special casing going on when a `Text` is part of a `TextFlow`. What are those cases and where is this happening? How does it even know that a `Text` is involved and that it is part of a `TextFlow`?
> 
We see these cases for every case when Text node is embedded in TextFlow. Basically we do not get x coordinate relative to the Text node since it is embedded in TextFlow. There are additional scenarios like RichText and Inline nodes present in TextFlow as well. 
In this case spans != null and text is supplied as parameter from Text class. This condition is considered as Text embedded in TextFlow.
Below is the simple test code which shows above mentioned case.


public class TextFlowRTLSample extends Application {
    TextFlow control = new TextFlow();
    Label hitInfo2 = new Label();
    Label pickResult = new Label();

    @Override
    public void start(Stage primaryStage) throws Exception {
        Node[] ts = createRTLText();

        control.getChildren().addAll(ts);
        control.addEventHandler(MouseEvent.ANY, this::handleMouseEvent);
        VBox root = new VBox(control, hitInfo2, pickResult);

        Scene scene = new Scene(root, 400, 600);
        scene.setNodeOrientation(NodeOrientation.RIGHT_TO_LEFT);
        primaryStage.setScene(scene);
        primaryStage.show();
    }

    protected static Text t(String text) {
        Text t = new Text(text);
        t.setFont(new Font(48));
        return t;
    }
    protected Node[] createRTLText() {
        return new Node[] {
                t("Arabic**"),
                t("شسيبلاتنمح"),
        };
    }
    protected void handleMouseEvent(MouseEvent ev) {
        PickResult pick = ev.getPickResult();
        Node n = pick.getIntersectedNode();
        hitInfo2.setText(null);
        if (n == null) {
            pickResult.setText("null");
        } else {
            pickResult.setText(n.getClass().getSimpleName() + "." + n.hashCode());
            if (n instanceof Text t) {
                Point3D p3 = pick.getIntersectedPoint();
                Point2D p = new Point2D(p3.getX(), p3.getY());
                HitInfo h = t.hitTest(p);
                hitInfo2.setText("Text: " + String.valueOf(h));
            }
        }
    }

    public static void main(String[] args) {
        launch();
    }
}


> However, that would break down if there are also special cases for `Text` within `TextFlow` -- I can't think of what those would be, and if it indeed is the case, I strongly suspect that `PrismTextLayout` may be overstepping its mandate (ie, such concerns when a `Text` is nested in a `TextFlow` should be the concern of `TextFlow`). Since I think simplifying this class would be a very good step, I'd just like to ensure we're not introducing more special casing now to make this future work even more impossible.

I agree that simplifying would help in making this class clean, but at the same time I believe these changes are necessary to handle Text and TextFlow.

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

PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1952485420