Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v7]
On Thu, 1 Feb 2024 08:06:18 GMT, Karthik P K wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
>> line 513:
>>
>>> 511: if ((x > run.getWidth() &&
>>> (!isMultiRunText || run.getStart() == curRunStart)) || textWidthPrevLine >
>>> 0) {
>>> 512: getBounds(run.getTextSpan(),
>>> textBounds);
>>> 513: x -= (runs[0].getLocation().x -
>>> textBounds.getMinX());
>>
>> suggestion: we are still in the for loop, so perhaps it makes sense to
>> extract
>> `(runs[0].getLocation().x - textBounds.getMinX());`
>> to a variable outside of the loop
>
> The idea is that outside the loop we don't know if we need to subtract the
> textBound min x value and the starting location of the first run or not. That
> is why this is present inside the loop. Once this is done we are breaking out
> of the loop so this will not get called multiple times.
> Let me know if you have any suggestions.
you are right: I missed the `break` in line 520
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1474745002
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v7]
On Wed, 31 Jan 2024 21:09:53 GMT, Andy Goryachev wrote:
>> Karthik P K has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix issue with multiline text
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
> line 513:
>
>> 511: if ((x > run.getWidth() && (!isMultiRunText
>> || run.getStart() == curRunStart)) || textWidthPrevLine > 0) {
>> 512: getBounds(run.getTextSpan(),
>> textBounds);
>> 513: x -= (runs[0].getLocation().x -
>> textBounds.getMinX());
>
> suggestion: we are still in the for loop, so perhaps it makes sense to
> extract
> `(runs[0].getLocation().x - textBounds.getMinX());`
> to a variable outside of the loop
The idea is that outside the loop we don't know if we need to subtract the
textBound min x value and the starting location of the first run or not. That
is why this is present inside the loop. Once this is done we are breaking out
of the loop so this will not get called multiple times.
Let me know if you have any suggestions.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1473962144
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v7]
On Wed, 31 Jan 2024 10:24:20 GMT, Karthik P K wrote: >> In the `getHitInfo()` method of PrismTextLayout, RTL node orientation >> conditions were not considered, hence hit test values such as character >> index and insertion index values were incorrect. >> >> Added checks for RTL orientation of nodes and fixed the issue in >> `getHitInfo()` to calculate correct hit test values. >> >> Added system tests to validate the changes. > > Karthik P K has updated the pull request incrementally with one additional > commit since the last revision: > > Fix issue with multiline text I will work on the inline node issue and the issue in Rich text area. The rich text area issue is basically because of the repeated text node BOLD. Im not really sure if the increased scale is causing any issue, I will get into it. - PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1920730601
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v7]
On Wed, 31 Jan 2024 10:24:20 GMT, Karthik P K wrote: >> In the `getHitInfo()` method of PrismTextLayout, RTL node orientation >> conditions were not considered, hence hit test values such as character >> index and insertion index values were incorrect. >> >> Added checks for RTL orientation of nodes and fixed the issue in >> `getHitInfo()` to calculate correct hit test values. >> >> Added system tests to validate the changes. > > Karthik P K has updated the pull request incrementally with one additional > commit since the last revision: > > Fix issue with multiline text ... and got an exception hovering over "Rich Text" text on the TextFlow page: Exception in thread "JavaFX Application Thread" java.lang.IllegalArgumentException: offset is out of bounds: 7 at java.base/sun.util.locale.provider.BreakIteratorProviderImpl$GraphemeBreakIterator.following(BreakIteratorProviderImpl.java:255) at javafx.graphics/com.sun.javafx.text.PrismTextLayout.getHitInfo(PrismTextLayout.java:582) at javafx.graphics/javafx.scene.text.Text.hitTest(Text.java:1035) at monkey_tester/com.oracle.tools.fx.monkey.pages.TextFlowPage.handleMouseEvent(TextFlowPage.java:252) at javafx.base/com.sun.javafx.event.CompositeEventHandler$NormalEventHandlerRecord.handleBubblingEvent(CompositeEventHandler.java:247) at javafx.base/com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:80) at javafx.base/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:232) at javafx.base/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:189) at javafx.base/com.sun.javafx.event.CompositeEventDispatcher.dispatchBubblingEvent(CompositeEventDispatcher.java:59) at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:58) at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114) at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56) at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114) at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56) at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114) at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56) at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114) at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56) at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114) at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56) at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114) at javafx.base/com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74) at javafx.base/com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:54) at javafx.base/javafx.event.Event.fireEvent(Event.java:198) at javafx.base/com.sun.javafx.event.EventQueue.fire(EventQueue.java:48) at javafx.graphics/javafx.scene.Scene$MouseHandler.handleEnterExit(Scene.java:3901) at javafx.graphics/javafx.scene.Scene$MouseHandler.process(Scene.java:3972) at javafx.graphics/javafx.scene.Scene.processMouseEvent(Scene.java:1893) at javafx.graphics/javafx.scene.Scene$ScenePeerListener.mouseEvent(Scene.java:2711) at javafx.graphics/com.sun.javafx.tk.quantum.GlassViewEventHandler$MouseEventNotification.run(GlassViewEventHandler.java:411) at javafx.graphics/com.sun.javafx.tk.quantum.GlassViewEventHandler$MouseEventNotification.run(GlassViewEventHandler.java:1) at java.base/java.security.AccessController.doPrivileged(AccessController.java:400) at javafx.graphics/com.sun.javafx.tk.quantum.GlassViewEventHandler.lambda$2(GlassViewEventHandler.java:450) at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit.runWithoutRenderLock(QuantumToolkit.java:430) at javafx.graphics/com.sun.javafx.tk.quantum.GlassViewEventHandler.handleMouseEvent(GlassViewEventHandler.java:449) at javafx.graphics/com.sun.glass.ui.View.handleMouseEvent(View.java:551) at javafx.graphics/com.sun.glass.ui.View.notifyMouse(View.java:937) at javafx.graphics/com.sun.glass.ui.mac.MacView.notifyMouse(MacView.java:127)  - PR Comment:
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v7]
On Wed, 31 Jan 2024 10:24:20 GMT, Karthik P K wrote:
>> In the `getHitInfo()` method of PrismTextLayout, RTL node orientation
>> conditions were not considered, hence hit test values such as character
>> index and insertion index values were incorrect.
>>
>> Added checks for RTL orientation of nodes and fixed the issue in
>> `getHitInfo()` to calculate correct hit test values.
>>
>> Added system tests to validate the changes.
>
> Karthik P K has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix issue with multiline text
Almost works! At least all tests where only text segments are present: bidi,
RTL, mixed text, multi line.
It does fail when a Node is added to TextFlow, will explain in a separate
comment.
Inline Nodes mess up computation. In this example, the Text.hitInfo shows
bogus values anywhere in the word "trailing":
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 513:
> 511: if ((x > run.getWidth() && (!isMultiRunText
> || run.getStart() == curRunStart)) || textWidthPrevLine > 0) {
> 512: getBounds(run.getTextSpan(), textBounds);
> 513: x -= (runs[0].getLocation().x -
> textBounds.getMinX());
suggestion: we are still in the for loop, so perhaps it makes sense to extract
`(runs[0].getLocation().x - textBounds.getMinX());`
to a variable outside of the loop
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 517:
> 515: for (int j = runs.length - 1; j > i; j--) {
> 516: if (runs[j].getStart() != curRunStart &&
> runs[j].getTextSpan().getText().equals(text) && !runs[j].isLinebreak()) {
> 517: ltrIndex += runs[j].getLength();
runs[j] is repeated 4 times... should it be a local variable?
-
PR Review: https://git.openjdk.org/jfx/pull/1323#pullrequestreview-1854843704
PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-192439
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1473468220
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1473469342
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v7]
> In the `getHitInfo()` method of PrismTextLayout, RTL node orientation > conditions were not considered, hence hit test values such as character index > and insertion index values were incorrect. > > Added checks for RTL orientation of nodes and fixed the issue in > `getHitInfo()` to calculate correct hit test values. > > Added system tests to validate the changes. Karthik P K has updated the pull request incrementally with one additional commit since the last revision: Fix issue with multiline text - Changes: - all: https://git.openjdk.org/jfx/pull/1323/files - new: https://git.openjdk.org/jfx/pull/1323/files/a58f754e..52ee61cc Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1323=06 - incr: https://webrevs.openjdk.org/?repo=jfx=1323=05-06 Stats: 6 lines in 1 file changed: 0 ins; 2 del; 4 mod Patch: https://git.openjdk.org/jfx/pull/1323.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1323/head:pull/1323 PR: https://git.openjdk.org/jfx/pull/1323
