Re: RFR: 8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs [v8]
On Mon, 22 May 2023 16:30:40 GMT, Kevin Rushforth wrote: > Can you file a new (P4) testbug as a follow-up issue to use it replace the > two existing uses of `firePulse`? Created [JDK-8308608](https://bugs.openjdk.org/browse/JDK-8308608) - PR Comment: https://git.openjdk.org/jfx/pull/1091#issuecomment-1558550971
Re: RFR: 8301312: Create implementation of NSAccessibilityButton protocol [v4]
On Wed, 17 May 2023 20:31:27 GMT, Alexander Zuev wrote: >> Add the common base component for all the new implementing native classes >> Change native peer creation to use the new base component The new code will >> instantiate new protocol implementation for the given role if it exists or >> an old one if it does not exist >> Added BUTTON role implementing class > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > Properly use NSObject instead of the GlassAccessible type. Initial testing looks good. I'll do more testing soon. I did get a one-time exception while playing around with Ensemble8, I can't reproduce it. It was in the existing code, so it might be unrelated to your change. I left a couple inline questions. I noticed that your branch is about 3 months out of date. Can you merge the latest master. modules/javafx.graphics/src/main/native-glass/mac/a11y/AccessibleBase.m line 124: > 122: > 123: // Actions support > 124: - (BOOL)performAccessibleAction:(jlong)actionId Would it make sense to have this parameter be an `NSString` (like the similar method in `GlassAcessible`) and do the cast to `jlong` in this method rather than having all the callers do it? Of course, that only works if all of the calls will use NSString, so what you have is more flexible. modules/javafx.graphics/src/main/native-glass/mac/a11y/AccessibleBase.m line 206: > 204: NSObject* accessible = (NSObject*)jlong_to_ptr(macAccessible); > 205: return ptr_to_jlong(NSAccessibilityUnignoredAncestor(accessible)); > 206: } Why was this method moved from GlassAccessible.m to here? It seems fine, I was just curious. - PR Review: https://git.openjdk.org/jfx/pull/1084#pullrequestreview-1437812622 PR Review Comment: https://git.openjdk.org/jfx/pull/1084#discussion_r1201258731 PR Review Comment: https://git.openjdk.org/jfx/pull/1084#discussion_r1201254372
Re: Navigation KeyEvents consumed when there is nothing to navigate, bug?
I'm talking about the FX versions of these events:
/**
* This event occurs when a key has been pressed.
*/
public static final EventType KEY_PRESSED =
new EventType(KeyEvent.ANY, "KEY_PRESSED");
/**
* This event occurs when a key has been released.
*/
public static final EventType KEY_RELEASED =
new EventType(KeyEvent.ANY, "KEY_RELEASED");
/**
* This event occurs when a character-generating key was typed
* (pressed and released). The event contains the {@code character}
* field containing the typed string, the {@code code} and {@code text}
* fields are not used.
*/
public static final EventType KEY_TYPED =
new EventType(KeyEvent.ANY, "KEY_TYPED");
For navigation keys, KEY_PRESSED is swallowed whether it can be used or
not. KEY_TYPED comes through (but not for cursor keys as they have no
character, but tab does). KEY_RELEASED also happens (but only after the
key is released, key repeat generated more KEY_PRESSED events which are
all swallowed).
--John
On 22/05/2023 23:09, Michael Strauß wrote:
The only key events in WinUI are KeyDown and KeyUp (both are defined
on the UIElement class).
Tab keys are not consumed for both KeyDown and KeyUp (i.e. a parent
element receives the events with regular KeyDown/Up handlers), while
action keys are always consumed.
On Mon, May 22, 2023 at 10:05 PM John Hendrikx wrote:
You mean KEY_TYPED or KEY_PRESSED ? In my experience, it consumes only
the PRESSED events while TYPED and RELEASED are left alone.
On 22/05/2023 20:37, Michael Strauß wrote:
Just to add a data point:
I've tried to recreate this scenario in a WinUI 3 application. A
single button, and nowhere to shift focus.
While the button does consume action keys (space and enter), it does
not consume the tab key.
Interestingly, it also doesn't consume the tab key when the focus
_can_ be moved to a different control.
Re: RFR: 8304041: [WebView] Text on the left and right of emoji is not visible
On Thu, 6 Apr 2023 20:00:01 GMT, Phil Race wrote: > This fixes a problem with mixing Emoji and regular text in Webview. > The Prism implementation expects that Emoji will not be in the same call to > drawString as regular glyphs, > since they need very different handling. > This fix breaks up the runs webview hands to Prism. > The test program in the bug report now renders properly (which it never did > before in any release) This PR is still being worked on. - PR Comment: https://git.openjdk.org/jfx/pull/1083#issuecomment-1558101205
Re: RFR: 8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs [v8]
On Mon, 22 May 2023 17:14:30 GMT, Andy Goryachev wrote: > what do you people think of the number of pulses parameter? is 10 enough? is > there a reasonable upper limit that's needed for any processing to settle? I think 10 should be a reasonable number of pulses for our tests. Most graphs settle down within 3 layout passes. > another question is whether it could be made static - i.e. not require a > Scene. Perhaps we could add a pulse listener to all visible Scenes? Would > that be sufficient, since we only need a signal from one to indicate the > pulse has been performed, and there is another and another (... up to 10 or > N) pulses that basically guarantees that things are settled. I doubt this is worth it. This is a test utility, and if one of the very few tests that have multiple scenes needed to call it, it wouldn't be hard to have the test call it for each scene (and would give the test more control). > And if the layout has not settled by that time, we have a bigger issue (can > we detect this? I've seen continuous layout loops in one of my programs) Right. This is tangential to this PR, but at some point we might want to resurrect the discussion on PR #433 to see if we can improve the current situation. - PR Comment: https://git.openjdk.org/jfx/pull/1091#issuecomment-1558059567
Re: RFR: 8306329 : Update ICU4C to 73.1 [v4]
On Thu, 18 May 2023 17:22:46 GMT, Hima Bindu Meda wrote: >> Updated ICU to v73.1. Verified build and sanity. No issues seen. > > Hima Bindu Meda has updated the pull request incrementally with one > additional commit since the last revision: > > Update preamble in LICENSE @johanvos @tiainen Can one of you be the second reviewer? - PR Comment: https://git.openjdk.org/jfx/pull/1138#issuecomment-1558038710
Re: RFR: 8307316: Let JavaFX be built on unknown architectures
On Wed, 3 May 2023 16:02:34 GMT, John Neffenger wrote:
> Please review these changes to the Gradle build files and the dependency
> verification file. The initial version of this pull request extends the
> permitted build platforms for Linux to the Java architectures `arm`,
> `ppc64le`, and `s390x` and adds an entry to the dependency verification file
> for the `i386` architecture. The Debian names for these architectures are
> `armhf`, `i386`, `ppc64el`, and `s390x`.
Changing the errors to warnings seems good, as does adding the additional
artifact. I left a question about the `IS_64` change.
buildSrc/linux.gradle line 48:
> 46: "-Wextra", "-Wall", "-Wformat-security", "-Wno-unused",
> "-Wno-parentheses", "-Werror=trampolines"] // warning flags
> 47:
> 48: if (OS_ARCH == "i386") {
Why was this change needed, and is it sufficient? It seems that there is a
larger problem (likely out of scope for this fix) as to how `IS_64` is set that
might need follow-up.
-
PR Review: https://git.openjdk.org/jfx/pull/1124#pullrequestreview-1437626675
PR Review Comment: https://git.openjdk.org/jfx/pull/1124#discussion_r1201130504
Re: Navigation KeyEvents consumed when there is nothing to navigate, bug?
The only key events in WinUI are KeyDown and KeyUp (both are defined on the UIElement class). Tab keys are not consumed for both KeyDown and KeyUp (i.e. a parent element receives the events with regular KeyDown/Up handlers), while action keys are always consumed. On Mon, May 22, 2023 at 10:05 PM John Hendrikx wrote: > > You mean KEY_TYPED or KEY_PRESSED ? In my experience, it consumes only > the PRESSED events while TYPED and RELEASED are left alone. > > On 22/05/2023 20:37, Michael Strauß wrote: > > Just to add a data point: > > I've tried to recreate this scenario in a WinUI 3 application. A > > single button, and nowhere to shift focus. > > While the button does consume action keys (space and enter), it does > > not consume the tab key. > > Interestingly, it also doesn't consume the tab key when the focus > > _can_ be moved to a different control.
Re: Navigation KeyEvents consumed when there is nothing to navigate, bug?
You mean KEY_TYPED or KEY_PRESSED ? In my experience, it consumes only the PRESSED events while TYPED and RELEASED are left alone. On 22/05/2023 20:37, Michael Strauß wrote: Just to add a data point: I've tried to recreate this scenario in a WinUI 3 application. A single button, and nowhere to shift focus. While the button does consume action keys (space and enter), it does not consume the tab key. Interestingly, it also doesn't consume the tab key when the focus _can_ be moved to a different control.
Re: Navigation KeyEvents consumed when there is nothing to navigate, bug?
Just to add a data point: I've tried to recreate this scenario in a WinUI 3 application. A single button, and nowhere to shift focus. While the button does consume action keys (space and enter), it does not consume the tab key. Interestingly, it also doesn't consume the tab key when the focus _can_ be moved to a different control.
Re: Should javafx.scene.robot.Robot be able to use nested event loops?
On 2023-05-22T07:42:23 -0700 Martin Fox wrote: > Mark, > > Do you have a sample app that exhibits this behavior? I can’t reproduce it on > Mac or Windows. > > I can’t think of anything on the Robot side that would cause this. A Robot > just asks the OS to post an event. The event isn’t targeted at a specific > process or window and the Robot has no control over where it ends up. If it > does get delivered to your app it should behave exactly like a user generated > event. > > If the user can close the window it means there’s an event loop running and > that should be draining the OS queue. So I’m puzzled by what’s going on here. > > Anyway, not expected behavior. Still a mystery. Amusingly, I spent half an hour extracting the smallest code sample that would reproduce it and, of course, it turned out I couldn't reproduce it. As I mentioned, my mail hasn't been reaching the list, and I first ran into this a few weeks back. It's possible that a JavaFX update in the intervening weeks fixed the root cause (I likely wasn't even up-to-date when I ran into it, so it could be any reasonably recent release that fixed it). I'm back working on the original ITs now, so I'll see if I run into it again. -- Mark Raynsford | https://www.io7m.com
Re: RFR: 8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs [v8]
On Mon, 22 May 2023 14:21:33 GMT, Karthik P K wrote: >> Since surrogate pairs are internally considered as 2 characters and text >> field is null in `HitInfo` when `getInsertionIndex` is invoked from >> `TextFlow`, wrong insertion index was returned. >> >> Updated code to calculate insertion index in `getHitInfo` method of >> `PrismTextLayout` class when `hitTest` of trailing side of surrogate pair is >> requested. Since text runs are processed in this method already, calculating >> the insertion index in this method looks better than calculating in >> `getInsertionIndex` of `HitInfo` method. >> The latter approach also requires the text to be sent to `HitInfo` as >> parameter from the `hitTest` method of `TextFlow`. If the number of `Text` >> nodes in `TextFlow` are very large, processing all the `Text` nodes on each >> `hitTest` method invocation might cause performance issue. Hence implemented >> first approach. >> >> Added system test to validate the fix. > > Karthik P K has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - Merge master to resolve merge conflict > - Stabilizing the system tests > - Add new test. Optimizations to make the tests robust > - Change insertion index initialization approach. Add additional test > - Initialize insertion index in PrismTextLayout > - Address code review > - Fix insertion index calculation issue with emojis. Add additional test > cases > - Fix insertion index calculation issue in TextFlow what do you people think of the number of pulses parameter? is 10 enough? is there a reasonable upper limit that's needed for any processing to settle? another question is whether it could be made static - i.e. not require a Scene. Perhaps we could add a pulse listener to all visible Scenes? Would that be sufficient, since we only need a signal from one to indicate the pulse has been performed, and there is another and another (... up to 10 or N) pulses that basically guarantees that things are settled. And if the layout has not settled by that time, we have a bigger issue (can we detect this? I've seen continuous layout loops in one of my programs) - PR Comment: https://git.openjdk.org/jfx/pull/1091#issuecomment-1557601430
Re: RFR: 8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs [v8]
On Mon, 22 May 2023 14:21:33 GMT, Karthik P K wrote: >> Since surrogate pairs are internally considered as 2 characters and text >> field is null in `HitInfo` when `getInsertionIndex` is invoked from >> `TextFlow`, wrong insertion index was returned. >> >> Updated code to calculate insertion index in `getHitInfo` method of >> `PrismTextLayout` class when `hitTest` of trailing side of surrogate pair is >> requested. Since text runs are processed in this method already, calculating >> the insertion index in this method looks better than calculating in >> `getInsertionIndex` of `HitInfo` method. >> The latter approach also requires the text to be sent to `HitInfo` as >> parameter from the `hitTest` method of `TextFlow`. If the number of `Text` >> nodes in `TextFlow` are very large, processing all the `Text` nodes on each >> `hitTest` method invocation might cause performance issue. Hence implemented >> first approach. >> >> Added system test to validate the fix. > > Karthik P K has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - Merge master to resolve merge conflict > - Stabilizing the system tests > - Add new test. Optimizations to make the tests robust > - Change insertion index initialization approach. Add additional test > - Initialize insertion index in PrismTextLayout > - Address code review > - Fix insertion index calculation issue with emojis. Add additional test > cases > - Fix insertion index calculation issue in TextFlow Btw, I took a look at the new `Util::waitForIdle` method and it looks great. Can you file a new (P4) testbug as a follow-up issue to use it replace the two existing uses of `firePulse`? - PR Comment: https://git.openjdk.org/jfx/pull/1091#issuecomment-1557541177
Re: RFR: 8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs [v8]
On Mon, 22 May 2023 14:21:33 GMT, Karthik P K wrote: >> Since surrogate pairs are internally considered as 2 characters and text >> field is null in `HitInfo` when `getInsertionIndex` is invoked from >> `TextFlow`, wrong insertion index was returned. >> >> Updated code to calculate insertion index in `getHitInfo` method of >> `PrismTextLayout` class when `hitTest` of trailing side of surrogate pair is >> requested. Since text runs are processed in this method already, calculating >> the insertion index in this method looks better than calculating in >> `getInsertionIndex` of `HitInfo` method. >> The latter approach also requires the text to be sent to `HitInfo` as >> parameter from the `hitTest` method of `TextFlow`. If the number of `Text` >> nodes in `TextFlow` are very large, processing all the `Text` nodes on each >> `hitTest` method invocation might cause performance issue. Hence implemented >> first approach. >> >> Added system test to validate the fix. > > Karthik P K has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - Merge master to resolve merge conflict > - Stabilizing the system tests > - Add new test. Optimizations to make the tests robust > - Change insertion index initialization approach. Add additional test > - Initialize insertion index in PrismTextLayout > - Address code review > - Fix insertion index calculation issue with emojis. Add additional test > cases > - Fix insertion index calculation issue in TextFlow @prrace Would you be willing to take a look? - PR Comment: https://git.openjdk.org/jfx/pull/1091#issuecomment-1557495583
Re: RFR: 8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs [v6]
On Fri, 19 May 2023 16:24:44 GMT, Andy Goryachev wrote:
>> Adding `Toolkit::firePulse` is not making the tests stable. In fact I'm
>> getting new failure after adding this in
>> `testTextFlowInsertionIndexUsingMultipleEmojis()` test.
>>
>> TextFlowSurrogatePairInsertionIndexTest >
>> testTextFlowInsertionIndexUsingMultipleEmojis FAILED
>> java.lang.NullPointerException: Cannot read the array length because
>> "runs" is null
>> at
>> javafx.graphics@21-internal/javafx.scene.text.Text.getSpanBounds(Text.java:359)
>> at
>> javafx.graphics@21-internal/javafx.scene.text.Text.doComputeGeomBounds(Text.java:1159)
>> at
>> javafx.graphics@21-internal/javafx.scene.text.Text$1.doComputeGeomBounds(Text.java:149)
>> at
>> javafx.graphics@21-internal/com.sun.javafx.scene.shape.TextHelper.computeGeomBoundsImpl(TextHelper.java:90)
>> at
>> javafx.graphics@21-internal/com.sun.javafx.scene.NodeHelper.computeGeomBounds(NodeHelper.java:117)
>> at
>> javafx.graphics@21-internal/javafx.scene.Node.updateGeomBounds(Node.java:3813)
>> at
>> javafx.graphics@21-internal/javafx.scene.Node.getGeomBounds(Node.java:3775)
>> at
>> javafx.graphics@21-internal/javafx.scene.Node.getLocalBounds(Node.java:3723)
>> at
>> javafx.graphics@21-internal/javafx.scene.Node.updateTxBounds(Node.java:3877)
>> at
>> javafx.graphics@21-internal/javafx.scene.Node.getTransformedBounds(Node.java:3669)
>> at
>> javafx.graphics@21-internal/javafx.scene.Node.updateBounds(Node.java:777)
>> at
>> javafx.graphics@21-internal/javafx.scene.Parent.updateBounds(Parent.java:1834)
>> at
>> javafx.graphics@21-internal/javafx.scene.Scene$ScenePulseListener.pulse(Scene.java:2615)
>> at
>> javafx.graphics@21-internal/com.sun.javafx.tk.Toolkit.lambda$runPulse$2(Toolkit.java:401)
>> at
>> java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
>> at
>> javafx.graphics@21-internal/com.sun.javafx.tk.Toolkit.runPulse(Toolkit.java:400)
>> at
>> javafx.graphics@21-internal/com.sun.javafx.tk.Toolkit.firePulse(Toolkit.java:430)
>> at
>> test.robot.javafx.scene.TextFlowSurrogatePairInsertionIndexTest.testTextFlowInsertionIndexUsingMultipleEmojis(TextFlowSurrogatePairInsertionIndexTest.java:233)
>>
>> Could increasing the delay time is the only solution now for the test
>> failure seen previously and then we follow up with the approach suggested by
>> @kevinrushforth ?
>>
>> @andy-goryachev-oracle could you please help me in understanding how
>> `Util.runAndWait(() -> { });` help in this case?
>> Also I'm using the...
>
>> could you please help me in understanding how `Util.runAndWait(() -> { });`
>> help in this case
>
> After discussing the issue with @kevinrushforth it probably won't.
>
> The idea was to replace Thread.sleep() with something that can work reliably
> even when the process takes a long time (i.e. cold boot). Simply increasing
> the sleep time to 1000 ms may not be sufficient, I think.
>
> What we need is an equivalent of java.awt.Robot.waitForIdle() which we don't
> have in FX. The problem here is that all the processing involving CSS,
> layout may take several pulses, and it certainly not guaranteed to be over
> when the next event is processed in `Util.runAndWait()`.
>
> We could still use Toolkit.firePulse(), but this apparently is a hack, and it
> alters the normal control flow - that is something we are trying to avoid.
>
> Another variant is to add something like that to Util and use that in place
> of Thread.sleep(). This method will trigger and wait for an arbitrary number
> of pulses (currently 10, but we can pick any reasonable number):
>
>
> /**
> * Triggers and waits for 10 pulses to complete in the specified scene.
> */
> public static void waitForIdle(Scene scene) {
> int count = 10;
> CountDownLatch latch = new CountDownLatch(count);
> Runnable pulseListener = () -> {
> latch.countDown();
> Platform.requestNextPulse();
> };
>
> runAndWait(() -> {
> scene.addPostLayoutPulseListener(pulseListener);
> });
>
> try {
> Platform.requestNextPulse();
> waitForLatch(latch, 30, "waitForIdle timeout");
> } finally {
> runAndWait(() -> {
> scene.removePostLayoutPulseListener(pulseListener);
> });
> }
> }
>
>
> I am not sure why we have Scene.addPulseListener() and not a static
> equivalent of Robot.waitForIdle(), but here is some earlier work on the pulse
> listener:
>
> https://bugs.openjdk.org/browse/JDK-8097917
@andy-goryachev-oracle Do you think this needs a second reviewer, or are you
satisfied that a single review is sufficient?
-
PR Comment: https://git.openjdk.org/jfx/pull/1091#issuecomment-1557471464
Re: RFR: 8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs [v6]
On Mon, 22 May 2023 15:52:55 GMT, Kevin Rushforth wrote:
>>> could you please help me in understanding how `Util.runAndWait(() -> { });`
>>> help in this case
>>
>> After discussing the issue with @kevinrushforth it probably won't.
>>
>> The idea was to replace Thread.sleep() with something that can work reliably
>> even when the process takes a long time (i.e. cold boot). Simply increasing
>> the sleep time to 1000 ms may not be sufficient, I think.
>>
>> What we need is an equivalent of java.awt.Robot.waitForIdle() which we don't
>> have in FX. The problem here is that all the processing involving CSS,
>> layout may take several pulses, and it certainly not guaranteed to be over
>> when the next event is processed in `Util.runAndWait()`.
>>
>> We could still use Toolkit.firePulse(), but this apparently is a hack, and
>> it alters the normal control flow - that is something we are trying to avoid.
>>
>> Another variant is to add something like that to Util and use that in place
>> of Thread.sleep(). This method will trigger and wait for an arbitrary
>> number of pulses (currently 10, but we can pick any reasonable number):
>>
>>
>> /**
>> * Triggers and waits for 10 pulses to complete in the specified scene.
>> */
>> public static void waitForIdle(Scene scene) {
>> int count = 10;
>> CountDownLatch latch = new CountDownLatch(count);
>> Runnable pulseListener = () -> {
>> latch.countDown();
>> Platform.requestNextPulse();
>> };
>>
>> runAndWait(() -> {
>> scene.addPostLayoutPulseListener(pulseListener);
>> });
>>
>> try {
>> Platform.requestNextPulse();
>> waitForLatch(latch, 30, "waitForIdle timeout");
>> } finally {
>> runAndWait(() -> {
>> scene.removePostLayoutPulseListener(pulseListener);
>> });
>> }
>> }
>>
>>
>> I am not sure why we have Scene.addPulseListener() and not a static
>> equivalent of Robot.waitForIdle(), but here is some earlier work on the
>> pulse listener:
>>
>> https://bugs.openjdk.org/browse/JDK-8097917
>
> @andy-goryachev-oracle Do you think this needs a second reviewer, or are you
> satisfied that a single review is sufficient?
I don't mind someone else take a look at this, @kevinrushforth .
-
PR Comment: https://git.openjdk.org/jfx/pull/1091#issuecomment-1557477810
Re: RFR: 8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs [v6]
On Fri, 19 May 2023 15:41:50 GMT, Andy Goryachev wrote:
> For the purposes of this test, we could remove this sequence from the test, I
> think.
Removed red heart from the text sequence.
> Another variant is to add something like that to Util and use that in place
> of Thread.sleep(). This method will trigger and wait for an arbitrary number
> of pulses (currently 10, but we can pick any reasonable number):
Added above suggested method in Util and tests looks to be stable now.
Currently waiting for 10 pulses as suggested. Added parameterized method to
specify the number of pulses to be waited as well.
Also removed the unnecessary latches as I was already using `Util.runAndWait(()
-> { });`.
-
PR Comment: https://git.openjdk.org/jfx/pull/1091#issuecomment-1557312984
Re: RFR: 8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs [v8]
On Mon, 22 May 2023 14:21:33 GMT, Karthik P K wrote: >> Since surrogate pairs are internally considered as 2 characters and text >> field is null in `HitInfo` when `getInsertionIndex` is invoked from >> `TextFlow`, wrong insertion index was returned. >> >> Updated code to calculate insertion index in `getHitInfo` method of >> `PrismTextLayout` class when `hitTest` of trailing side of surrogate pair is >> requested. Since text runs are processed in this method already, calculating >> the insertion index in this method looks better than calculating in >> `getInsertionIndex` of `HitInfo` method. >> The latter approach also requires the text to be sent to `HitInfo` as >> parameter from the `hitTest` method of `TextFlow`. If the number of `Text` >> nodes in `TextFlow` are very large, processing all the `Text` nodes on each >> `hitTest` method invocation might cause performance issue. Hence implemented >> first approach. >> >> Added system test to validate the fix. > > Karthik P K has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - Merge master to resolve merge conflict > - Stabilizing the system tests > - Add new test. Optimizations to make the tests robust > - Change insertion index initialization approach. Add additional test > - Initialize insertion index in PrismTextLayout > - Address code review > - Fix insertion index calculation issue with emojis. Add additional test > cases > - Fix insertion index calculation issue in TextFlow verified on macOS, Windows 11. looks good! - Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1091#pullrequestreview-1436893604
Re: Navigation KeyEvents consumed when there is nothing to navigate, bug?
I assume these events are consumed so the user doesn’t encounter surprising behavior when they hit the limits of the navigable area. If the user presses a navigation key a few too many times or holds it down to auto-repeat the navigation should either stop when they hit the edge or wrap around to the other side. If the extra keystrokes are left unconsumed they might be handled in some unexpected way. That’s my conjecture, I haven’t tweaked the code and tried it out myself. > On May 18, 2023, at 7:01 PM, John Hendrikx wrote: > > My question is: > > When a control has focus, and a navigation key is pressed that has no effect, > should the navigation key be consumed? > > With no effect I mean: > > - TAB / SHIFT-TAB is not changing focus because there is only a single > control that can receive focus (the tab keys will loop back to first/last > controls) > - UP/DOWN/LEFT/RIGHT keys is not changing focus because there is no control > that can receive focus that is further up, down, left or right than the > current control > > IMHO, it shouldn't because events that you're not acting upon should not be > consumed. > > My use case where this matters: > > I have a fully keyboard controlled application. When pressing a hotkey to > change some state, a popup window appears briefly confirming the state > change. This popup can contain a fully functioning control (like a slider, > spinner or checkbox) to show the current state, but also to interact with it > further (using keys only). Any event that is not relevant for the popup will > close the popup immediately. In other words, I press a key to increase > volume, a volume control briefly appears, and when I press LEFT/RIGHT (the > volume control is horizontally orientated) I can also adjust the volume > further. When I press a key not handled by the popup, the popup closes and > the event is forwarded to the original Scene -- this works for all keys, > except the navigation keys. > > So, the Navigation keys are always consumed by the focused control in the > popup, whether it can use them or not. A slider can use left/right or > up/down depending on orientation, but the other keys should bubble through, > yet they're all consumed. So when I press UP or DOWN for a horizontal > slider, it tries to navigate to another control, but there is none (the popup > only contains one control). The key is however consumed despite not being > usable. > > --John > >
Re: Should javafx.scene.robot.Robot be able to use nested event loops?
Mark, Do you have a sample app that exhibits this behavior? I can’t reproduce it on Mac or Windows. I can’t think of anything on the Robot side that would cause this. A Robot just asks the OS to post an event. The event isn’t targeted at a specific process or window and the Robot has no control over where it ends up. If it does get delivered to your app it should behave exactly like a user generated event. If the user can close the window it means there’s an event loop running and that should be draining the OS queue. So I’m puzzled by what’s going on here. Anyway, not expected behavior. Still a mystery. Martin > On May 20, 2023, at 2:19 PM, Mark Raynsford wrote: > > (Apologies for what might be the third duplicate to the list; for some > reason list mail is reaching me, but none of my messages to the list > are getting through). > > Hello! > > I've been putting together some integration tests using > javafx.scene.robot.Robot for an application. I've noticed that if the > application creates a Stage and then calls showAndWait() on it, there > appears to be no way to get a robot to send events to that stage. If > that Stage is opened during an integration test, and then manually > closed by the user, all of the events (mouse clicks, keys typed, etc) > that _would_ have been sent are immediately applied in the context of > the outer event loop. > > Is this expected behaviour? The various Robot implementation classes > are suspiciously short, so I'm guessing that this is something that's > just not supported. > > -- > Mark Raynsford | https://www.io7m.com
Re: RFR: 8301763: Adding children to wrong index leaves inconsistent state in Parent#childrenSet [v3]
On Mon, 22 May 2023 14:01:04 GMT, Lukasz Kostyra wrote: >> This issue happened because `childSet` member of Parent was modified during >> `onProposedChange()` call - that call did not recognize negative indexes as >> invalid, which caused an exception when actually adding the Node to a List. >> >> This seemed like the simplest solution which doesn't rework a lot of code >> underneath. Exceptions coming from a backing list/collection technically are >> handled by `VetoableListDecorator`'s try-catch clauses, however >> `VetoableListDecorator` does not provide an interface to react when such an >> exception happens - without it we cannot revert `childSet` back to its >> original state. > > Lukasz Kostyra has updated the pull request incrementally with two additional > commits since the last revision: > > - ParentTest: Add tests for NPE and *All calls > - ObservableListWrapper: Add from-to index check to remove(int, int) Marked as reviewed by jhendrikx (Committer). - PR Review: https://git.openjdk.org/jfx/pull/1136#pullrequestreview-1436772354
Re: RFR: 8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs [v8]
> Since surrogate pairs are internally considered as 2 characters and text > field is null in `HitInfo` when `getInsertionIndex` is invoked from > `TextFlow`, wrong insertion index was returned. > > Updated code to calculate insertion index in `getHitInfo` method of > `PrismTextLayout` class when `hitTest` of trailing side of surrogate pair is > requested. Since text runs are processed in this method already, calculating > the insertion index in this method looks better than calculating in > `getInsertionIndex` of `HitInfo` method. > The latter approach also requires the text to be sent to `HitInfo` as > parameter from the `hitTest` method of `TextFlow`. If the number of `Text` > nodes in `TextFlow` are very large, processing all the `Text` nodes on each > `hitTest` method invocation might cause performance issue. Hence implemented > first approach. > > Added system test to validate the fix. Karthik P K has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - Merge master to resolve merge conflict - Stabilizing the system tests - Add new test. Optimizations to make the tests robust - Change insertion index initialization approach. Add additional test - Initialize insertion index in PrismTextLayout - Address code review - Fix insertion index calculation issue with emojis. Add additional test cases - Fix insertion index calculation issue in TextFlow - Changes: https://git.openjdk.org/jfx/pull/1091/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1091&range=07 Stats: 408 lines in 3 files changed: 407 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1091.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1091/head:pull/1091 PR: https://git.openjdk.org/jfx/pull/1091
Re: RFR: 8301763: Adding children to wrong index leaves inconsistent state in Parent#childrenSet [v2]
On Wed, 17 May 2023 23:15:28 GMT, John Hendrikx wrote: > A roll back mechanism would just complicate the code for very little benefit > (a correct FX application won't run into these situations, you'll only be > likely to encounter them during development). The whole mechanism surrounding > the children list and bad additions should be seen as a best effort early > warning system (like ConcurrentModificationExeption). Understood, let's stick with current approach then. - PR Comment: https://git.openjdk.org/jfx/pull/1136#issuecomment-1557268177
Re: RFR: 8301763: Adding children to wrong index leaves inconsistent state in Parent#childrenSet [v3]
> This issue happened because `childSet` member of Parent was modified during > `onProposedChange()` call - that call did not recognize negative indexes as > invalid, which caused an exception when actually adding the Node to a List. > > This seemed like the simplest solution which doesn't rework a lot of code > underneath. Exceptions coming from a backing list/collection technically are > handled by `VetoableListDecorator`'s try-catch clauses, however > `VetoableListDecorator` does not provide an interface to react when such an > exception happens - without it we cannot revert `childSet` back to its > original state. Lukasz Kostyra has updated the pull request incrementally with two additional commits since the last revision: - ParentTest: Add tests for NPE and *All calls - ObservableListWrapper: Add from-to index check to remove(int, int) - Changes: - all: https://git.openjdk.org/jfx/pull/1136/files - new: https://git.openjdk.org/jfx/pull/1136/files/63023839..f1a8797d Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1136&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1136&range=01-02 Stats: 36 lines in 2 files changed: 36 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1136.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1136/head:pull/1136 PR: https://git.openjdk.org/jfx/pull/1136
Re: RFR: 8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs [v7]
> Since surrogate pairs are internally considered as 2 characters and text > field is null in `HitInfo` when `getInsertionIndex` is invoked from > `TextFlow`, wrong insertion index was returned. > > Updated code to calculate insertion index in `getHitInfo` method of > `PrismTextLayout` class when `hitTest` of trailing side of surrogate pair is > requested. Since text runs are processed in this method already, calculating > the insertion index in this method looks better than calculating in > `getInsertionIndex` of `HitInfo` method. > The latter approach also requires the text to be sent to `HitInfo` as > parameter from the `hitTest` method of `TextFlow`. If the number of `Text` > nodes in `TextFlow` are very large, processing all the `Text` nodes on each > `hitTest` method invocation might cause performance issue. Hence implemented > first approach. > > Added system test to validate the fix. Karthik P K has updated the pull request incrementally with one additional commit since the last revision: Stabilizing the system tests - Changes: - all: https://git.openjdk.org/jfx/pull/1091/files - new: https://git.openjdk.org/jfx/pull/1091/files/a43bbdb2..6684075e Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1091&range=06 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1091&range=05-06 Stats: 67 lines in 2 files changed: 33 ins; 26 del; 8 mod Patch: https://git.openjdk.org/jfx/pull/1091.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1091/head:pull/1091 PR: https://git.openjdk.org/jfx/pull/1091
Re: RFR: JDK-8306990: The guarantees given by Region's floor and ceiling functions should work for larger values
On Thu, 27 Apr 2023 11:41:40 GMT, Kevin Rushforth wrote: >> Region has floor and ceiling functions that ensure that calling them twice >> in a row will yield the same result: >> >> ceil(x) = ceil(ceil(x)) >> >> However, due to use of a constant `EPSILON` which is added/subtracted before >> doing the rounding, this only works for small numbers (in the range of 0-50 >> approximately). For larger values and scales, rounding errors can easily >> occur. This is visible as artifacts on screen where controls are a pixel >> wider than they should be. >> >> The use of the `EPSILON` constant is incorrect, as its value depends on the >> magnitude of the value in question (as magnitude increases, the fractional >> precision decreases). >> >> The Math class offers the function `ulp` that should be used here. It >> represents the smallest possible change in value for a given double. >> >> Extending the existing test case >> `snappingASnappedValueGivesTheSameValueTest` to use larger magnitude numbers >> exposes the problems. > > @andy-goryachev-oracle can you also review this? @kevinrushforth I think this one is ready to go, and should help to already solve some of the off-by-one pixel errors for controls that are larger than 100 pixels (in any direction) that make use of the ceiling or floor functions - PR Comment: https://git.openjdk.org/jfx/pull/1118#issuecomment-1556904744
