Re: RFR: 8277756: DatePicker listener might not be added when using second constructor
On Tue, 24 May 2022 21:35:15 GMT, Marius Hanl wrote: > The `valueProperty()` and `chronologyProperty()` listener are now added in > the second constructor of `DatePicker` > (`public DatePicker(LocalDate localDate)`) instead of the first one (`public > DatePicker()`). > Therefore, both listener are now added no matter which constructor is used. Looks good to me. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/801
Re: RFR: 8284665: First selected item of a TreeItem multiple selection gets removed if new items are constantly added to the TreeTableView [v2]
On Tue, 24 May 2022 09:51:29 GMT, Jose Pereda wrote: >> This PR fixes an issue with selection of multiple items in TableView and >> TreeTableView controls that gets moved unexpectedly when new items are added >> even way below the selected items. >> >> A couple of tests have been added. They fail without this PR (first >> selected item gets deselected, and table anchor gets moved), and pass with >> this PR (first selected item keeps selected, and table anchor remains at the >> same place). > > Jose Pereda has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Merge branch 'master' into 8284665-tableviewanchor > - Only shift anchor if existing one is affected by the change event, and > tests Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/790
Re: RFR: 8286261: Selection of non-expanded non-leaf treeItem grows unexpectedly when adding two-level descendants
On Fri, 6 May 2022 10:16:41 GMT, Jose Pereda wrote: > This PR extends the check if a treeItem is expanded to all its ancestors, as > in case one ancestor is collapsed, all its children will be hidden. > > 4 tests are included, two for TreeView and two for TreeTableView. This looks good to me. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/791
Re: RFR: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView) [v2]
On Thu, 19 May 2022 14:23:55 GMT, Robert Lichtenberger wrote: >> Separate test class added for TreeTableView case. >> Fix is analogous to JDK-8251480. > > Robert Lichtenberger has updated the pull request incrementally with two > additional commits since the last revision: > > - 8285197: TableColumnHeader: calc of cell width must respect row styling > (TreeTableView) > >Test class cosmetic cleanups #2. > - 8285197: TableColumnHeader: calc of cell width must respect row styling > (TreeTableView) > >Test class cosmetic cleanups. Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/779
Re: RFR: 8284665: First selected item of a TreeItem multiple selection gets removed if new items are constantly added to the TreeTableView
On Thu, 5 May 2022 16:21:45 GMT, Jose Pereda wrote: > This PR fixes an issue with selection of multiple items in TableView and > TreeTableView controls that gets moved unexpectedly when new items are added > even way below the selected items. > > A couple of tests have been added. They fail without this PR (first selected > item gets deselected, and table anchor gets moved), and pass with this PR > (first selected item keeps selected, and table anchor remains at the same > place). The fix looks good. I verified that tests fail without this fix and pass with it. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/790
Re: RFR: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView)
On Thu, 21 Apr 2022 08:38:20 GMT, Robert Lichtenberger wrote: > Separate test class added for TreeTableView case. > Fix is analogous to JDK-8251480. The fix looks good. The newly introduced test file needs some cosmetic cleanups. modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableColumnHeaderTest.java line 2: > 1: /* > 2: * Copyright (c) 2018, 2021, Oracle and/or its affiliates. All rights > reserved. Replace "2018, 2021" with just "2022" as this is newly introduced test file. modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableColumnHeaderTest.java line 33: > 31: import javafx.event.Event; > 32: import javafx.scene.Node; > 33: import javafx.scene.control.*; Replace generic import statement with specific ones. modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableColumnHeaderTest.java line 51: > 49: import java.util.List; > 50: > 51: import static org.junit.Assert.*; Replace generic import statement with specific ones. - Changes requested by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/779
Integrated: 8285360: [TestBug] Cleanup a few ignored javafx.controls unit tests
On Thu, 21 Apr 2022 11:23:35 GMT, Ajit Ghaisas wrote: > This PR is to cleanup a few `javafx.controls` unit tests that were ignored. > > Here is the list of targeted unit test classes- > - Ignored tests re-enabled and fixed - `DateCellTest`, `CellTest`, > `PaginationTest` > - Ignored tests removed - `RadioMenuItemTest`, `PopupControlTest` > > Results of `javafx.controls` unit tests- > **Before this PR :** > Total tests - 8610 > Failures - 0 > Ignored - 246 > > **After this PR :** > Total tests - 8608 > Failures - 0 > Ignored - 235 This pull request has now been integrated. Changeset: 318204b4 Author:Ajit Ghaisas URL: https://git.openjdk.java.net/jfx/commit/318204b47530e532abf8738693e6eefa2de00543 Stats: 58 lines in 5 files changed: 33 ins; 24 del; 1 mod 8285360: [TestBug] Cleanup a few ignored javafx.controls unit tests Reviewed-by: arapte - PR: https://git.openjdk.java.net/jfx/pull/780
Re: RFR: 8193442: Removing TreeItem from a TreeTableView sometime changes selectedItem [v3]
On Thu, 21 Apr 2022 08:37:11 GMT, Jose Pereda wrote: >> This PR fixes >> JDK-[8193442](https://bugs.openjdk.java.net/browse/JDK-8193442), but also >> [JDK-8187596](https://bugs.openjdk.java.net/browse/JDK-8187596), and >> verifies that the tests mentioned in >> [JDK-8088157](https://bugs.openjdk.java.net/browse/JDK-8088157) are working >> (with a minor fix). >> >> When removing an item that is below the selected item from TreeTableView or >> TreeView controls the selection and/or focus was wrongly changed in some >> occasions, because a shift in the selection was applied. >> >> This PR adds a method to ControlUtils to get the index of the sibling that >> is selected/focused or contains the descendant item with the current >> selection/focus. >> >> This index is required to compare properly if the selected/focus item is >> above or below the item that was removed, by comparing the indices of >> siblings. >> >> Tests have been added to TreeViewTest and TreeTableViewTest based on the >> existing tests on JDK-8193442 and JDK-8187596. The four tests fail without >> this PR, pass with it. >> >> In the process, I noticed that the ignored tests referred from JDK-8088157 >> were already passing, after removing some obsolete asserts, even without >> this PR. > > Jose Pereda has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains four commits: > > - Fix spacing in tests > - Merge branch 'master' into 8193442-treeitemselection > - Merge branch 'master' into 8193442-treeitemselection > - Don't shift selection/focus if item is below removed element Looks good to me. Apart from the bug id in title, you can use `/issue` command to list additional issues this PR fixes. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/753
Re: RFR: 8285360: [TestBug] Cleanup a few ignored javafx.controls unit tests [v2]
On Fri, 22 Apr 2022 06:27:49 GMT, Ambarish Rapte wrote: >> Ajit Ghaisas has updated the pull request incrementally with one additional >> commit since the last revision: >> >> address review comments > > modules/javafx.controls/src/test/java/test/javafx/scene/control/CellTest.java > line 387: > >> 385: cell.requestFocus(); >> 386: Toolkit.getToolkit().firePulse(); >> 387: > > Minor: Optional: > Adding `assertTrue(cell.isEditing());` here can be a good sanity check. But I > leave it to you. > Similar comment for the change in `DateCellTest.loseFocusWhileEditing()` Done. > modules/javafx.controls/src/test/java/test/javafx/scene/control/CellTest.java > line 392: > >> 390: >> 391: assertFalse(cell.isEditing()); >> 392: } > > I would recommend to call `stage.hide()` similar to that in the `@AfterClass > cleanup` methods. > Similar comment for the change in `DateCellTest.loseFocusWhileEditing()` I have added `stage.hide()` to these tests, also to the `PaginationTest` class. - PR: https://git.openjdk.java.net/jfx/pull/780
Re: RFR: 8285360: [TestBug] Cleanup a few ignored javafx.controls unit tests [v2]
> This PR is to cleanup a few `javafx.controls` unit tests that were ignored. > > Here is the list of targeted unit test classes- > - Ignored tests re-enabled and fixed - `DateCellTest`, `CellTest`, > `PaginationTest` > - Ignored tests removed - `RadioMenuItemTest`, `PopupControlTest` > > Results of `javafx.controls` unit tests- > **Before this PR :** > Total tests - 8610 > Failures - 0 > Ignored - 246 > > **After this PR :** > Total tests - 8608 > Failures - 0 > Ignored - 235 Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: address review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/780/files - new: https://git.openjdk.java.net/jfx/pull/780/files/679c54e2..2d8366fc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=780&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=780&range=00-01 Stats: 9 lines in 3 files changed: 9 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/780.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/780/head:pull/780 PR: https://git.openjdk.java.net/jfx/pull/780
RFR: 8285360: [TestBug] Cleanup a few ignored javafx.controls unit tests
This PR is to cleanup a few `javafx.controls` unit tests that were ignored. Here is the list of targeted unit test classes- - Ignored tests re-enabled and fixed - `DateCellTest`, `CellTest`, `PaginationTest` - Ignored tests removed - `RadioMenuItemTest`, `PopupControlTest` Results of `javafx.controls` unit tests- **Before this PR :** Total tests - 8610 Failures - 0 Ignored - 246 **After this PR :** Total tests - 8608 Failures - 0 Ignored - 235 - Commit messages: - fix or remove ignored unit tests Changes: https://git.openjdk.java.net/jfx/pull/780/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=780&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285360 Stats: 49 lines in 5 files changed: 24 ins; 24 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/780.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/780/head:pull/780 PR: https://git.openjdk.java.net/jfx/pull/780
Re: RFR: 8273339: IOOBE with ListChangeListener added to the selectedItems list of a TableView [v3]
On Thu, 10 Feb 2022 10:16:36 GMT, Jose Pereda wrote: >> This PR converts the change's `from` field from a list of tablePositions >> into a list of selected indices of rows. >> >> It includes two tests for TableView and one for TreeTableView (the second >> test wasn't included due to an >> [issue|https://bugs.openjdk.java.net/browse/JDK-8232825] with key >> navigation), that pass with this PR and fail without it. > > Jose Pereda has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains three commits: > > - Merge branch 'master' into 8273339-changefrom > - Apply change only when cell selection is enabled > - Convert change from list of tablePosition to list of rows Looks good to me. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/710
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v4]
On Wed, 30 Mar 2022 12:22:23 GMT, Robert Lichtenberger wrote: >> This fix respects a row factory, if present. >> It will put the cell that is used to measure the column width as child below >> the row. >> In that way the row's style will be used. > > Robert Lichtenberger has updated the pull request incrementally with one > additional commit since the last revision: > > 8251480: TableColumnHeader: calc of cell width must respect row styling > > Assertion removed. Looks good to me. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8283509: Invisible menus can lead to IndexOutOfBoundsException [v2]
On Wed, 30 Mar 2022 10:57:48 GMT, Robert Lichtenberger wrote: >> findSibling adapted to only use visible menus when calculating the >> index. > > Robert Lichtenberger has updated the pull request incrementally with one > additional commit since the last revision: > > 8251480: TableColumnHeader: calc of cell width must respect row styling > > Comment improved. Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/759
Re: RFR: 8283509: Invisible menus can lead to IndexOutOfBoundsException
On Tue, 29 Mar 2022 12:11:54 GMT, Robert Lichtenberger wrote: > > Have you considered keeping the same while loop in findSibling() method and > > skipping invisible Menus in it? This is to avoid creating and traversing a > > new list. > > This would not give the correct result. findSibling must return the index in > the list of **visible** menus, since this is what setFocusedMenuIndex() > really expects. If we only skip invisible menus we will possibly return an > index that is out of bounds, which is exactly what this bug is about. OK. Thanks for the explanation. The fix and the test looks good. Only one minor correction needed is in comment. `// Traverse all menus in menubar to find nextIndex` should be `// Traverse all visible menus in menubar to find nextIndex` - PR: https://git.openjdk.java.net/jfx/pull/759
Re: RFR: 8283509: Invisible menus can lead to IndexOutOfBoundsException
On Tue, 22 Mar 2022 14:42:07 GMT, Robert Lichtenberger wrote: > findSibling adapted to only use visible menus when calculating the > index. Have you considered keeping the same while loop in findSibling() method and skipping invisible Menus in it? This is to avoid creating and traversing a new list. - PR: https://git.openjdk.java.net/jfx/pull/759
Re: RFR: 8281723: Spinner with split horizontal arrows and a border places right arrow incorrectly [v2]
On Wed, 9 Mar 2022 07:48:53 GMT, John Hendrikx wrote: >> I added a test case for `SpinnerSkin` that checks the arrow positioning. >> >> While adding the tests I discovered more problems with the positioning aside >> from the one mentioned in the JBS ticket. >> >> 1) Vertical split arrow placement also forgot to take the padding into >> account while placing the decrement arrow button -- I've taken the liberty >> to fix that problem as well in the same PR. >> >> 2) When arrows are placed next to each other either on the right or left, >> the arrow widths are not normalized to be the width of the widest arrow. >> All other placements will normalize either the width or height, except for >> these two. Specifically, when the arrows are **split** on the left and >> right they **are** normalized to the same width. >> >> For point 2, here is the problem illustrated with actual widths on left and >> layout result on right: >> >> [ <- ] [ -> ] [ spinner ] --> [ <- ] [ -> ] [ >> spinner ] >> [ spinner ] [ <- ] [ -> ] --> [ spinner ] [ <- ] >> [ -> ] >> >> While for split horizontal it does normalize the width to that of the widest >> arrow, and so layout becomes: >> >> [ <- ] [ spinner ] [ -> ] --> [ <- ] [ spinner ] >> [ -> ] >> >> While I'm here I could fix this as well, and adjust the test case to match. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright year The fix looks good to me. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/748
Re: RFR: 8282093: LineChart path incorrect when outside lower bound [v2]
On Tue, 1 Mar 2022 06:12:59 GMT, Abhinay Agarwal wrote: >> This regression was caused in PR #667 in which I didn't take into account >> the lower bounds. I have added more tests and one manual test along with the >> fix. The manual test can be used to identify any future issues with paths >> outside lower and upper bounds easily. > > Abhinay Agarwal has updated the pull request incrementally with one > additional commit since the last revision: > > Improve layout in manual test Thanks for fixing this regression. Changes look good. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/744
Re: RFR: 8282100: Missed top/left bouncing for ScrollPane on Raspberry Pi with Touchscreen
On Fri, 18 Feb 2022 15:21:49 GMT, Alexander Scherbatiy wrote: > There is the bouncing when scrolling a node on a ScrollPane to the > right/bottom (the node on the scroll pane is scrolled further than its > width/height so the background is visible and then automatically is scrolled > back to the node bounds) on Raspberry Pi with Touchscreen. > There is no bouncing when node is scrolled to the left/top. > > The fix updates ScrollPaneSkin.updatePosX()/updatePosY() methods to not > discard out of the range top/left minX/Y values in case the touch is > supported. The change looks correct. I verified that it does not have any side effect on non-touch devices (it is obvious from the code changes as well). I did not test on the platform this PR is intended for. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/736
Re: RFR: 8279228 Leak in ScrollPaneSkin, related to touch events
On Thu, 23 Dec 2021 17:43:19 GMT, Florian Kirmaier wrote: > Fixing memoryleak, related to touch events in ScrollPaneWhen touchDetected or > mouseDown is true, the sbTouch animation is running, > and the node is removed from the Scene, then the animation will never stop, > causing a memory leak. > A simple fix is to also check, whether the Node is visible, by checking the > "isTreeShowing" property. This simple fix looks good to me. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/701
Re: RFR: 8281953: NullPointer in InputMethod components in JFXPanel [v2]
On Mon, 21 Feb 2022 12:35:34 GMT, eduardsdv wrote: >> 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 Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/735
Re: RFR: 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. The fix looks good. I thought about returning `null` if either the scene or the window is null. As we need to fix this corner case, whether returning `null` or the default point (as proposed in this PR) does not matter. Both approaches will work. - Have you verified the sample program attached to the JBS runs successfully with your fix? - I have a very minor formatting comment on the test. 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` - PR: https://git.openjdk.java.net/jfx/pull/735
[jfx18] Integrated: 8271085: TabPane: Redundant API docs
On Mon, 7 Feb 2022 10:53:00 GMT, Ajit Ghaisas wrote: > This is a Javadoc cleanup and correction fix for the TabPane as described in > the JBS. > > Changes done for all the Properties of the TabPane - > - Moved the property description to be over the property field. > - Removed the unnecessary docs on property setter/getter and Property method. This pull request has now been integrated. Changeset: 8b879eef Author:Ajit Ghaisas URL: https://git.openjdk.java.net/jfx/commit/8b879eef96c9635a9ed16599344fa90568be2845 Stats: 173 lines in 1 file changed: 22 ins; 109 del; 42 mod 8271085: TabPane: Redundant API docs Reviewed-by: kcr, nlisker - PR: https://git.openjdk.java.net/jfx/pull/728
Re: RFR: 8187309: TreeCell must not change tree's data
On Wed, 9 Feb 2022 14:04:55 GMT, Jeanette Winzenburg wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/TreeView.java >> line 341: >> >>> 339: setFocusModel(new TreeViewFocusModel(this)); >>> 340: >>> 341: setOnEditCommit(DEFAULT_EDIT_COMMIT_HANDLER); >> >> You are adding the default edit commit handler to TreeView. Although it >> seems to be correct, this default handler is likely to be overwritten if the >> user code already has a setOnEditCommit() call. This is shown by the >> test_rt_29650() failure in TreeviewTest.java which you have corrected. >> >> The changes done in this PR makes TreeView similar to ListView and TableView >> in terms of having the default edit commit handler. >> >> Unfortunately, I do not see how can we avoid user code accidentally >> overwriting the default edit commit handler. Documenting this possibility >> seems to be the only way ahead. >> Any other idea? > > Yes, the change might break application code, though that code would be > buggy. Actually, the behavior as implemented now, _already is_ documented :) > >> It is very important to note that if you call >> setOnEditCommit(javafx.event.EventHandler) with your own EventHandler, then >> you will be removing the default handler. Unless you then handle the >> writeback to the property (or the relevant data source), nothing will >> happen. > > so: if they have a custom handler that doesn't save the data, they were > violating the specification (though were getting away with it due to the bug). > > Personally, I think that we cannot keep backward compatibility to bugs. Thanks for the explanation. I see that it is already well documented. - PR: https://git.openjdk.java.net/jfx/pull/724
Re: RFR: 8187309: TreeCell must not change tree's data
On Wed, 2 Feb 2022 14:18:18 GMT, Jeanette Winzenburg wrote: > Issue was TreeView commit editing implementation violated the spec'ed > mechanism: > > - no default commit handler on TreeView > - TreeCell modifying the data directly > > Fix is to move the saving of the edited value from cell into a default commit > handler in tree and set that handler in the constructor. > > Added tests that failed/passed before/after the fix (along with a sanity test > for default commit that passed also before). Also fixed a test bug (incorrect > registration of custom commit handler, see > [JDK-8280951)](https://bugs.openjdk.java.net/browse/JDK-8280951) in > TreeViewTest.test_rt_29650 to keep it passing. Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/724
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs [v2]
On Wed, 9 Feb 2022 13:34:13 GMT, Nir Lisker wrote: >> Good suggestion. >> >> Also optional, you can remove the `` since they are not needed for the >> initial paragraph. > > There are many redundant `` tags in the property docs too if you want to > deal with them as well for consistency (not important). Fixed the description. Also, removed many redundant `` tags. - PR: https://git.openjdk.java.net/jfx/pull/728
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs [v3]
> This is a Javadoc cleanup and correction fix for the TabPane as described in > the JBS. > > Changes done for all the Properties of the TabPane - > - Moved the property description to be over the property field. > - Removed the unnecessary docs on property setter/getter and Property method. Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: Address additional review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/728/files - new: https://git.openjdk.java.net/jfx/pull/728/files/66630a39..537dcac1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=728&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=728&range=01-02 Stats: 25 lines in 1 file changed: 0 ins; 4 del; 21 mod Patch: https://git.openjdk.java.net/jfx/pull/728.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/728/head:pull/728 PR: https://git.openjdk.java.net/jfx/pull/728
Re: RFR: 8187309: TreeCell must not change tree's data
On Wed, 2 Feb 2022 14:18:18 GMT, Jeanette Winzenburg wrote: > Issue was TreeView commit editing implementation violated the spec'ed > mechanism: > > - no default commit handler on TreeView > - TreeCell modifying the data directly > > Fix is to move the saving of the edited value from cell into a default commit > handler in tree and set that handler in the constructor. > > Added tests that failed/passed before/after the fix (along with a sanity test > for default commit that passed also before). Also fixed a test bug (incorrect > registration of custom commit handler, see > [JDK-8280951)](https://bugs.openjdk.java.net/browse/JDK-8280951) in > TreeViewTest.test_rt_29650 to keep it passing. modules/javafx.controls/src/main/java/javafx/scene/control/TreeView.java line 341: > 339: setFocusModel(new TreeViewFocusModel(this)); > 340: > 341: setOnEditCommit(DEFAULT_EDIT_COMMIT_HANDLER); You are adding the default edit commit handler to TreeView. Although it seems to be correct, this default handler is likely to be overwritten if the user code already has a setOnEditCommit() call. This is shown by the test_rt_29650() failure in TreeviewTest.java which you have corrected. The changes done in this PR makes TreeView similar to ListView and TableView in terms of having the default edit commit handler. Unfortunately, I do not see how can we avoid user code accidentally overwriting the default edit commit handler. Documenting this possibility seems to be the only way ahead. Any other idea? - PR: https://git.openjdk.java.net/jfx/pull/724
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs [v2]
> This is a Javadoc cleanup and correction fix for the TabPane as described in > the JBS. > > Changes done for all the Properties of the TabPane - > - Moved the property description to be over the property field. > - Removed the unnecessary docs on property setter/getter and Property method. Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: Address review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/728/files - new: https://git.openjdk.java.net/jfx/pull/728/files/31ae34d7..66630a39 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=728&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=728&range=00-01 Stats: 59 lines in 1 file changed: 11 ins; 14 del; 34 mod Patch: https://git.openjdk.java.net/jfx/pull/728.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/728/head:pull/728 PR: https://git.openjdk.java.net/jfx/pull/728
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs
On Mon, 7 Feb 2022 16:17:32 GMT, Nir Lisker wrote:
>> This is a Javadoc cleanup and correction fix for the TabPane as described in
>> the JBS.
>>
>> Changes done for all the Properties of the TabPane -
>> - Moved the property description to be over the property field.
>> - Removed the unnecessary docs on property setter/getter and Property method.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 174:
>
>> 172: /**
>> 173: * Sets the model used for tab selection. By changing the model
>> you can alter
>> 174: * how the tabs are selected and which tabs are first or last.
>
> This description is phrased like it's a setter. Probably
>
> The selection model used for selecting tabs. Changing the model alters
> how the tabs are selected and which tabs are first or last.
>
> (there's no need to the `` either)
Fixed.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 188:
>
>> 186: * the TabPane will immediately update the location of the tabs to
>> reflect
>> 187: * this.
>> 188: * The default position for the tabs is {@code Side.Top}.
>
> I would rephrase a bit:
>
> The position of the tabs in the {@code TabPane}. Changes to the value of this
> property
> immediately update the location of the tabs.
>
> @defaultValue {@code Side.Top}
>
> The 2nd sentence seems redundant anyway and I suggest to remove it. Unless
> otherwise specified, all value changes are applied immediately (only
> `Animation` properties come to mind that specify that the animation has to be
> stopped for the changes to take effect).
I like the rephrased version. I prefer keeping the 2nd sentence in this case as
it indicates a non-trivial visual update.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 244:
>
>> 242: * Refer to the {@link TabClosingPolicy} enumeration for further
>> details.
>> 243: *
>> 244: * The default closing policy is TabClosingPolicy.SELECTED_TAB
>
> * The default value should be in an `@defaultValue` annotation.
> * Missing `{@code }`s
> * The line "Refer to the {@link TabClosingPolicy}" is redundant I think since
> it's linked in the signature/definition, and if we want to keep it then an
> `@see` might be preferable.
> * "end-user's" (missing apostrophe)
Fixed points 1, 2 and 4.
Regarding point 3 -
I have removed the description regarding possible options and have kept only
the line-
`Refer to the {@link TabClosingPolicy} enumeration for further details.`
Mentioning all the options here seems to be a duplication of Javadoc for
enumeration TabClosingPolicy.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 270:
>
>> 268: * the graphic isn't rotated, resulting in it always appearing
>> upright. If
>> 269: * rotateGraphic is set to {@code true}, the graphic will rotate
>> such that it
>> 270: * rotates with the tab text.
>
> * Missing `@code`s
> * I would rephrase the 2nd paragraph to be simpler:
> ```
> If the value is {@code false}, the graphic isn't rotated, resulting in it
> always appearing upright.
> If it is {@code true}, the graphic is rotate with the tab text.
> ```
> * `@defaultValue {@code false}`
Fixed.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 295:
>
>> 293: *
>> 294: * This value can also be set via CSS using {@code
>> -fx-tab-min-width}.
>> 295: *
>
> I would write
>
> The minimum width of a tab in the TabPane. This can be used to limit
> the length of text in tabs to prevent truncation. Setting the same minimum
> and maximum widths will fix the width of the tab.
>
> It makes it clear that it applies to each tab individually (and not the total
> width of the tabs). The same applies for the other min/max height/width
> properties.
>
> The sentence "By default the min equals to the max." seems wrong. The
> `DEFAULT_TAB_MIN_WIDTH` constant is set to 0. It should also be in a
> `@defaultValue`.
>
> The CSS mention is good, bit I never saw it mentioned for properties before.
> Makes me wonder if we should add the CSS property as part of a property's
> description in general via the automated javadoc tool.
Good suggestion. Fixed.
Regarding CSS mention for properties, I am not sure whether that can be
automated. If there is a way to achieve it, I can file a separate bug to
address it.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 337:
>
>> 335: *
>> 336: * This value can also be set via CSS using {@code
>> -fx-tab-max-width}.
>> 337: *
>
> * "By default the min equals to the max." The default is `Double.MAX_VALUE`.
> * Comma before "the text will be truncated".
Fixed.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 378:
>
>> 376: *
>> 377: * This value can also be set via CSS using {@code
>> -fx-tab-min-h
Re: [jfx18] RFR: 8271085: TabPane: Redundant API docs
On Mon, 7 Feb 2022 16:24:46 GMT, Kevin Rushforth wrote:
>> This is a Javadoc cleanup and correction fix for the TabPane as described in
>> the JBS.
>>
>> Changes done for all the Properties of the TabPane -
>> - Moved the property description to be over the property field.
>> - Removed the unnecessary docs on property setter/getter and Property method.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 188:
>
>> 186: * the TabPane will immediately update the location of the tabs to
>> reflect
>> 187: * this.
>> 188: * The default position for the tabs is {@code Side.Top}.
>
> Ordinarily we would use a `@defaultValue` tag here. In this case, you are
> just moving existing docs from one method to another, so it could be done in
> a follow-up bug. Can you file a follow-up bug?
I have fixed it as part of this PR itself.
-
PR: https://git.openjdk.java.net/jfx/pull/728
Re: RFR: 8273336: Clicking a selected cell from a group of selected cells in a TableView clears the selected items list but remains selected [v3]
On Mon, 7 Feb 2022 18:46:55 GMT, Jose Pereda wrote: >> This PR adds a predicate to TableView and TreeTableView selection models >> order to remove rows from the selection only when there are no selected >> cells in that given row, when cell selection is enabled. >> >> Two tests have been added as well, that fail without this PR and pass with >> it. > > Jose Pereda has updated the pull request incrementally with one additional > commit since the last revision: > > Address feedback from reviewer Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/709
[jfx18] RFR: 8271085: TabPane: Redundant API docs
This is a Javadoc cleanup and correction fix for the TabPane as described in the JBS. Changes done for all the Properties of the TabPane - - Moved the property description to be over the property field. - Removed the unnecessary docs on property setter/getter and Property method. - Commit messages: - Fix Property javadocs Changes: https://git.openjdk.java.net/jfx/pull/728/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=728&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8271085 Stats: 122 lines in 1 file changed: 17 ins; 97 del; 8 mod Patch: https://git.openjdk.java.net/jfx/pull/728.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/728/head:pull/728 PR: https://git.openjdk.java.net/jfx/pull/728
Re: RFR: 8278980: Update WebKit to 613.1 [v2]
On Wed, 2 Feb 2022 15:29:51 GMT, Ambarish Rapte wrote: >> Update JavaFX WebKit to GTK WebKit 2.34 (613.1). >> >> Verified the updated version build, tests run and sanity testing. >> This does not cause any issues except a unit test failure >> `IrresponsiveScriptTest`. >> It is recorded and ignored using >> [JDK-8280421](https://bugs.openjdk.java.net/browse/JDK-8280421) > > Ambarish Rapte has updated the pull request incrementally with one additional > commit since the last revision: > > Restore commits that were mistakenly reverted. > > 8279013: ES2Pipeline fails to detect AMD vega20 graphics card > 8277853: With Touch enabled devices scrollbar disappears and the table is > scrolled to the beginning > 8277122: SplitPane divider drag can hang the layout Marked as reviewed by aghaisas (Reviewer). I have tested on macOS and Windows. No new issue observed. - PR: https://git.openjdk.java.net/jfx/pull/723
Re: RFR: 8273336: Clicking a selected cell from a group of selected cells in a TableView clears the selected items list but remains selected
On Fri, 7 Jan 2022 19:36:45 GMT, Jose Pereda wrote: > This PR adds a predicate to TableView and TreeTableView selection models > order to remove rows from the selection only when there are no selected cells > in that given row, when cell selection is enabled. > > Two tests have been added as well, that fail without this PR and pass with it. This looks good to me. @mstr2, I see that you have some made some suggestions on JBS. It would be great if you can review this PR. - PR: https://git.openjdk.java.net/jfx/pull/709
Re: RFR: 8277122: SplitPane divider drag can hang the layout [v4]
On Thu, 27 Jan 2022 20:48:40 GMT, Marius Hanl wrote: >> When a divider is moved via drag or code it will call **requestLayout()** >> for the **SplitPane**. >> While this is fine, it is also called when the >> **SplitPaneSkin#layoutChildren(..)** method is repositioning the divider. >> >> This makes no sense since we are currently layouting everything, so we don't >> need to request it again. (The divider positioning is the first part of >> **layoutChildren(..)**. In the second part the SplitPane content is layouted >> based off those positions) >> >> -> With this behaviour the layout may hang under some conditions (check >> attached source). The problem is that the **requestLayout()** will mark the >> **SplitPane** dirty but won't propagate to the parent since the >> **SplitPane** is currently doing a layout. >> >> This PR fixes this by not requesting layout when we are currently doing it >> (and thus repositioning the dividers as part of it). >> >> Note: I also fixed a simple typo of a private method in SplitPaneSkin: >> initializeDivderEventHandlers -> initializeDiv**i**derEventHandlers > > Marius Hanl has updated the pull request incrementally with one additional > commit since the last revision: > > 8277122: Added and used global stageLoader + changed copyright year to 2022 The fix looks good. +1. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/669
Re: [jfx18] RFR: 8280275: JUnit5 tests using Assumptions API fail to compile in some cases
On Wed, 19 Jan 2022 15:18:48 GMT, Kevin Rushforth wrote: > Fixed a test dependency issue in `build.gradle` that causes a compilation > failure when running `gradle test` if the `Assumptions` API from JUnit5 is > used. I added a test that fails to compile without the build fix and passes > with the fix. > > This was first discovered when testing the patch for PR #715, which fails to > compile on my local macOS system. After applying this fix, I can compile and > run the test from that PR. I verified that the test fails to compile without fix & runs successfully with the fix. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/720
Re: RFR: 8187307: ListView, TableView, TreeView: receives editCancel event when edit is committed
On Tue, 30 Nov 2021 12:32:37 GMT, Jeanette Winzenburg wrote: > The misbehaviour was that an edit handler received both a commit and cancel > event when cell commitEdit is called. That happened whenever a collaborator > reset the controls editing state (either directly or indirectly) while > processing the editCommit event. The reason was that the cell had not yet > updated its own editing state when receiving the change of editing from the > control. > > Fix is to update cell's editing state before firing the event, that is change > the sequence or method calls from fire/super.commit to super.commit/fire. > > Added tests that fail/pass before/after the fix. The fix and tests look good. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/684
Re: RFR: 8244234: MenuButton: NPE on removing from scene with open popup [v2]
On Mon, 17 Jan 2022 15:32:13 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. > > 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 Marked as reviewed by aghaisas (Reviewer). - 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.
The fix looks OK, but, a few cleanups are required.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuButtonSkinBase.java
line 157:
> 155:
> 156: // We only need to remove the mnemonics from the old
> scene,
> 157: // they will be added to the new one as soon as the pop
> becomes visible again.
typo : pop --> popup
modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuButtonSkinBase.java
line 250:
> 248: @Override protected double computeMinWidth(double height, double
> topInset, double rightInset, double bottomInset, double leftInset) {
> 249: return leftInset
> 250:+ label.minWidth(height)
Unintended spacing changes are there in computePrefHeight, computePrefWidth,
computeMinHeight and computeMinWidth methods. Please revert these unrelated
spacing changes.
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/MenuButtonSkinBaseTest.java
line 1:
> 1: package test.javafx.scene.control.skin;
Please add the missing copyright header.
-
PR: https://git.openjdk.java.net/jfx/pull/713
Re: [jfx18] RFR: 8273998: Clarify specification for Window properties controlled by the window manager
On Fri, 7 Jan 2022 18:19:23 GMT, Kevin Rushforth wrote: > Update the API specification for the `Window` and `Stage` classes to clarify > that the values for some properties and methods can be changed or ignored by > the platform. Several of the properties already have a comment to the effect > that the value can change externally, and thus the properties are not > bindable, but this should be clarified further and for all such properties > and methods. > > See the Description of the JBS issues for this bug and its associated CSR for > more details. Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/708
Re: RFR: 8273998: Clarify specification for Window properties controlled by the window manager
On Fri, 7 Jan 2022 18:19:23 GMT, Kevin Rushforth wrote: > Update the API specification for the `Window` and `Stage` classes to clarify > that the values for some properties and methods can be changed or ignored by > the platform. Several of the properties already have a comment to the effect > that the value can change externally, and thus the properties are not > bindable, but this should be clarified further and for all such properties > and methods. > > See the Description of the JBS issues for this bug and its associated CSR for > more details. Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/708
Re: RFR: 8273998: Clarify specification for Window properties controlled by the window manager
On Fri, 7 Jan 2022 18:19:23 GMT, Kevin Rushforth wrote:
> Update the API specification for the `Window` and `Stage` classes to clarify
> that the values for some properties and methods can be changed or ignored by
> the platform. Several of the properties already have a comment to the effect
> that the value can change externally, and thus the properties are not
> bindable, but this should be clarified further and for all such properties
> and methods.
>
> See the Description of the JBS issues for this bug and its associated CSR for
> more details.
Changes look good to me except at one place where you have removed the
description. I will it out separately.
modules/javafx.graphics/src/main/java/javafx/stage/Window.java line 660:
> 658: /**
> 659: * Whether or not this {@code Window} has the keyboard or input
> focus.
> 660: *
Is this removal intentional?
-
PR: https://git.openjdk.java.net/jfx/pull/708
Re: RFR: 8197991: Selecting many items in a TableView is very slow [v6]
On Fri, 7 Jan 2022 08:04:53 GMT, Abhinay Agarwal wrote: >> This work improves the performance of `MultipleSelectionModel` over large >> data sets by caching some values and avoiding unnecessary calls to >> `SelectedIndicesList#size`. It further improves the performance by reducing >> the number of iterations required to find the index of an element in the >> BitSet. >> >> The work is based on [an abandoned >> patch](https://github.com/openjdk/jfx/pull/127) submitted by @yososs >> >> There are currently 2 manual tests for this fix. > > Abhinay Agarwal has updated the pull request incrementally with two > additional commits since the last revision: > > - Format code and reduce test size > - Add size assertion to test The code changes look good. I tested on Mac. This PR drastically improves the selection time for the provided manual test. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/673
Re: RFR: 8279328: CssParser uses default charset instead of UTF-8
On Wed, 29 Dec 2021 15:39:39 GMT, Michael Strauß wrote: > `CssParser.parse(URL)` is specified to assume UTF-8 file encoding, but > (implicitly) uses the default charset to read the file, potentially resulting > in incorrect interpretation of the file content. > > This can be fixed by explicitly specifying the UTF-8 charset for > `InputStreamReader`. Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/705
Re: RFR: 8279328: CssParser uses default charset instead of UTF-8
On Wed, 29 Dec 2021 15:39:39 GMT, Michael Strauß wrote: > `CssParser.parse(URL)` is specified to assume UTF-8 file encoding, but > (implicitly) uses the default charset to read the file, potentially resulting > in incorrect interpretation of the file content. > > This can be fixed by explicitly specifying the UTF-8 charset for > `InputStreamReader`. The added test passes on my Mac without changing CssParser.java. It may be due to the default charset being UTF-8. Which platform did you test on? - PR: https://git.openjdk.java.net/jfx/pull/705
Re: RFR: 8278134: Move static utility methods to infrastructure (EditAndScrollTest) [v2]
On Thu, 16 Dec 2021 12:54:27 GMT, Jeanette Winzenburg wrote: >> Extracted static test utility methods from EditAndScrollTest into new >> VirtualizedControlTestUtils, added rudimentary tests for the methods. > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > fixed c&p error in doc Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/690
Re: RFR: 8278260: JavaFX shared libraries not stripped on Linux or macOS [v2]
On Wed, 15 Dec 2021 22:57:26 GMT, Kevin Rushforth wrote: >> Build change to strip the non-global symbols from native shared libraries on >> Linux and macOS by running `strip -x`, unless doing a `-PCONF=DebugNative` >> build. >> >> Here is a before / after size comparison. All sizes in KBytes: >> >> ### Linux >> >> | Native Library | Current | Stripped | >> | --- | - | | >> | libavplugin-54.so | 52 | 48 | >> | libavplugin-56.so | 52 | 48 | >> | libavplugin-57.so | 52 | 48 | >> | libavplugin-ffmpeg-56.so | 52 | 48 | >> | libavplugin-ffmpeg-57.so | 52 | 48 | >> | libavplugin-ffmpeg-58.so | 52 | 48 | >> | libdecora_sse.so | 76 | 72 | >> | libfxplugins.so | 56 | 52 | >> | libglass.so | 16 | 12 | >> | libglassgtk2.so | 932 | 324 | >> | libglassgtk3.so | 932 | 324 | >> | libgstreamer-lite.so | 2,280 | 2,100 | >> | libjavafx_font.so | 20 | 16 | >> | libjavafx_font_freetype.so | 28 | 28 | >> | libjavafx_font_pango.so | 28 | 24 | >> | libjavafx_iio.so | 148 | 140 | >> | libjfxmedia.so | 2,048 | 516 | >> | libjfxwebkit.so | 106,696 | 88,428 | >> | libprism_common.so | 8 | 8 | >> | libprism_es2.so | 64 | 64 | >> | libprism_sw.so | 68 | 64 | >> >> ### macOS >> >> | Native Library | Current | Stripped | >> | --- | - | | >> | libdecora_sse.dylib | 88 | 88 | >> | libfxplugins.dylib | 88 | 84 | >> | libglass.dylib | 360 | 324 | >> | libglib-lite.dylib | 1,192 | 1,148 | >> | libgstreamer-lite.dylib | 1,708 | 1584 | >> | libjavafx_font.dylib | 64 | 64 | >> | libjavafx_iio.dylib | 308 | 300 | >> | libjfxmedia.dylib | 212 | 200 | >> | libjfxmedia_avf.dylib | 100 | 88 | >> | libjfxwebkit.dylib | 85,896 | 71,636 | >> | libprism_common.dylib | 20 | 20 | >> | libprism_es2.dylib | 64 | 64 | >> | libprism_sw.dylib | 88 | 88 | > > Kevin Rushforth has updated the pull request incrementally with one > additional commit since the last revision: > > Only strip binaries when doing a production build (`-PCONF=Release`) Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/695
Re: RFR: 8278134: Move static utility methods to infrastructure (EditAndScrollTest)
On Thu, 9 Dec 2021 12:43:54 GMT, Jeanette Winzenburg wrote: > Extracted static test utility methods from EditAndScrollTest into new > VirtualizedControlTestUtils, added rudimentary tests for the methods. This looks good except for a typo which is not introduced by you! modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/infrastructure/VirtualizedControlTestUtils.java line 86: > 84: > 85: /** > 86: * Returns a vertical ScrollBar of the control. I found this typo. I know, you just moved it from one file to another.. but, it would be good to fix in this PR :) Typo : This should be - "Returns a horizontal..." - PR: https://git.openjdk.java.net/jfx/pull/690
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v4]
On Fri, 10 Dec 2021 15:24:44 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: > > 8276167: Avoid additional Toolkit.getToolkit().firePulse() > > Avoid using of additional Toolkit.getToolkit().firePulse() in the test, > adjust expected values instead. Marked as reviewed by aghaisas (Reviewer). - 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) Fix looks good. I verified that the tests fail without fix and pass with it. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8201538: Remove implementation support for applets from JavaFX
On Tue, 31 Aug 2021 16:28:53 GMT, Kevin Rushforth wrote: > This PR removes the obsolete applet implementation from JavaFX. It is an > ongoing maintenance burden to carry around this legacy code. Also, cleaning > this up could help in the implementation of GTK4, Wayland, and Metal, since > we won't have to account for the way applet windows are created and managed. > > ## Notes to reviewers: > > The first part of the removal was to eliminate the methods and classes on the > Java side that are associated with creating and managing an applet window, > and which are no longer called. After these were removed, I then removed the > corresponding methods and classes on the native side that are no longer > called. > > ### Shared Code > > The following were removed from the shared code. > > Removed Java classes > > > com.sun.javafx.tk.AppletWindow > com.sun.javafx.tk.quantum.GlassAppletWindow > > > Removed methods > > The following methods were removed in the parent class and all subclasses. > > > com.sun.glass.ui.Application: > public abstract Window createWindow(long parent) > > com.sun.glass.ui.Window: > public boolean getAppletMode() > public void setAppletMode(boolean appletMode) > public void dispatchNpapiEvent(Map eventInfo) > protected abstract long _createChildWindow(long parent) > protected Window(long parent) > protected abstract int _getEmbeddedX(long ptr) > protected abstract int _getEmbeddedY(long ptr) > > com.sun.javafx.tk.Toolkit: > public abstract AppletWindow createAppletWindow(...) > public abstract void closeAppletWindow() > > com.sun.javafx.tk.quantum.WindowStage: > static void setAppletWindow(GlassAppletWindow aw) > static GlassAppletWindow getAppletWindow() > > > > ### Linux (Gtk) Java code > > The following classes or methods were removed: > > > com.sun.glass.ui.gtk.GtkChildWindow (class removed) > > com.sun.glass.ui.gtk.GtkWindow: > protected GtkWindow(long parent) > > > ### Linux (Gtk) native glass: > > The following native classes were removed: > > > WindowContextChild > WindowContextPlug > > > ### macOS Java code > > The following classes or methods were removed: > > > com.sun.glass.events.mac.NpapiEvent (class removed) > > com.sun.glass.ui.mac.MacApplication: > native protected String _getRemoteLayerServerName() > > com.sun.glass.ui.View: > public int getNativeRemoteLayerId(String serverName) > > com.sun.glass.ui.mac.MacView: > native protected int _getNativeRemoteLayerId(long ptr, String serverName) > native protected void _hostRemoteLayerId(long ptr, int nativeLayerId) > > com.sun.glass.ui.mac.MacWindow: > protected MacWindow(long parent) > > > ### macOS native code > > The following native classes were removed: > > > GlassEmbeddedWindow* > GlassNSEvent > GlassView3D+Remote > RemoteLayerSupport > > > I also removed the `jIsChild` parameter from the window creation code which > allowed for removing a lot of dead blocks of code. The main window creation > method was: > > > - (id)_initWithContentRect:(NSRect)contentRect > styleMask:(NSUInteger)windowStyle > screen:(NSScreen *)screen jwindow:(jobject)jwindow > jIsChild:(jboolean)jIsChild > > > This created a `GlassEmbeddedWindow` iff `jIsChild == JNI_TRUE`. Since > `jIsChild` was only set to true by the (now removed) > `_createChildWindow(long)` method, we can remove the parameter, the > `GlassEmbeddedWindow*` classes, and all code blocks that are qualified by `if > (jIsChild)`. > > ### Windows Java code > > The following classes or methods were removed: > > > com.sun.glass.ui.win.WinChildWindow (class removed) > > com.sun.glass.ui.win.WinWindow: > protected WinWindow(long parent) > > > ### Windows native code > > After removing all references to `IsChild()`, which was only ever true for > `_createChildWindow()`, we can also remove the following: > > > GlassApplication::InstallMouseLLHook > GlassApplication::UninstallMouseLLHook > > > ### iOS Java code > > > com.sun.glass.ui.ios.IosWindow: > protected IosWindow(long parent) > > > ### iOS native code > > With the removal of the `_createChildWindow` method, the following JNI method > in `IosWindow` can be removed: > > > Java_com_sun_glass_ui_ios_IosWindow__1createChildWindow(JNIEnv *, jobject, > jlong) > > > As a note, I don't have a setup to build this. It is a simple, safe change, > but should be double-checked by someone from Gluon. Changes look good. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/615
Re: RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height [v3]
On Thu, 2 Dec 2021 22:49:45 GMT, Michael Strauß wrote: >> This PR fixes an issue where the scroll delta of ScrollPane incorrectly >> depends on the size of its content. >> This leads to extremely slow scrolling when the content is only slightly >> larger than the ScrollPane. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > Changes per review Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/660
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v2]
On Fri, 3 Dec 2021 11:16:28 GMT, Johan Vos wrote: > The hard values have been changed a number of times, and I believe it is not > really a good metric. I agree completely. > Rather than requiring that the amount of calls should be a fixed number, I > think it makes more sense to ensure that the amount of calls stays within > reasonable boundaries, and is not growing exponentially when we add linearly > more items, for example. My point is exactly this. I see that as part of this PR, you have added the upper boundary (rather than a fixed number) for assertions. I am asking whether we need a lower boundary as well? - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v2]
On Tue, 30 Nov 2021 11:02:41 GMT, Johan Vos wrote:
>> After (re)setting the number of elements, make sure to do at least some
>> estimation of the total size.
>> Added a testcase for this scenario.
>
> Johan Vos has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Move functionality in the setCellCount() to the invalidated block.
> Some hard numbers used in tests (counters for evaluations) were changed
> because of this.
> Instead of relying on hard values, I modified the failing was into relative
> ones.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 860:
> 858: int cellCount = get();
> 859: resetSizeEstimates();
> 860: recalculateAndImproveEstimatedSize(2);
We can use recalculateEstimatedSize() instead of this method.
The effect is the same improvement with a size of 2.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 1124:
> 1122: Platform.runLater(() -> {
> 1123: Toolkit.getToolkit().firePulse();
> 1124: assertTrue(rt_35395_counter < 7);
I see that you have modified assertions to use "lesser than" some expected
value. This may hide some incorrect test outcomes.
Along with "lesser than" assertion, do you think we should add a "greater than"
assertion as well? This will have a range bounded expected value.
This is applicable for all assertion changes in this PR.
-
PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8277457: AccessControlException: access denied ("java.net.NetPermission" "getCookieHandler") [v2]
On Thu, 25 Nov 2021 00:29:36 GMT, Kevin Rushforth wrote: >> As indicated in the bug report, WebView needs to call to >> `CookieManager::getDefault` within a `doPrivileged` block so that it will >> work when a security manager is enabled. There are two calls in >> `com.sun.webkit.network.CookieJar` that are not wrapped in a `doPrivileged`. >> >> I added a manual test (since it requires loading a page over http/https >> which we can't do in our automated tests) based on the test program that was >> attached to JBS. > > Kevin Rushforth has updated the pull request incrementally with one > additional commit since the last revision: > > Address review feedback: use method reference instead of lambda Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/681
Re: RFR: 8272118: ListViewSkin et al: must not cancel edit on scrolling
On Thu, 25 Nov 2021 15:46:01 GMT, Jeanette Winzenburg wrote: > Issue was that mouse pressed on the scrollbars of all virtualized controls > cancelled the edit. That's inconsistent with other scroll triggers > (mouseWheel, programmatic). Fixed by removing the cancel. > > Added tests that failed/passed before/after the fix. Also added tests that > passed both before/after to guarantee that required functionality of the > mouse pressed (= requesting focus on the control if needed) is still working. Looks good to me. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/682
Re: RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height
On Tue, 2 Nov 2021 10:49:45 GMT, Michael Strauß wrote:
> This PR fixes an issue where the scroll delta of ScrollPane incorrectly
> depends on the size of its content.
> This leads to extremely slow scrolling when the content is only slightly
> larger than the ScrollPane.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/ScrollPaneSkin.java
line 896:
> 894: double vPixelValue;
> 895: if (nodeHeight > 0.0) {
> 896: vPixelValue = vRange / (nodeHeight - contentHeight);
This may result in divide by 0.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/ScrollPaneSkin.java
line 928:
> 926: double hPixelValue;
> 927: if (nodeWidth > 0.0) {
> 928: hPixelValue = hRange / (nodeWidth - contentWidth);
This may result in divide by 0.
-
PR: https://git.openjdk.java.net/jfx/pull/660
Re: RFR: 8276490: Incorrect path for duplicate x and y values, when path falls outside axis bound [v3]
On Mon, 22 Nov 2021 14:04:01 GMT, Abhinay Agarwal wrote: >> PathElements were skipped in AreaChart if the data point were outside axis >> bounds and had duplicate value for either x or y. This is now fixed with >> this PR. > > Abhinay Agarwal has updated the pull request incrementally with one > additional commit since the last revision: > > Update line ending Marked as reviewed by aghaisas (Reviewer). Looks good to me. - PR: https://git.openjdk.java.net/jfx/pull/667
Re: RFR: 8276144: Update boot JDK to 17.0.1
On Mon, 15 Nov 2021 18:26:00 GMT, Kevin Rushforth wrote: > Now that JavaFX has updated to gradle 7.3 -- see > [JDK-8276142](https://bugs.openjdk.java.net/browse/JDK-8276142) / PR #671 -- > we can update the boot JDK used to build JavaFX to JDK 17.0.1. > > I have run a smoke test on all three platforms. > > I will run a full set of tests in parallel with the review. Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/670
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v5]
On Fri, 19 Nov 2021 11:42:16 GMT, Jeanette Winzenburg wrote: >> Cleanup of Tree-/TableRowSkin to support switching skins >> >> The misbehavior/s >> - memory leaks due to manually registered listeners that were not removed >> - side-effects due to listeners still active on old skin (like NPEs) >> >> Fix >> - use skin api for all listener registration (for automatic removal in >> dispose) >> - ~~do not install listeners that are not needed (fixedCellSize, same as in >> fix of ListCellSkin >> [JDK-8246745](https://bugs.openjdk.java.net/browse/JDK-8246745))~~ not >> handled here, see >> [JDK-8277000](https://bugs.openjdk.java.net/browse/JDK-8277000) >> >> Added tests for each listener involved in the fix to guarantee it's still >> working and does not have unwanted side-effects after switching skins. >> >> Note: there are pecularities in row skins (like not updating themselves on >> property changes of its control, throwing NPEs when not added to a >> VirtualFlow) which are not part of this issue but covered in >> [JDK-8274065](https://bugs.openjdk.java.net/browse/JDK-8274065) > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > changes as requested in review Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Mon, 15 Nov 2021 13:28:40 GMT, Jeanette Winzenburg wrote: >> My PR is already merged, so this is not a problem. :) >> I dont know, but since this is only fixing a (also before) wrong comment it >> might be okay as it is very minor? :) > > FYI: now the listener registration - including the incorrect code comment > (which is the same as in current master) - is back at the old location in the > re-inserted setupTreeTableViewListeners. So still need input whether it's > okay to correct the code comment here. I think, it is okay to fix the comment in this review itself. It is very minor to warrant a separate PR. Also, this anomaly was discovered in this review makes it a candidate to be fixed here. - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v4]
On Mon, 15 Nov 2021 13:11:06 GMT, Jeanette Winzenburg wrote: >> Cleanup of Tree-/TableRowSkin to support switching skins >> >> The misbehavior/s >> - memory leaks due to manually registered listeners that were not removed >> - side-effects due to listeners still active on old skin (like NPEs) >> >> Fix >> - use skin api for all listener registration (for automatic removal in >> dispose) >> - ~~do not install listeners that are not needed (fixedCellSize, same as in >> fix of ListCellSkin >> [JDK-8246745](https://bugs.openjdk.java.net/browse/JDK-8246745))~~ not >> handled here, see >> [JDK-8277000](https://bugs.openjdk.java.net/browse/JDK-8277000) >> >> Added tests for each listener involved in the fix to guarantee it's still >> working and does not have unwanted side-effects after switching skins. >> >> Note: there are pecularities in row skins (like not updating themselves on >> property changes of its control, throwing NPEs when not added to a >> VirtualFlow) which are not part of this issue but covered in >> [JDK-8274065](https://bugs.openjdk.java.net/browse/JDK-8274065) > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > reverted fixedCellSize handling Overall the fix looks OK. I have 2 minor comments. modules/javafx.controls/src/shims/java/javafx/scene/control/skin/TableSkinShim.java line 2: > 1: /* > 2: * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved. 2021 :) - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8276490: Incorrect path for duplicate x and y values, when path falls outside axis bound
On Fri, 12 Nov 2021 09:44:47 GMT, Abhinay Agarwal wrote:
> PathElements were skipped in AreaChart if the data point were outside axis
> bounds and had duplicate value for either x or y. This is now fixed with this
> PR.
modules/javafx.controls/src/test/java/test/javafx/scene/chart/AreaChartTest.java
line 187:
> 185: }
> 186:
> 187: @Test public void testPathOutsideXBoundsWithDuplicateXAndLowerY() {
I wonder whether we need another set of similar tests with X and Y interchanged?
Something like - testPathOutsideYBoundsWithDuplicateYAndLowerX
-
PR: https://git.openjdk.java.net/jfx/pull/667
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v4]
On Sat, 30 Oct 2021 20:40:12 GMT, Michael Strauß wrote: >> This PR fixes an issue with mnemonic parsing by removing the restriction >> that a mnemonic symbol must be a letter. Now, it can be any character except >> whitespace. > > Michael Strauß has updated the pull request incrementally with four > additional commits since the last revision: > > - revert rename > - revert rename > - test for KeyCombination > - renamed TextBinding to MnemonicParser, refactored tests Marked as reviewed by aghaisas (Reviewer). The fix and test is fine. - PR: https://git.openjdk.java.net/jfx/pull/647
Integrated: 8275848: Deprecate for removal mistakenly exposed field from class javafx.scene.shape.Box
On Fri, 29 Oct 2021 07:31:05 GMT, Ajit Ghaisas wrote: > This PR deprecates mistakenly exposed field from class javafx.scene.shape.Box. This pull request has now been integrated. Changeset: d9e1ad97 Author: Ajit Ghaisas URL: https://git.openjdk.java.net/jfx/commit/d9e1ad976873799128871c0544f6eb26e843b880 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod 8275848: Deprecate for removal mistakenly exposed field from class javafx.scene.shape.Box Reviewed-by: kcr, nlisker - PR: https://git.openjdk.java.net/jfx/pull/655
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]
On Fri, 29 Oct 2021 13:58:35 GMT, Michael Strauß wrote: >> This PR fixes an issue with mnemonic parsing by removing the restriction >> that a mnemonic symbol must be a letter. Now, it can be any character except >> whitespace. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > changed javadoc Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v2]
On Fri, 29 Oct 2021 12:34:50 GMT, Michael Strauß wrote: >> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextBinding.java >> line 238: >> >>> 236: } >>> 237: >>> 238: return !isExtendedMnemonic(s, position); >> >> I am not sure why do we need to check for isExtendedMnemonic() inside >> isSimpleMnemonic() implementation. >> These two methods should be separate and it is up to the callers to check >> and take appropriate action. > > Done. The corresponding comment for the method isSimpleMnemonic() also needs correction. You can limit it to - /** * Determines whether the string contains a simple mnemonic at the specified position. * A simple mnemonic is any two-character string similar to "_x", where x is not an * underscore or a whitespace character. */ - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8275848: Deprecate for removal mistakenly exposed field from class javafx.scene.shape.Box [v2]
> This PR deprecates mistakenly exposed field from class javafx.scene.shape.Box. Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: review fix - Changes: - all: https://git.openjdk.java.net/jfx/pull/655/files - new: https://git.openjdk.java.net/jfx/pull/655/files/d5021e2d..6305fde5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=655&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=655&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/655.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/655/head:pull/655 PR: https://git.openjdk.java.net/jfx/pull/655
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working
On Wed, 20 Oct 2021 16:54:35 GMT, Michael Strauß wrote: > This PR fixes an issue with mnemonic parsing by removing the restriction that > a mnemonic symbol must be a letter. Now, it can be any character except > whitespace. modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextBinding.java line 238: > 236: } > 237: > 238: return !isExtendedMnemonic(s, position); I am not sure why do we need to check for isExtendedMnemonic() inside isSimpleMnemonic() implementation. These two methods should be separate and it is up to the callers to check and take appropriate action. - PR: https://git.openjdk.java.net/jfx/pull/647
Re: RFR: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit [v3]
On Fri, 22 Oct 2021 10:36:24 GMT, Jeanette Winzenburg wrote: >> cell startEdit is supposed to update the editing location on its associated >> control - was done in ListCell, not in Tree-/TableCell nor TreeCell. >> >> Fix was to add control.edit(..). Note that ListCell was also touched to use >> the exact same method call pattern as the fixed cell types. >> >> Added/unignored cell tests that are failing/passing before/after the fix. > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > next try with fixing the formatting Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/638
RFR: 8275848: Deprecate mistakenly exposed field from class javafx.scene.shape.Box
This PR deprecates mistakenly exposed field from class javafx.scene.shape.Box. - Commit messages: - Deprecate DEFAULT_SIZE constant field Changes: https://git.openjdk.java.net/jfx/pull/655/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=655&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8275848 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/655.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/655/head:pull/655 PR: https://git.openjdk.java.net/jfx/pull/655
Integrated: 8271091: Missing API docs in UI controls classes
On Wed, 20 Oct 2021 14:42:44 GMT, Ajit Ghaisas wrote: > This PR fixes javadoc warnings in javafx.controls and javafx.web modules. > Note : > - The javadoc needs to be generated with the JDK 18 EA build. > - 2 javadoc warnings in javafx.controls TabPane class will be fixed under - > [JDK-8271085](https://bugs.openjdk.java.net/browse/JDK-8271085) > - There are still 20 javadoc warnings remaining in javafx.controls module and > 3 warnings remaining in javafx.web module. The root cause is different and > they will be addressed under > [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) This pull request has now been integrated. Changeset: a9474055 Author:Ajit Ghaisas URL: https://git.openjdk.java.net/jfx/commit/a9474055994d104288aff22fdb70f76ed8519627 Stats: 269 lines in 49 files changed: 147 ins; 15 del; 107 mod 8271091: Missing API docs in UI controls classes Reviewed-by: nlisker, kcr - PR: https://git.openjdk.java.net/jfx/pull/646
Integrated: 8271090: Missing API docs in scenegraph classes
On Fri, 22 Oct 2021 11:23:07 GMT, Ajit Ghaisas wrote: > This PR fixes javadoc warnings primarily in javafx.graphics module along with > a remaining few in javafx.fxml, javafx.base and javafx.media modules. > > Note : > - The javadoc needs to be generated with the JDK 18 EA build. > - There are still few remaining warnings in these modules. The root cause is > different and they will be addressed under > [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) This pull request has now been integrated. Changeset: e7a106fa Author:Ajit Ghaisas URL: https://git.openjdk.java.net/jfx/commit/e7a106faf3c0bb0126080d2f516248195679bf61 Stats: 133 lines in 28 files changed: 89 ins; 4 del; 40 mod 8271090: Missing API docs in scenegraph classes Reviewed-by: kcr, nlisker - PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271091: Missing API docs in UI controls classes [v5]
> This PR fixes javadoc warnings in javafx.controls and javafx.web modules. > Note : > - The javadoc needs to be generated with the JDK 18 EA build. > - 2 javadoc warnings in javafx.controls TabPane class will be fixed under - > [JDK-8271085](https://bugs.openjdk.java.net/browse/JDK-8271085) > - There are still 20 javadoc warnings remaining in javafx.controls module and > 3 warnings remaining in javafx.web module. The root cause is different and > they will be addressed under > [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: fix review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/646/files - new: https://git.openjdk.java.net/jfx/pull/646/files/b4d5dc4f..1ab36f72 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=646&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=646&range=03-04 Stats: 10 lines in 3 files changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jfx/pull/646.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/646/head:pull/646 PR: https://git.openjdk.java.net/jfx/pull/646
Re: RFR: 8271091: Missing API docs in UI controls classes [v4]
On Wed, 27 Oct 2021 16:12:11 GMT, Nir Lisker wrote:
>> Ajit Ghaisas has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix review comments
>
> modules/javafx.controls/src/main/java/javafx/scene/control/CheckMenuItem.java
> line 89:
>
>> 87:
>> **/
>> 88: /**
>> 89: * Constructs a default {@code CheckMenuItem}.
>
> `Label` uses "Creates an empty label" for the default constructor because it
> has no text or graphic. Maybe it's more informative that way.
It does make sense to modify it. I will change it.
> modules/javafx.controls/src/main/java/javafx/scene/control/PopupControl.java
> line 1133:
>
>> 1131: */
>> 1132: public CSSBridge() {
>> 1133: }
>
> Looking at the [docs for
> 17](https://openjfx.io/javadoc/17/javafx.controls/javafx/scene/control/PopupControl.CSSBridge.html),
> the constructor there is `protected`, here it's `public`. Was this changed
> recently? If it's supposed to be `protected`, then the constructor is for
> subclasses.
This is a very good catch. Thanks!
Changing this to public was a mistake this PR was about to make.
In openjfx17, there is no constructor defined - that means - it gets generated
implicitly which is `protected`.
I introduced an explicit constructor in this PR while fixing the "missing
javadoc" comment. Inadvertently I had made it `public` in this PR.
Now, I have made it `protected` in the latest commit.
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualContainerBase.java
> line 67:
>
>> 65:
>> 66: /**
>> 67: * Constructs a {@code VirtualContainerBase}
>
> The class is `abstract`, so possibly the constructor should be `protected`,
> and we might want to say "Constructor for subclasses" anyway.
I have corrected the javadoc to match "Constructor for subclasses to call.".
I would like to avoid changing the access modifier to `protected` as part of
javadoc fix.
-
PR: https://git.openjdk.java.net/jfx/pull/646
Re: RFR: 8271090: Missing API docs in scenegraph classes [v4]
> This PR fixes javadoc warnings primarily in javafx.graphics module along with > a remaining few in javafx.fxml, javafx.base and javafx.media modules. > > Note : > - The javadoc needs to be generated with the JDK 18 EA build. > - There are still few remaining warnings in these modules. The root cause is > different and they will be addressed under > [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: fix review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/650/files - new: https://git.openjdk.java.net/jfx/pull/650/files/e4d337c5..69b6a759 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=650&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=650&range=02-03 Stats: 16 lines in 2 files changed: 0 ins; 0 del; 16 mod Patch: https://git.openjdk.java.net/jfx/pull/650.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/650/head:pull/650 PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271090: Missing API docs in scenegraph classes [v3]
On Wed, 27 Oct 2021 16:06:38 GMT, Nir Lisker wrote:
> Added a few more comments, otherwise looks fine.
Thanks for your detailed review.
> modules/javafx.graphics/src/main/java/javafx/stage/PopupWindow.java line 156:
>
>> 154:
>> 155: /**
>> 156: * Creates a {@code PopupWindow}.
>
> The `PopupWIndow` class is abstract. Do we still keep this wording?
I have changed it to - "Constructor for subclasses to call." as in other cases.
> modules/javafx.graphics/src/main/java/javafx/stage/Window.java line 794:
>
>> 792: *
>> 793: * An {@link IllegalStateException} is thrown if this property is
>> set
>> 794: * on a thread other than the JavaFX Application Thread.
>
> Shouldn't this be in a `@throws`?
Yes, only javadoc for `setScene()` method should use `@throws`.
The description for the property does not (and should not) recognize a
`@throws`, hence I have kept the original description intact.
-
PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271091: Missing API docs in UI controls classes [v4]
On Wed, 20 Oct 2021 15:45:52 GMT, Nir Lisker wrote: >> Ajit Ghaisas has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix review comments > > Took a quick look at the new docs. I didn't check the resulting HTML or the > corrected of the description, just format and grammar. @nlisker, can you please take a look and approve? - PR: https://git.openjdk.java.net/jfx/pull/646
Re: RFR: 8271090: Missing API docs in scenegraph classes [v3]
On Tue, 26 Oct 2021 09:54:43 GMT, Ajit Ghaisas wrote: >> This PR fixes javadoc warnings primarily in javafx.graphics module along >> with a remaining few in javafx.fxml, javafx.base and javafx.media modules. >> >> Note : >> - The javadoc needs to be generated with the JDK 18 EA build. >> - There are still few remaining warnings in these modules. The root cause is >> different and they will be addressed under >> [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) > > Ajit Ghaisas has updated the pull request incrementally with one additional > commit since the last revision: > > fix review comments @nlisker, can you please take a look and approve? - PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Wed, 27 Oct 2021 09:56:46 GMT, Jeanette Winzenburg wrote: >> Cleanup of Tree-/TableRowSkin to support switching skins >> >> The misbehavior/s >> - memory leaks due to manually registered listeners that were not removed >> - side-effects due to listeners still active on old skin (like NPEs) >> >> Fix >> - use skin api for all listener registration (for automatic removal in >> dispose) >> - do not install listeners that are not needed (fixedCellSize, same as in >> fix of ListCellSkin >> [JDK-8246745](https://bugs.openjdk.java.net/browse/JDK-8246745)) >> >> Added tests for each listener involved in the fix to guarantee it's still >> working and does not have unwanted side-effects after switching skins. >> >> Note: there are pecularities in row skins (like not updating themselves on >> property changes of its control, throwing NPEs when not added to a >> VirtualFlow) which are not part of this issue but covered in >> [JDK-8274065](https://bugs.openjdk.java.net/browse/JDK-8274065) > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > re-added forgotten code comments Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Wed, 27 Oct 2021 09:50:32 GMT, Jeanette Winzenburg wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkin.java >> line 134: >> >>> 132: // that when it changes, we can appropriately add / >>> remove cells that may or may not >>> 133: // be required (because we remove all cells that are >>> not visible). >>> 134: >>> registerChangeListener(getVirtualFlow().widthProperty(), e -> >>> tableView.requestLayout()); >> >> If this listener is removed, then is there a chance of reintroducing >> [JDK-8144500](https://bugs.openjdk.java.net/browse/JDK-8144500)? >> Unfortunately, there is no test added for that bug.. so it is difficult to >> catch regression, if any. > > hmm .. the listener is not removed, its registration is moved to > updateTableViewSkin. There are tests covering that it really is still > registered :) > > Forgot to move the old code comment, though. Re-added. Thanks for the explanation. - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin
On Fri, 24 Sep 2021 16:01:38 GMT, Jeanette Winzenburg wrote: > Cleanup of Tree-/TableRowSkin to support switching skins > > The misbehavior/s > - memory leaks due to manually registered listeners that were not removed > - side-effects due to listeners still active on old skin (like NPEs) > > Fix > - use skin api for all listener registration (for automatic removal in > dispose) > - do not install listeners that are not needed (fixedCellSize, same as in fix > of ListCellSkin > [JDK-8246745](https://bugs.openjdk.java.net/browse/JDK-8246745)) > > Added tests for each listener involved in the fix to guarantee it's still > working and does not have unwanted side-effects after switching skins. > > Note: there are pecularities in row skins (like not updating themselves on > property changes of its control, throwing NPEs when not added to a > VirtualFlow) which are not part of this issue but covered in > [JDK-8274065](https://bugs.openjdk.java.net/browse/JDK-8274065) I have two comments regarding possibility of introducing a regression. Can you please confirm that it does not cause any side effect? Rest of the fix looks ok. modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkin.java line 134: > 132: // that when it changes, we can appropriately add / > remove cells that may or may not > 133: // be required (because we remove all cells that are not > visible). > 134: registerChangeListener(getVirtualFlow().widthProperty(), > e -> tableView.requestLayout()); If this listener is removed, then is there a chance of reintroducing [JDK-8144500](https://bugs.openjdk.java.net/browse/JDK-8144500)? Unfortunately, there is no test added for that bug.. so it is difficult to catch regression, if any. modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java line 154: > 152: // that when it changes, we can appropriately add / > remove cells that may or may not > 153: // be required (because we remove all cells that are not > visible). > 154: registerChangeListener(getVirtualFlow().widthProperty(), > e -> treeTableView.requestLayout()); Same comment as in TableRowSkin regarding this listener. - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8271090: Missing API docs in scenegraph classes [v2]
On Mon, 25 Oct 2021 23:27:19 GMT, Kevin Rushforth wrote: > Looks good with a couple suggestions on `setScene`. We might want to also > file a follow-up javadoc bug so we can get rid of the javadocs for that > method altogether. I have filed - [JDK-8275910](https://bugs.openjdk.java.net/browse/JDK-8275910) for this. - PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271090: Missing API docs in scenegraph classes [v3]
> This PR fixes javadoc warnings primarily in javafx.graphics module along with > a remaining few in javafx.fxml, javafx.base and javafx.media modules. > > Note : > - The javadoc needs to be generated with the JDK 18 EA build. > - There are still few remaining warnings in these modules. The root cause is > different and they will be addressed under > [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: fix review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/650/files - new: https://git.openjdk.java.net/jfx/pull/650/files/9ff692db..e4d337c5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=650&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=650&range=01-02 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/650.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/650/head:pull/650 PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271091: Missing API docs in UI controls classes [v4]
> This PR fixes javadoc warnings in javafx.controls and javafx.web modules. > Note : > - The javadoc needs to be generated with the JDK 18 EA build. > - 2 javadoc warnings in javafx.controls TabPane class will be fixed under - > [JDK-8271085](https://bugs.openjdk.java.net/browse/JDK-8271085) > - There are still 20 javadoc warnings remaining in javafx.controls module and > 3 warnings remaining in javafx.web module. The root cause is different and > they will be addressed under > [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: fix review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/646/files - new: https://git.openjdk.java.net/jfx/pull/646/files/7cc2de52..b4d5dc4f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=646&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=646&range=02-03 Stats: 5 lines in 2 files changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/646.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/646/head:pull/646 PR: https://git.openjdk.java.net/jfx/pull/646
Re: RFR: 8271091: Missing API docs in UI controls classes [v3]
On Mon, 25 Oct 2021 22:53:53 GMT, Kevin Rushforth wrote: >> Ajit Ghaisas has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix review comments > > modules/javafx.controls/src/main/java/javafx/scene/control/PopupControl.java > line 179: > >> 177: * HTML element. Note that, like the HTML style attribute, this >> 178: * variable contains style properties and values and not the >> 179: * selector portion of a style rule. > > It would be good to add a sentence here with the information (which was > formerly in the getter) that a value of `null` is converted to the empty > string. Added. > modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line > 508: > >> 506: * The maximum height of the tabs in the TabPane. >> 507: * @return the maximum height of the tabs >> 508: */ > > In reverting this, you introduced a whitespace (indentation) change. Fixed. - PR: https://git.openjdk.java.net/jfx/pull/646
Re: RFR: 8271091: Missing API docs in UI controls classes [v3]
> This PR fixes javadoc warnings in javafx.controls and javafx.web modules. > Note : > - The javadoc needs to be generated with the JDK 18 EA build. > - 2 javadoc warnings in javafx.controls TabPane class will be fixed under - > [JDK-8271085](https://bugs.openjdk.java.net/browse/JDK-8271085) > - There are still 20 javadoc warnings remaining in javafx.controls module and > 3 warnings remaining in javafx.web module. The root cause is different and > they will be addressed under > [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: fix review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/646/files - new: https://git.openjdk.java.net/jfx/pull/646/files/38563835..7cc2de52 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=646&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=646&range=01-02 Stats: 124 lines in 8 files changed: 48 ins; 21 del; 55 mod Patch: https://git.openjdk.java.net/jfx/pull/646.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/646/head:pull/646 PR: https://git.openjdk.java.net/jfx/pull/646
Re: RFR: 8271091: Missing API docs in UI controls classes [v2]
On Fri, 22 Oct 2021 13:37:38 GMT, Kevin Rushforth wrote:
>> Ajit Ghaisas has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> javadoc minor corrections
>
> modules/javafx.controls/src/main/java/javafx/scene/control/DatePicker.java
> line 470:
>
>> 468: * {@code CssMetaData} of its superclasses.
>> 469: * @return the {@code CssMetaData}
>> 470: * @since JavaFX 8.0
>
> The class already has an `@since JavaFX 8.0` tag so this is unnecessary. It's
> also unrelated to this fix (and we would need a CSR for any changes to
> `@since` tags).
OK. I will remove this since tag.
> modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java
> line 160:
>
>> 158: /**
>> 159: * Focuses the item at the given index.
>> 160: * @param index the index of the item that needs to be focused
>
> Minor: I recommend removing "that needs" from this `@param`.
Done.
> modules/javafx.controls/src/main/java/javafx/scene/control/PopupControl.java
> line 201:
>
>> 199: * this specific {@code PopupControl}.
>> 200: * @return the {@code StringProperty} representing the CSS style
>> 201: */
>
> I recommend taking the information from the setter and copying it here. The
> docs can/should then be removed from both the setter and the getter. The
> first sentence should describe the property so does not need to start with
> "Gets the ...". You can add the information about `null` being turned into
> the empty string as a sentence in the Description. The `@defaultValue empty
> string` means that the information in the `@return` of the getter is
> unnecessary.
I could move the description from the setter to the javadoc of `getStyle()`
method as suggested. A `@return` is still needed for `getStyle()` as not to get
the javadoc warning.
> modules/javafx.controls/src/main/java/javafx/scene/control/SortEvent.java
> line 47:
>
>> 45: /**
>> 46: * Gets the default singleton {@code SortEvent}.
>> 47: * @param the type of control
>
> Can you also add this `@param` tag to the class description? I'm a little
> surprised that javadoc doesn't flag it as missing.
Added.
> modules/javafx.controls/src/main/java/javafx/scene/control/SortEvent.java
> line 59:
>
>> 57: /**
>> 58: * Constructs a new {@code Event} with the specified event source,
>> target
>> 59: * and type. If the source or target is set to {@code null}, it is
>> replaced
>
> That should be "a new `{@code SortEvent}` ...". Also, since there is no type
> parameter, you should remove "and type" (and replace the comma after source
> with "and").
Done.
> modules/javafx.controls/src/main/java/javafx/scene/control/SortEvent.java
> line 63:
>
>> 61: *
>> 62: * @param source the event source which sent the event
>> 63: * @param target the target of the scroll to operation
>
> That should be "the target of the event".
Changed.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 405:
>
>> 403: }
>> 404:
>> 405: public final DoubleProperty tabMaxWidthProperty() {
>
> The best practice for properties is to put the description on exactly one of
> the `xxx` private field or the `xxxProperty` method, and not on the setter or
> getter (unless there is a specific need to document something special in the
> setter or getter). There is a separate JBS issue,
> [JDK-8271085](https://bugs.openjdk.java.net/browse/JDK-8271085), tracking the
> fix for the `maxWidth` and `maxHeight` properties.
>
> You can either revert this change, in which case we'll need a new PR for that
> issue (it can be done later), or else modify this change to match the best
> practice as noted in JDK-8271085 and add that issue to this PR. The latter
> will require addressing the other methods in this file as noted in that JBS
> bug.
I prefer reverting this and handling it separately under a separate PR.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 501:
>
>> 499: }
>> 500:
>> 501: public final DoubleProperty tabMaxHeightProperty() {
>
> Same comment as above. This needs to be reverted or modified.
I prefer reverting this and handling it separately under a separate PR.
-
PR: https://git.openjdk.java.net/jfx/pull/646
Re: RFR: 8271090: Missing API docs in scenegraph classes [v2]
On Fri, 22 Oct 2021 15:00:58 GMT, Kevin Rushforth wrote:
>> Ajit Ghaisas has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8271090 - fix review comments
>
> modules/javafx.graphics/src/main/java/javafx/stage/Window.java line 781:
>
>> 779: * Sets the {@code Scene} of this {@code Window}.
>> 780: * @param value the {@code Scene} to be set
>> 781: */
>
> The javadoc tool automatically generates docs for this setter (you can see
> this from the generated docs without this change), including the property
> description, so this change will actually lose information. I'm guessing that
> the tool warns on this method because the setter is protected (rather than
> public). So we should probably file a bug against the javadoc tool. As for
> how to fix it, you can either suppress the warning (I'm not sure whether
> that's possible), or you can copy the information from the property method
> with a leading sentence that matches what javadoc _would_ generate.
Suppressing this warning is logical - but not possible.
As suggested, I have copied the property information and added the set method
description that was generated by javadoc tool before this fix.
-
PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271090: Missing API docs in scenegraph classes [v2]
On Fri, 22 Oct 2021 15:30:36 GMT, Kevin Rushforth wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/shape/Box.java line 91:
>>
>>> 89: * Default size of the {@code Box}.
>>> 90: */
>>> 91: public static final double DEFAULT_SIZE = 2;
>>
>> This field was exposed by mistake probably. The other shapes don't expose
>> theirs. I recommend to deprecate for removal.
>
> Agreed. That would need to be done in a follow-up issue, and with a CSR. So
> just revert this addition for this PR.
Reverted this change.
Filed [JDK-8275848](https://bugs.openjdk.java.net/browse/JDK-8275848) for
addressing this.
-
PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271090: Missing API docs in scenegraph classes [v2]
On Fri, 22 Oct 2021 14:37:14 GMT, Nir Lisker wrote:
>> Ajit Ghaisas has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8271090 - fix review comments
>
> modules/javafx.media/src/main/java/javafx/scene/media/Track.java line 85:
>
>> 83: /**
>> 84: * Gets the Map containing all known metadata for this
>> Track.
>> 85: * @return the Map containing all known metadata for
>> this Track
>
> We tend to use `{@code }` over ` `, but I don't think it matter.
I have just maintained the local convention used in this file.
-
PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271090: Missing API docs in scenegraph classes [v2]
> This PR fixes javadoc warnings primarily in javafx.graphics module along with > a remaining few in javafx.fxml, javafx.base and javafx.media modules. > > Note : > - The javadoc needs to be generated with the JDK 18 EA build. > - There are still few remaining warnings in these modules. The root cause is > different and they will be addressed under > [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: 8271090 - fix review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/650/files - new: https://git.openjdk.java.net/jfx/pull/650/files/239c0634..9ff692db Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=650&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=650&range=00-01 Stats: 38 lines in 6 files changed: 21 ins; 12 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/650.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/650/head:pull/650 PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8274669: Dialog sometimes ignores max height
On Sat, 2 Oct 2021 23:53:02 GMT, Marius Hanl wrote: > This PR fixes a visual glitch which may happen when showing a dialog. > When a max height is set and the pref height of the dialog content is bigger > the dialog starts to flicker between the max height and the pref height. > > This happens because in one case the height is not bound between the min and > the max height (-> max height is ignored). > - First layout pass: The dialog will incorrectly resize to a the pref height, > which is bigger than the max height > - Second layout pass: The dialog will correctly resize to the max height > - repeat > > The fix is to bound the height there as well (Fix in **layoutChildren()**). > Example where a red stackpane has a bigger pref height then the max height of > the dialog (see also example in ticket): > > https://user-images.githubusercontent.com/66004280/135734463-03b422f4-710d-4436-9715-c91bdb768d76.mp4 > > * only the max height test fails before and succeeds after. Marked as reviewed by aghaisas (Reviewer). In this case limiting the height between min and max makes sense. - PR: https://git.openjdk.java.net/jfx/pull/637
RFR: 8271090: Missing API docs in scenegraph classes
This PR fixes javadoc warnings primarily in javafx.graphics module along with a remaining few in javafx.fxml, javafx.base and javafx.media modules. Note : - The javadoc needs to be generated with the JDK 18 EA build. - There are still few remaining warnings in these modules. The root cause is different and they will be addressed under [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) - Commit messages: - 8271090 - fix javadoc warnings - base,media,fxml - 8271090 - fix javadoc warnings Changes: https://git.openjdk.java.net/jfx/pull/650/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=650&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8271090 Stats: 107 lines in 29 files changed: 74 ins; 0 del; 33 mod Patch: https://git.openjdk.java.net/jfx/pull/650.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/650/head:pull/650 PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271091: Missing API docs in UI controls classes [v2]
> This PR fixes javadoc warnings in javafx.controls and javafx.web modules. > Note : > - The javadoc needs to be generated with the JDK 18 EA build. > - There are still 20 javadoc warnings remaining in javafx.controls module and > 3 warnings remaining in javafx.web module. The root cause is different and > they will be addressed under > [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: javadoc minor corrections - Changes: - all: https://git.openjdk.java.net/jfx/pull/646/files - new: https://git.openjdk.java.net/jfx/pull/646/files/6f802004..38563835 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=646&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=646&range=00-01 Stats: 55 lines in 47 files changed: 0 ins; 0 del; 55 mod Patch: https://git.openjdk.java.net/jfx/pull/646.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/646/head:pull/646 PR: https://git.openjdk.java.net/jfx/pull/646
RFR: 8271091: Missing API docs in UI controls classes
This PR fixes javadoc warnings in javafx.controls and javafx.web modules. Note : - The javadoc needs to be generated with the JDK 18 EA build. - There are still 20 javadoc warnings remaining in javafx.controls module and 3 warnings remaining in javafx.web module. The root cause is different and they will be addressed under [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) - Commit messages: - 8271091 - fix javadoc warnings Changes: https://git.openjdk.java.net/jfx/pull/646/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=646&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8271091 Stats: 248 lines in 49 files changed: 114 ins; 11 del; 123 mod Patch: https://git.openjdk.java.net/jfx/pull/646.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/646/head:pull/646 PR: https://git.openjdk.java.net/jfx/pull/646
Re: RFR: 8275138: WebView: UserAgent string is empty for first request
On Thu, 14 Oct 2021 12:32:24 GMT, Kevin Rushforth wrote:
> The failure was caused by a change that was done in connection with the
> WebKit 610.2 update,
> [JDK-8259635](https://bugs.openjdk.java.net/browse/JDK-8259635). The
> `FrameLoaderClient::userAgent` function was changed in WebKit itself to be a
> const method in WebKit 610.2. We override that method in
> `FrameLoaderClientJava` to return the user agent string, which is done by
> reading the user agent from an instance of the `Page` class.
> `FrameLoaderClientJava` has a `m_page` field that is lazily initialized
> whenever it is needed. This lazy initialization can no longer be done from
> the `userAgent` method, since it is `const`, as can be seen from this change
> that was done as part of WebKit 610.2:
>
>
> ---
> a/modules/javafx.web/src/main/native/Source/WebKitLegacy/java/WebCoreSupport/FrameLoaderClientJava.cpp
> +++
> b/modules/javafx.web/src/main/native/Source/WebKitLegacy/java/WebCoreSupport/FrameLoaderClientJava.cpp
> -String FrameLoaderClientJava::userAgent(const URL&)
> +String FrameLoaderClientJava::userAgent(const URL&) const
> {
> -return page()->settings().userAgent();
> +if (!m_page)
> +return emptyString();
> +return m_page->settings().userAgent();
> }
>
>
> Formerly, if the `m_page` field was uninitialized,
> FrameLoaderClient::userAgent would have initialized it by the call to the
> `page()` function, but since it is now `const` we can't do that. This means
> that the user agent string will be empty until some other method is called
> that initializes the `m_page` field.
>
> The fix is to initialize that field when the `WebPage` is created. We can't
> do it in the constructor, since the needed reference to the `Page` class
> isn't yet available, so I added an `init` method that is called from
> `WebPage::twkInit`.
>
> I added a new unit test that fails without the fix and passes with the fix.
Marked as reviewed by aghaisas (Reviewer).
The fix looks fine. I built and tested on mac.
The new test fails without the fix and passes with it.
-
PR: https://git.openjdk.java.net/jfx/pull/643
Re: RFR: 8274137: TableView scrollbar/header misaligned when reloading data [v3]
On Wed, 6 Oct 2021 12:22:54 GMT, Robert Lichtenberger wrote: > Ist there anything left for me to do? IIRC someone must now /sponsor this > change and then I can /integrate it, right? This PR needs two review approvals; currently it has one. We shall wait for @kevinrushforth to review and approve it. Once he does it, you can /integrate it and one of us will /sponsor it. - PR: https://git.openjdk.java.net/jfx/pull/629
Re: RFR: 8274137: TableView scrollbar/header misaligned when reloading data [v3]
On Mon, 27 Sep 2021 05:27:25 GMT, Robert Lichtenberger wrote: >> This PR fixes JDK-8274137 by removing the optimization from updateHbar() >> that will no-op the method in case the VirtualFlow is invisible or currently >> has no scene. >> Since changes to the hBar's value can happen even if the VirtualFlow is not >> currently visible, the synchronisation between hBar and clipX must happen >> all the time. >> >> A test agains VirtualFlow has been added that will fail before the change >> and pass afterwards. > > Robert Lichtenberger has updated the pull request incrementally with one > additional commit since the last revision: > > 8274137: TableView scrollbar/header misaligned when reloading data > > Remove @Override annotations unrelated to fix. Marked as reviewed by aghaisas (Reviewer). Fix is fine! - PR: https://git.openjdk.java.net/jfx/pull/629
