Re: RFR: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed [v4]
On Wed, 30 Mar 2022 13:27:40 GMT, Johan Vos wrote:
>> When the size of a ListCell is changed and a scrollTo method is invoked
>> without having a layout calculation in between, the old (wrong) size is used
>> to calculcate the total estimate. This happens e.g. when the size is changed
>> in the `updateItem` method.
>> This PR will immediately resize the cell and sets the new value in the cache
>> containing the cellsizes.
>
> Johan Vos has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Don't shift cells if we are already showing the lowest index cell.
I don't believe that it is possible to position the content and scrollbar
**exactly**, if the VirtualFlow **estimates** the size of the cells.
For that case we need an external cell size provider, that I, as an application
developer, can provide to the e.g. ListView.
I thought of something like this. The existing estimation logic can then be
implemented as a default cell size provider.
interface CellSizeProvider {
/**
* Returns the size of all cells.
* IMPORTANT: Be carefully by implementing this method. It can drastically
degrade the performance of a component.
*/
double getTotalSize();
/**
* Returns the size of the cell for an item given by its index.
* IMPORTANT: Be carefully by implementing this method. It can drastically
degrade the performance of a component.
*/
double getCellSize(int);
/**
* The callback which is set by the VirtualFlow. It allows to calculate the
cell size for an item given by its index.
* IMPORTANT: This callback returns the exact value, but it may be slow.
*/
void setCellSizeCalculator(Function cellSizeCalculator);
}
-
PR: https://git.openjdk.java.net/jfx/pull/712
Integrated: 8281953: NullPointer in InputMethod components in JFXPanel
On Thu, 17 Feb 2022 12:57:27 GMT, eduardsdv wrote: > If the InputMethod's node is not in the scene, the default text location > point is returned. This pull request has now been integrated. Changeset: a0bb545b Author:Eduard Sedov Committer: Ajit Ghaisas URL: https://git.openjdk.java.net/jfx/commit/a0bb545b21d1139dd95d7ee9dea9298e89666d9b Stats: 17 lines in 2 files changed: 16 ins; 0 del; 1 mod 8281953: NullPointer in InputMethod components in JFXPanel Reviewed-by: aghaisas - PR: https://git.openjdk.java.net/jfx/pull/735
Re: RFR: 8281953: NullPointer in InputMethod components in JFXPanel [v2]
On Mon, 21 Feb 2022 11:30:17 GMT, Ajit Ghaisas wrote: >> eduardsdv has updated the pull request incrementally with one additional >> commit since the last revision: >> >>8281953: Format TextInputControlSkinTest > > modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TextInputControlSkinTest.java > line 105: > >> 103: // and that the default point is returned. >> 104: Point2D point = >> textField.getInputMethodRequests().getTextLocation(0); >> 105: assertEquals(new Point2D(0,0), point); > > Very minor : Please add a space between `0,0` I added the space and checked the fix again with the sample program. The fix works, the NullPointer does not occur anymore. - PR: https://git.openjdk.java.net/jfx/pull/735
Re: RFR: 8281953: NullPointer in InputMethod components in JFXPanel [v2]
> If the InputMethod's node is not in the scene, the default text location > point is returned. eduardsdv has updated the pull request incrementally with one additional commit since the last revision: 8281953: Format TextInputControlSkinTest - Changes: - all: https://git.openjdk.java.net/jfx/pull/735/files - new: https://git.openjdk.java.net/jfx/pull/735/files/c9c66b47..755444da Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=735=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=735=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/735.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/735/head:pull/735 PR: https://git.openjdk.java.net/jfx/pull/735
Re: RFR: 8281953: NullPointer in InputMethod components in JFXPanel
On Sat, 19 Feb 2022 17:40:54 GMT, delvh wrote:
>> If the InputMethod's node is not in the scene, the default text location
>> point is returned.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextInputControlSkin.java
> line 340:
>
>> 338: if (window == null) {
>> 339: return new Point2D(0, 0);
>> 340: }
>
> As long as each scene needs to be located in a window, the following will
> simplify the code:
> Suggestion:
>
> if (scene == null) {
> return new Point2D(0, 0);
> }
> Window window = scene.getWindow();
>
> Is it possible to have a scene without an assigned window?
Since the NullPointer occurs here because the synchronization between JavaFx
and AWT threads is not really possible in this case, we cannot ensure that this
method is only called when the scene is assigned to a window.
I would check here for null both the scene and the window.
-
PR: https://git.openjdk.java.net/jfx/pull/735
RFR: 8281953: NullPointer in InputMethod components in JFXPanel
If the InputMethod's node is not in the scene, the default text location point is returned. - Commit messages: - 8281953: NullPointer in InputMethod components in JFXPanel Changes: https://git.openjdk.java.net/jfx/pull/735/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=735=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281953 Stats: 17 lines in 2 files changed: 16 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/735.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/735/head:pull/735 PR: https://git.openjdk.java.net/jfx/pull/735
Integrated: 8244234: MenuButton: NPE on removing from scene with open popup
On Tue, 11 Jan 2022 14:42:19 GMT, eduardsdv wrote: > The NPE occurs when the skinnable is removed from the scene while the popup > is showing. > The MenuButtonSkinBase, when popup becomes hidden, tries to remove Mnemonics > from the scene and runs into NPE. > To avoid NPE a null-check is added to the 'showing' listener. > > Since the mnemonics cannot be removed from the scene in standard way, when > popup becomes hidden. > They are now also removed from the scene, when the skinnable is removed from > it. This pull request has now been integrated. Changeset: 6e215263 Author:Eduard Sedov Committer: Ajit Ghaisas URL: https://git.openjdk.java.net/jfx/commit/6e215263e6fe94099528d386ccda65b22bce3138 Stats: 131 lines in 2 files changed: 126 ins; 1 del; 4 mod 8244234: MenuButton: NPE on removing from scene with open popup Reviewed-by: aghaisas - PR: https://git.openjdk.java.net/jfx/pull/713
Re: RFR: 8244234: MenuButton: NPE on removing from scene with open popup [v2]
> The NPE occurs when the skinnable is removed from the scene while the popup > is showing. > The MenuButtonSkinBase, when popup becomes hidden, tries to remove Mnemonics > from the scene and runs into NPE. > To avoid NPE a null-check is added to the 'showing' listener. > > Since the mnemonics cannot be removed from the scene in standard way, when > popup becomes hidden. > They are now also removed from the scene, when the skinnable is removed from > it. eduardsdv has updated the pull request incrementally with one additional commit since the last revision: 8244234: Add changes suggested in review and add file header to the test - Changes: - all: https://git.openjdk.java.net/jfx/pull/713/files - new: https://git.openjdk.java.net/jfx/pull/713/files/0c598695..6749816f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=713=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=713=00-01 Stats: 36 lines in 2 files changed: 25 ins; 0 del; 11 mod Patch: https://git.openjdk.java.net/jfx/pull/713.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/713/head:pull/713 PR: https://git.openjdk.java.net/jfx/pull/713
Re: RFR: 8244234: MenuButton: NPE on removing from scene with open popup
On Tue, 11 Jan 2022 14:42:19 GMT, eduardsdv wrote: > The NPE occurs when the skinnable is removed from the scene while the popup > is showing. > The MenuButtonSkinBase, when popup becomes hidden, tries to remove Mnemonics > from the scene and runs into NPE. > To avoid NPE a null-check is added to the 'showing' listener. > > Since the mnemonics cannot be removed from the scene in standard way, when > popup becomes hidden. > They are now also removed from the scene, when the skinnable is removed from > it. I added changes suggested in the review and added the file header to the test class. Please review. - PR: https://git.openjdk.java.net/jfx/pull/713
RFR: 8244234: MenuButton: NPE on removing from scene with open popup
The NPE occurs when the skinnable is removed from the scene while the popup is showing. The MenuButtonSkinBase, when popup becomes hidden, tries to remove Mnemonics from the scene and runs into NPE. To avoid NPE a null-check is added to the 'showing' listener. Since the mnemonics cannot be removed from the scene in standard way, when popup becomes hidden. They are now also removed from the scene, when the skinnable is removed from it. - Commit messages: - 8244234: MenuButton: NPE on removing from scene with open popup Changes: https://git.openjdk.java.net/jfx/pull/713/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=713=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8244234 Stats: 116 lines in 2 files changed: 101 ins; 1 del; 14 mod Patch: https://git.openjdk.java.net/jfx/pull/713.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/713/head:pull/713 PR: https://git.openjdk.java.net/jfx/pull/713
Integrated: 8203463: [Accessibility, Narrator] NPE in TableView
On Wed, 22 Dec 2021 08:59:53 GMT, eduardsdv wrote: > The NullPointer occurs when `Accessible.getAttribute(Object)` returns null. > I checked all places where the `getAttribute(..)` method is called. > Everywhere it is checked for null `(Accessible,` `MacAccessible,` > `WinAccessible,` `WinTextRangeProvider`). Only at this one place in > `WinAccessible.GetPatternProvider(int)` it is missing. > > This PR adds this missing null-check in the > `WinAccessible.GetPatternProvider(int)` method. This pull request has now been integrated. Changeset: c705bd49 Author:eduardsdv <53449139+eduard...@users.noreply.github.com> Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/c705bd493931d88650542be5466d6add359f45b9 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8203463: [Accessibility, Narrator] NPE in TableView Reviewed-by: kcr - PR: https://git.openjdk.java.net/jfx/pull/700
RFR: 8203463: [Accessibility, Narrator] NPE in TableView
The NullPointer occurs when `Accessible.getAttribute(Object)` returns null. I checked all places where the `getAttribute(..)` method is called. Everywhere it is checked for null `(Accessible,` `MacAccessible,` `WinAccessible,` `WinTextRangeProvider`). Only at this one place in `WinAccessible.GetPatternProvider(int)` it is missing. This PR adds this missing null-check in the `WinAccessible.GetPatternProvider(int)` method. - Commit messages: - Merge branch 'openjdk:master' into bugfix/8203463-NPE-in-WinAccessible-GetPatternProvider - WinAccessible: Add missing null check Changes: https://git.openjdk.java.net/jfx/pull/700/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=700=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8203463 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/700.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/700/head:pull/700 PR: https://git.openjdk.java.net/jfx/pull/700
Integrated: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element
On Fri, 29 Oct 2021 12:19:40 GMT, eduardsdv wrote: > Fix VirtualFlow.scrollToTop(int) doesn't scroll to the top of the last > element but to the bottom of the last element. This pull request has now been integrated. Changeset: 11583392 Author:Eduard Sedov Committer: Johan Vos URL: https://git.openjdk.java.net/jfx/commit/115833923a644be8328b8f7a7a6845e5b42a57b3 Stats: 42 lines in 4 files changed: 37 ins; 0 del; 5 mod 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element Reviewed-by: aghaisas, jvos - PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
On Mon, 6 Dec 2021 14:58:52 GMT, eduardsdv wrote: >> Fix VirtualFlow.scrollToTop(int) doesn't scroll to the top of the last >> element but to the bottom of the last element. > > eduardsdv has updated the pull request incrementally with one additional > commit since the last revision: > > 8276170: Add junit for VirtualFlow.scrollToTop(int) Please review. - PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v4]
> Fix VirtualFlow.scrollToTop(int) doesn't scroll to the top of the last > element but to the bottom of the last element. eduardsdv has updated the pull request incrementally with one additional commit since the last revision: 8276167: Avoid additional Toolkit.getToolkit().firePulse() Avoid using of additional Toolkit.getToolkit().firePulse() in the test, adjust expected values instead. - Changes: - all: https://git.openjdk.java.net/jfx/pull/656/files - new: https://git.openjdk.java.net/jfx/pull/656/files/588a7e41..73e11f4b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=656=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=656=02-03 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/656.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/656/head:pull/656 PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
On Fri, 10 Dec 2021 15:00:56 GMT, Johan Vos wrote: >> Without this line the test fails in the line 1865 with the message >> 'expected:<7> but was:<8>'. >> I think this is because in the line 1850 'listView.scrollTo(99)' is >> executed, which now does not set the position to 1. >> >> It can also be caused by the >> https://bugs.openjdk.java.net/browse/JDK-8277785. When it is solved, this >> call will probably no longer be necessary. > > That is very well possible indeed. Since we estimate the size of the > different cells, it is hard to deterministically put values on the counts. > Therefor, I changed a number of tests from `equals` to `<` . I would > personally suggest not adding the `pulse` here, but rather change the test in > line 1865 with `... < 10 ` to make sure that there is no abnormal amount of > processing. (and maybe also `> 0 ` to make sure that the updateItem is at > least counted once). I removed the Toolkit.getToolkit().firePulse() and adjusted the expected values in the test. - PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
On Fri, 10 Dec 2021 12:33:29 GMT, Johan Vos wrote: >> eduardsdv has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8276170: Add junit for VirtualFlow.scrollToTop(int) > > modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewKeyInputTest.java > line 1844: > >> 1842: } >> 1843: listView.setPrefHeight(130); // roughly room for four rows >> 1844: Toolkit.getToolkit().firePulse(); > > Why is this needed here? Without this line the test fails in the line 1865 with the message 'expected:<7> but was:<8>'. I think this is because in the line 1850 'listView.scrollTo(99)' is executed, which now does not set the position to 1. - PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
On Fri, 10 Dec 2021 12:33:19 GMT, Johan Vos wrote:
>> eduardsdv has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8276170: Add junit for VirtualFlow.scrollToTop(int)
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
> line 1584:
>
>> 1582: boolean posSet = false;
>> 1583:
>> 1584: if (index > getCellCount() - 1) {
>
> I agree that the previous code violates the contract that the flow should
> show the specified cell aligned to the top.
>
> I wonder though if this test is needed at all. What is the goal of providing
> an index that is larger than getCellCount() -1?
This part was originally copied from the 'VirtualContainerBase'. There was this
comment: _special-case the 'greater than row count' condition to have it be
perfectly at position 1_.
Maybe to avoid throwing the IndexOutOfBoundsException or simply to scroll to
the end.
-
PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
On Mon, 6 Dec 2021 14:58:52 GMT, eduardsdv wrote: >> Fix VirtualFlow.scrollToTop(int) doesn't scroll to the top of the last >> element but to the bottom of the last element. > > eduardsdv has updated the pull request incrementally with one additional > commit since the last revision: > > 8276170: Add junit for VirtualFlow.scrollToTop(int) I added a junit which fails without patch and succeeds with it. - PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
> Fix VirtualFlow.scrollToTop(int) doesn't scroll to the top of the last > element but to the bottom of the last element. eduardsdv has updated the pull request incrementally with one additional commit since the last revision: 8276170: Add junit for VirtualFlow.scrollToTop(int) - Changes: - all: https://git.openjdk.java.net/jfx/pull/656/files - new: https://git.openjdk.java.net/jfx/pull/656/files/a7820448..588a7e41 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=656=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=656=01-02 Stats: 37 lines in 1 file changed: 37 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/656.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/656/head:pull/656 PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v2]
On Mon, 6 Dec 2021 08:22:37 GMT, eduardsdv wrote: >> Fix VirtualFlow.scrollToTop(int) doesn't scroll to the top of the last >> element but to the bottom of the last element. > > eduardsdv has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains two additional commits since > the last revision: > > - Merge branch 'openjdk:master' into > bugfix/JDK-8276167-VirtualFlow-scrollToTop > - JDK-8276167: fixing VirtualFlow.scrollToTop(int) I tested it again. Without the changes in this PR the bug is still there. If the item is larger than the viewport, the VirtualFlow.scrollToTop(int) scrolls to the end instead of to top of the item. - PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v2]
> Fix VirtualFlow.scrollToTop(int) doesn't scroll to the top of the last > element but to the bottom of the last element. eduardsdv has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge branch 'openjdk:master' into bugfix/JDK-8276167-VirtualFlow-scrollToTop - JDK-8276167: fixing VirtualFlow.scrollToTop(int) - Changes: - all: https://git.openjdk.java.net/jfx/pull/656/files - new: https://git.openjdk.java.net/jfx/pull/656/files/087e11d3..a7820448 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=656=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=656=00-01 Stats: 4192 lines in 92 files changed: 3532 ins; 274 del; 386 mod Patch: https://git.openjdk.java.net/jfx/pull/656.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/656/head:pull/656 PR: https://git.openjdk.java.net/jfx/pull/656
RFR: 8276167: fixing VirtualFlow.scrollToTop(int)
Fix VirtualFlow.scrollToTop(int) doesn't scroll to the top of the last element but to the bottom of the last element. - Commit messages: - JDK-8276167: fixing VirtualFlow.scrollToTop(int) Changes: https://git.openjdk.java.net/jfx/pull/656/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=656=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276167 Stats: 3 lines in 3 files changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/656.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/656/head:pull/656 PR: https://git.openjdk.java.net/jfx/pull/656
RFR: 8276170: Create Sources when publishing to Maven
Create sources.jars and attach they to the publish task, so that they can be uploaded to the (e.g. maven) repository automatically. - Commit messages: - 8276170: Remove trailing whitespaces - JDK-8276170: Create Sources when publishing to Maven Changes: https://git.openjdk.java.net/jfx/pull/657/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=657=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276170 Stats: 13 lines in 1 file changed: 13 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/657.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/657/head:pull/657 PR: https://git.openjdk.java.net/jfx/pull/657
