Re: RFR: 8284281: [Accessibility] [Win] [Narrator] Exceptions with TextArea & TextField when deleted last char [v2]
On Wed, 24 Aug 2022 14:21:44 GMT, Kevin Rushforth wrote: >> Ambarish Rapte has updated the pull request incrementally with one >> additional commit since the last revision: >> >> change the fix for TextArea Exception > > modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1990: > >> 1988: int offset = (Integer)parameters[0]; >> 1989: TextLine[] lines = getTextLayout().getLines(); >> 1990: if (offset > getTextInternal().length()) return >> lines.length; > > A couple questions: > > 1. This will return an offset that represents the position just past the end > of the list of lines. I presume this is OK? > 2. The `LINE_START` and `LINE_END` cases also can return null. Should a > similar fix be applied for those cases? When rechecked it looks wrong to change the return value/type. At several instances the return value `null` is used to assume that the control is `TextField`. 1. https://github.com/openjdk/jfx/blob/f95f09f5af561e3a6f3804a4f54aff83e9d2bb0b/modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinTextRangeProvider.java#L183 2. https://github.com/openjdk/jfx/blob/f95f09f5af561e3a6f3804a4f54aff83e9d2bb0b/modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinTextRangeProvider.java#L197 So changed to an alternative way of passing correct offset: If start(offset) is larger than length of text then pass length of text. - PR: https://git.openjdk.org/jfx/pull/884
Re: RFR: 8284281: [Accessibility] [Win] [Narrator] Exceptions with TextArea & TextField when deleted last char [v2]
> Issue: > When Narrator is running, > 1. Deleting last character from `TextField` throws > `IllegalArgumentException`, and > 2. Deleting last character from `TextArea` throws `NPE`. > > Fix: > When character is deleted, we receive an offset larger by one than the > current text length. This scenario needs to be handled correctly. > The change in `Text.java` fixes the NPE with TextArea, and, > The change in `WinTextRangeProvider.java` fixes the IllegalArgumentException > with TextField. > > To observe the issue. > 1. Run any program with TextField and/or TextArea > 3. Launch Windows Narrator > 4. Delete the last character from TextField / TextArea > 5. Observe the related Exception Ambarish Rapte has updated the pull request incrementally with one additional commit since the last revision: change the fix for TextArea Exception - Changes: - all: https://git.openjdk.org/jfx/pull/884/files - new: https://git.openjdk.org/jfx/pull/884/files/18635969..64375399 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=884&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=884&range=00-01 Stats: 3 lines in 2 files changed: 2 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/884.diff Fetch: git fetch https://git.openjdk.org/jfx pull/884/head:pull/884 PR: https://git.openjdk.org/jfx/pull/884
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v2]
On Fri, 26 Aug 2022 03:43:49 GMT, Nir Lisker wrote: > I haven't looked at the code yet, but in general it's not considered a good > idea to expose an object before it's instantiated. Not sure if we have a > choice here though. I agree, that's why the `underConstruction` flag is used to prevent calling `getRoot()` and `getChildren()`, which would leak a partially constructed instance to derived classes. - PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v2]
On Thu, 25 Aug 2022 22:18:44 GMT, Michael Strauß wrote: >> `Node` adds InvalidationListeners to its parent's `disabled` and >> `treeVisible` properties and calls its own `updateDisabled()` and >> `updateTreeVisible(boolean)` methods when the property values change. >> >> These listeners are not required, since `Node` can easily call the >> `updateDisabled()` and `updateTreeVisible(boolean)` methods on its children, >> saving the memory cost of maintaining listeners and bindings. > > Michael Strauß has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains five commits: > > - Replace null check by explicit underConstruction flag > - Merge branch 'master' into fixes/node-listener-cleanup > - Merge > - added tests > - Remove Node.disabled and Node.treeVisible listeners I haven't looked at the code yet, but in general it's not considered a good idea to expose an object before it's instantiated. Not sure if we have a choice here though. - PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v2]
On Thu, 25 Aug 2022 22:18:44 GMT, Michael Strauß wrote: >> `Node` adds InvalidationListeners to its parent's `disabled` and >> `treeVisible` properties and calls its own `updateDisabled()` and >> `updateTreeVisible(boolean)` methods when the property values change. >> >> These listeners are not required, since `Node` can easily call the >> `updateDisabled()` and `updateTreeVisible(boolean)` methods on its children, >> saving the memory cost of maintaining listeners and bindings. > > Michael Strauß has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains five commits: > > - Replace null check by explicit underConstruction flag > - Merge branch 'master' into fixes/node-listener-cleanup > - Merge > - added tests > - Remove Node.disabled and Node.treeVisible listeners It's true that `Parent.getChildren()` must not return `null`, and that we shouldn't add null checks to protect against bugs. However, in this specific instance, `Node.setTreeVisible` is called from the constructor of `Node`. At that point in time, the `Parent` constructor has not yet run, so its final fields are still unassigned (and therefore `null`). The same problem exists for `SubScene.getRoot()` a few lines earlier, which is specified to never return `null`, but will indeed return `null` if called when the `SubScene` is under construction. But there's a serious problem with the null checking approach: it's calling a protected method at an unexpected time, before the constructor of a derived class has run. Since that's not what derived classes would expect, I have instead opted for a different solution, which includes a `underConstruction` flag that is passed to `Node.setTreeVisible`. When the flag is set, the calls to `SubScene.getRoot()` and `Parent.getChildren()` are elided (they'd return `null` anyway, so there's no point in calling these methods). - PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v2]
> `Node` adds InvalidationListeners to its parent's `disabled` and > `treeVisible` properties and calls its own `updateDisabled()` and > `updateTreeVisible(boolean)` methods when the property values change. > > These listeners are not required, since `Node` can easily call the > `updateDisabled()` and `updateTreeVisible(boolean)` methods on its children, > saving the memory cost of maintaining listeners and bindings. Michael Strauß has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits: - Replace null check by explicit underConstruction flag - Merge branch 'master' into fixes/node-listener-cleanup - Merge - added tests - Remove Node.disabled and Node.treeVisible listeners - Changes: https://git.openjdk.org/jfx/pull/841/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=841&range=01 Stats: 81 lines in 4 files changed: 53 ins; 10 del; 18 mod Patch: https://git.openjdk.org/jfx/pull/841.diff Fetch: git fetch https://git.openjdk.org/jfx pull/841/head:pull/841 PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners
On Thu, 25 Aug 2022 13:50:19 GMT, Nir Lisker wrote: >> It's definitely a hypothetical case. `getChildren()` is called all over the >> place in JavaFX without a null check, so I see no reason for null checks >> here. > > Technically correct, although the [doc of > `getChildren()`](https://openjfx.io/javadoc/18/javafx.graphics/javafx/scene/Parent.html#getChildren()) > specifically says: > >> Note to subclasses: if you override this method, you must return from your >> implementation the result of calling this super method. The actual list >> instance returned from any getChildren() implementation must be the list >> owned and managed by this Parent. The only typical purpose for overriding >> this method is to promote the method to be public. > > So considering the case that an overriding method will return `null` is not > practical. > > As for a `null` child, I think that that's also not allowed. Considering that > a child can be added at most once to a scenegraph, it would mean that only 1 > `null` child is allowed. There might be more issues with `null` children. I > don't think there's a good reason to check for them. Yep, right. While it is possible to override `getChildren()` and return null, it will break everywhere so we can also omit the null check here. - PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners
On Thu, 25 Aug 2022 13:46:26 GMT, Kevin Rushforth wrote: >> Since `getChildren()` is not final, one can easily override it and return >> null. >> Therefore, this check should still be done here. >> Maybe we even need to check that every child is not null, since again I can >> override `getChildren()` and return a list that allows null. (The default >> implementation in `Parent` throws an exception when adding a null child) >> >> Interestingly, the javadoc of `getChildren()` states that one should call >> the super method when overriding (although this can't be enforced). So this >> is probably a hypothetical case. > > It's definitely a hypothetical case. `getChildren()` is called all over the > place in JavaFX without a null check, so I see no reason for null checks here. Technically correct, although the [doc of `getChildren()`](https://openjfx.io/javadoc/18/javafx.graphics/javafx/scene/Parent.html#getChildren()) specifically says: > Note to subclasses: if you override this method, you must return from your > implementation the result of calling this super method. The actual list > instance returned from any getChildren() implementation must be the list > owned and managed by this Parent. The only typical purpose for overriding > this method is to promote the method to be public. So considering the case that an overriding method will return `null` is not practical. As for a `null` child, I think that that's also not allowed. Considering that a child can be added at most once to a scenegraph, it would mean that only 1 `null` child is allowed. There might be more issues with `null` children. I don't think there's a good reason to check for them. - PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners
On Thu, 25 Aug 2022 13:36:01 GMT, Marius Hanl wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1919:
>>
>>> 1917: }
>>> 1918: }
>>> 1919: }
>>
>> Because we can use Java 17 now, you can use pattern matching for
>> `instanceof`. Also, from what I see, `getChildren()` can never return
>> `null`. So, we can write
>>
>> if (Node.this instanceof Parent parent) {
>> parent.getChildren().forEach(child -> child.updateDisabled());
>> }
>
> Since `getChildren()` is not final, one can easily override it and return
> null.
> Therefore, this check should still be done here.
> Maybe we even need to check that every child is not null, since again I can
> override `getChildren()` and return a list that allows null. (The default
> implementation in `Parent` throws an exception when adding a null child)
>
> Interestingly, the javadoc of `getChildren()` states that one should call the
> super method when overriding (although this can't be enforced). So this is
> probably a hypothetical case.
It's definitely a hypothetical case. `getChildren()` is called all over the
place in JavaFX without a null check, so I see no reason for null checks here.
-
PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners
On Thu, 25 Aug 2022 12:33:51 GMT, Nir Lisker wrote:
>> `Node` adds InvalidationListeners to its parent's `disabled` and
>> `treeVisible` properties and calls its own `updateDisabled()` and
>> `updateTreeVisible(boolean)` methods when the property values change.
>>
>> These listeners are not required, since `Node` can easily call the
>> `updateDisabled()` and `updateTreeVisible(boolean)` methods on its children,
>> saving the memory cost of maintaining listeners and bindings.
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1919:
>
>> 1917: }
>> 1918: }
>> 1919: }
>
> Because we can use Java 17 now, you can use pattern matching for
> `instanceof`. Also, from what I see, `getChildren()` can never return `null`.
> So, we can write
>
> if (Node.this instanceof Parent parent) {
> parent.getChildren().forEach(child -> child.updateDisabled());
> }
Since `getChildren()` is not final, one can easily override it and return null.
Therefore, this check should still be done here.
Maybe we even need to check that every child is not null, since again I can
override `getChildren()` and return a list that allows null. (The default
implementation in `Parent` throws an exception when adding a null child)
-
PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners
On Thu, 21 Jul 2022 04:43:15 GMT, Michael Strauß wrote:
> `Node` adds InvalidationListeners to its parent's `disabled` and
> `treeVisible` properties and calls its own `updateDisabled()` and
> `updateTreeVisible(boolean)` methods when the property values change.
>
> These listeners are not required, since `Node` can easily call the
> `updateDisabled()` and `updateTreeVisible(boolean)` methods on its children,
> saving the memory cost of maintaining listeners and bindings.
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1919:
> 1917: }
> 1918: }
> 1919: }
Because we can use Java 17 now, you can use pattern matching for `instanceof`.
Also, from what I see, `getChildren()` can never return `null`. So, we can write
if (Node.this instanceof Parent parent) {
parent.getChildren().forEach(child -> child.updateDisabled());
}
-
PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners
On Thu, 21 Jul 2022 04:43:15 GMT, Michael Strauß wrote: > `Node` adds InvalidationListeners to its parent's `disabled` and > `treeVisible` properties and calls its own `updateDisabled()` and > `updateTreeVisible(boolean)` methods when the property values change. > > These listeners are not required, since `Node` can easily call the > `updateDisabled()` and `updateTreeVisible(boolean)` methods on its children, > saving the memory cost of maintaining listeners and bindings. Tested with some layout and controls. LGTM. - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.org/jfx/pull/841
