Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 08:49:44 GMT, drmarmac wrote: >> This PR removes potentially incorrect usages of Stream.peek(). >> The changed code should be covered by the tests that are already present. > > drmarmac has updated the pull request incrementally with two additional > commits since the last revision: > > - Preallocate in SelectedIndicesList.set() > - Remove unused import Thank you for clarifications! Does not look like this is called from a loop, so whatever inefficiency we have is likely to be ok. - Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1430#pullrequestreview-1967473815
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 22:21:32 GMT, drmarmac wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java >> line 166: >> >>> 164: sm.startAtomic(); >>> 165: >>> 166: final List removed = new >>> ArrayList<>(c.getRemovedSize()); >> >> I wonder if we should add a check for 0 size here to bypass all this code >> and unnecessary object allocations if nothing is removed (same for added) > > We certainly could, or maybe use wasRemoved(), but I doubt there will be much > impact. I guess it's preferred to keep changes unrelated to the issue > minimal, so I'd leave it as it is if everyone's ok with that. These short circuiting operations tend to be worth it only if it's a critical path. The GC will collect the allocations very efficiently these days. I didn't check what this code segment is used for, but unless it's looped somewhere, I doubt there will by any change. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1543793491
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 17:53:14 GMT, Andy Goryachev wrote: >> drmarmac has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Preallocate in SelectedIndicesList.set() >> - Remove unused import > > modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java > line 166: > >> 164: sm.startAtomic(); >> 165: >> 166: final List removed = new >> ArrayList<>(c.getRemovedSize()); > > I wonder if we should add a check for 0 size here to bypass all this code and > unnecessary object allocations if nothing is removed (same for added) We certainly could, or maybe use wasRemoved(), but I doubt there will be much impact. I guess it's preferred to keep changes unrelated to the issue minimal, so I'd leave it as it is if everyone's ok with that. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1543790719
Re: RFR: 8092102: Labeled: truncated property [v8]
On Thu, 28 Mar 2024 21:44:49 GMT, Andy Goryachev wrote: >> Adds **Labeled.textTruncated** property which indicates when the text is >> visually truncated (and the ellipsis string is inserted) in order to fit the >> available width. >> >> The new property reacts to changes in the following properties: >> - ellipsisString >> - font >> - height >> - text >> - width >> - wrapText >> >> I don't think it's worth creating a headful test (headless won't work) due >> to relative simplicity of the code. >> >> **Alternative** >> >> The desired functionality can be just as easily achieved on an application >> level, by adding a similar property to a subclass. What is the benefit of >> adding this functionality to the core? >> >> UPDATE 2024/03/07: turns out Labeled in a TableView (in a TreeTableView as >> well) lives by different rules (TableCellSkinBase:152, >> TreeTableCellSkin:126). The consequence of this is that the new >> functionality **cannot** be fully implemented with the public APIs alone. >> >> **See Also** >> >> * [JDK-8327483](https://bugs.openjdk.org/browse/JDK-8327483) TreeView: Allow >> for tooltip when cell text is truncated >> * [JDK-8205211](https://bugs.openjdk.org/browse/JDK-8205211) Ability to show >> Tooltip only when text is shown with ellipsis (...) > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > add exports unit tests added, please re-review. - PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-2026206381
Re: RFR: 8092102: Labeled: truncated property [v8]
> Adds **Labeled.textTruncated** property which indicates when the text is > visually truncated (and the ellipsis string is inserted) in order to fit the > available width. > > The new property reacts to changes in the following properties: > - ellipsisString > - font > - height > - text > - width > - wrapText > > I don't think it's worth creating a headful test (headless won't work) due to > relative simplicity of the code. > > **Alternative** > > The desired functionality can be just as easily achieved on an application > level, by adding a similar property to a subclass. What is the benefit of > adding this functionality to the core? > > UPDATE 2024/03/07: turns out Labeled in a TableView (in a TreeTableView as > well) lives by different rules (TableCellSkinBase:152, > TreeTableCellSkin:126). The consequence of this is that the new > functionality **cannot** be fully implemented with the public APIs alone. > > **See Also** > > * [JDK-8327483](https://bugs.openjdk.org/browse/JDK-8327483) TreeView: Allow > for tooltip when cell text is truncated > * [JDK-8205211](https://bugs.openjdk.org/browse/JDK-8205211) Ability to show > Tooltip only when text is shown with ellipsis (...) Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: add exports - Changes: - all: https://git.openjdk.org/jfx/pull/1389/files - new: https://git.openjdk.org/jfx/pull/1389/files/45704b92..f98ff907 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1389=07 - incr: https://webrevs.openjdk.org/?repo=jfx=1389=06-07 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1389.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1389/head:pull/1389 PR: https://git.openjdk.org/jfx/pull/1389
Integrated: 8316372: Monkey Tester Application Part 3
On Mon, 18 Mar 2024 21:02:54 GMT, Andy Goryachev wrote: > Further changes to the MonkeyTester application: > > - remember split pane divider ✔ > - use 'private' instead of 'protected' in many cases ✔ > - added more scripts to the 'writing systems' text sample ✔ > - added RTL window control menu ✔ > - added embedded swing/fx in tools ✔ > - added copy popup menu in clipboard viewer ✔ > - added the custom css field to the css playground tool ✔ > - added many new pages ✔ > - split XYChartPage into separate pages ✔ > - switched to use property sheets (some choices might be incomplete) ✔ > > https://github.com/andy-goryachev-oracle/jfx/blob/8316372.monkey/tests/manual/monkey/README.md > > These are all the changes I could make in this test sprint, other > improvements will go to the follow-up ticket > https://bugs.openjdk.org/browse/JDK-8328828 This pull request has now been integrated. Changeset: bf237329 Author:Andy Goryachev URL: https://git.openjdk.org/jfx/commit/bf237329bf2d95bfdf248afc30d6df47d470d99a Stats: 10395 lines in 104 files changed: 6013 ins; 3418 del; 964 mod 8316372: Monkey Tester Application Part 3 Reviewed-by: kcr, kpk - PR: https://git.openjdk.org/jfx/pull/1406
Re: RFR: 8323511 Scrollbar Click jumps inconsistent amount of pixels [v2]
On Mon, 15 Jan 2024 08:31:59 GMT, Florian Kirmaier wrote: >> As seen in the unit test of the PR, when we click on the area above/below >> the scrollbar the position jumps - but the jump is now not always consistent. >> In the current version on the last cell - the UI always jumps to the top. In >> the other cases, the assumed default cell height is used. >> >> With this PR, always the default cell height is used, to determine how much >> is scrolled. >> This makes the behavior more consistent. >> >> Especially from the unit-test, it's clear that with this PR the behavior is >> much more consistent. >> >> This is also related to the following PR: >> https://github.com/openjdk/jfx/pull/1194 > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8323511 > reverted accidental indentation chang Intuitively, the added test seems to be the right thing to do. It indeed fails before and succeeds after the patch. However, I'm not sure about the implementation. The suggested patch changes the conceptual idea of `VirtualFlow.scrollTo(int index)` where a negative index is not specified (this is probably what @Maran23 asked at https://github.com/openjdk/jfx/pull/1326#discussion_r1530233902 . The way the `scrollTo(int index)` is modified doesn't sound right to me. In case of the test, where the function is called with `index = -1`, it will first try to obtain the cell at index -2 (which will always return false), and then it will try to obtain the cell at index 0 (the one we need to have indeed), and then scroll X pixels where X is the height of the cell at index -1. In the test, all cells except the one at 0 have height 100, so the cell at -1 will have 100 as well, so it will use that. This seems a complex way to deal with the issue. It would need a clear definition of "default size" and it changes the implicit assumption that scrollTo(index) should be called with a valid index (which does not have to be visible in the current viewport). I believe the first thing that needs to be done, is to extend the "specification" that is currently in comments in the VirtualScrollBar: /* * Scroll one cell further in the direction the user has clicked if only one cell is shown. * Otherwise, a click on the trough would have no effect when cell height > viewport height. */ What does this mean if there is no cell further in the direction the user has clicked? Should we them scroll to the top of the current cell (current behavior), or should we use a more gradual scroll (which is the new behavior, which I believe is better indeed)? If the latter is the preferred case, this looks to a behavior that is more similar to the Event that is received when the mousewheel is used (and which invokes `VirtualFlow.scrollPixels(double delta)`) - PR Comment: https://git.openjdk.org/jfx/pull/1326#issuecomment-2025937722
Re: RFR: 8316372: Monkey Tester Application Part 3 [v7]
On Mon, 25 Mar 2024 22:36:48 GMT, Andy Goryachev wrote: >> Further changes to the MonkeyTester application: >> >> - remember split pane divider ✔ >> - use 'private' instead of 'protected' in many cases ✔ >> - added more scripts to the 'writing systems' text sample ✔ >> - added RTL window control menu ✔ >> - added embedded swing/fx in tools ✔ >> - added copy popup menu in clipboard viewer ✔ >> - added the custom css field to the css playground tool ✔ >> - added many new pages ✔ >> - split XYChartPage into separate pages ✔ >> - switched to use property sheets (some choices might be incomplete) ✔ >> >> https://github.com/andy-goryachev-oracle/jfx/blob/8316372.monkey/tests/manual/monkey/README.md >> >> These are all the changes I could make in this test sprint, other >> improvements will go to the follow-up ticket >> https://bugs.openjdk.org/browse/JDK-8328828 > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > spinner Looks good. I ran through several of the new and existing pages. - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1406#pullrequestreview-1967067101
Re: RFR: 8328577: Toolbar's overflow button overlaps the items [v2]
On Thu, 28 Mar 2024 18:53:00 GMT, eduardsdv wrote: >> This change fixes the calculation of which nodes go to the toolbar and which >> go to the overflow menu. >> It is now determined before the nodes are removed from the scene graph. >> This is important because the values returned by >> ``Node.prefWidth(..)``/``Node.prefHeight(..)`` may depend on whether the >> node is added to the scene graph. >> >> Furthermore I corrected the ``hasOveflow`` value passed to the >> ``organizeOverflow(double, boolean)`` in ``correctOverflow(double)``. > > eduardsdv has updated the pull request incrementally with one additional > commit since the last revision: > > JDK-8328577: Enforce the overflowed nodes are added to the scene even if > the overflow menu is not visible Thanks for finding it. I have reworked my solution. It is indeed necessary to add the overflow nodes to the scene of the popup and reapply the CSS so that the ``prefWidth(..)``/``prefHeight(..)`` return correct values, even if the nodes are no longer in the toolbar. I will try to create a unit test that ensures the bug does not occur again. - PR Comment: https://git.openjdk.org/jfx/pull/1434#issuecomment-2025915810
Re: RFR: 8092102: Labeled: truncated property [v7]
> Adds **Labeled.textTruncated** property which indicates when the text is > visually truncated (and the ellipsis string is inserted) in order to fit the > available width. > > The new property reacts to changes in the following properties: > - ellipsisString > - font > - height > - text > - width > - wrapText > > I don't think it's worth creating a headful test (headless won't work) due to > relative simplicity of the code. > > **Alternative** > > The desired functionality can be just as easily achieved on an application > level, by adding a similar property to a subclass. What is the benefit of > adding this functionality to the core? > > UPDATE 2024/03/07: turns out Labeled in a TableView (in a TreeTableView as > well) lives by different rules (TableCellSkinBase:152, > TreeTableCellSkin:126). The consequence of this is that the new > functionality **cannot** be fully implemented with the public APIs alone. > > **See Also** > > * [JDK-8327483](https://bugs.openjdk.org/browse/JDK-8327483) TreeView: Allow > for tooltip when cell text is truncated > * [JDK-8205211](https://bugs.openjdk.org/browse/JDK-8205211) Ability to show > Tooltip only when text is shown with ellipsis (...) Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits: - added unit tests - Merge remote-tracking branch 'origin/master' into 8092102.truncated - test - Merge remote-tracking branch 'origin/master' into 8092102.truncated - Merge branch 'master' into 8092102.truncated - labeled helper - handle tree/table view cells - Merge remote-tracking branch 'origin/master' into 8092102.truncated - review comments - Merge remote-tracking branch 'origin/master' into 8092102.truncated - ... and 1 more: https://git.openjdk.org/jfx/compare/7a4d2976...45704b92 - Changes: https://git.openjdk.org/jfx/pull/1389/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1389=06 Stats: 306 lines in 7 files changed: 276 ins; 19 del; 11 mod Patch: https://git.openjdk.org/jfx/pull/1389.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1389/head:pull/1389 PR: https://git.openjdk.org/jfx/pull/1389
Re: RFR: 8328577: Toolbar's overflow button overlaps the items [v2]
> This change fixes the calculation of which nodes go to the toolbar and which > go to the overflow menu. > It is now determined before the nodes are removed from the scene graph. > This is important because the values returned by > ``Node.prefWidth(..)``/``Node.prefHeight(..)`` may depend on whether the node > is added to the scene graph. > > Furthermore I corrected the ``hasOveflow`` value passed to the > ``organizeOverflow(double, boolean)`` in ``correctOverflow(double)``. eduardsdv has updated the pull request incrementally with one additional commit since the last revision: JDK-8328577: Enforce the overflowed nodes are added to the scene even if the overflow menu is not visible - Changes: - all: https://git.openjdk.org/jfx/pull/1434/files - new: https://git.openjdk.org/jfx/pull/1434/files/b7e25d53..82994291 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1434=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1434=00-01 Stats: 69 lines in 1 file changed: 21 ins; 22 del; 26 mod Patch: https://git.openjdk.org/jfx/pull/1434.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1434/head:pull/1434 PR: https://git.openjdk.org/jfx/pull/1434
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 08:49:44 GMT, drmarmac wrote: >> This PR removes potentially incorrect usages of Stream.peek(). >> The changed code should be covered by the tests that are already present. > > drmarmac has updated the pull request incrementally with two additional > commits since the last revision: > > - Preallocate in SelectedIndicesList.set() > - Remove unused import modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java line 166: > 164: sm.startAtomic(); > 165: > 166: final List removed = new > ArrayList<>(c.getRemovedSize()); I wonder if we should add a check for 0 size here to bypass all this code if nothing is removed (same for added) - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1543395945
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 15:48:50 GMT, Andy Goryachev wrote: > a minor question: would it be much easier to understand if the code was > written in conventional procedural style? a straightforward `for` loop? Even if it were (and I'm not sure it would be), it would be a more intrusive change than is required by the fix, so I wouldn't recommend it. - PR Comment: https://git.openjdk.org/jfx/pull/1430#issuecomment-2025771068
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 08:49:44 GMT, drmarmac wrote: >> This PR removes potentially incorrect usages of Stream.peek(). >> The changed code should be covered by the tests that are already present. > > drmarmac has updated the pull request incrementally with two additional > commits since the last revision: > > - Preallocate in SelectedIndicesList.set() > - Remove unused import Looks good. Should be good regarding performance as well. - Marked as reviewed by mhanl (Committer). PR Review: https://git.openjdk.org/jfx/pull/1430#pullrequestreview-1966854737
Integrated: 8328754: Fix missing @Overrides in test
On Fri, 22 Mar 2024 15:55:35 GMT, Andy Goryachev wrote: > Fixing missing @ OVERRIDES in tests. > > This is still a trivial change since all the spots are identified by the IDE. This pull request has now been integrated. Changeset: 7a4d2976 Author:Andy Goryachev URL: https://git.openjdk.org/jfx/commit/7a4d2976a245053399ad760930c0e6fb13a67637 Stats: 96 lines in 27 files changed: 69 ins; 0 del; 27 mod 8328754: Fix missing @Overrides in test Reviewed-by: aghaisas, kpk - PR: https://git.openjdk.org/jfx/pull/1427
Re: RFR: 8328754: Fix missing @Overrides in test [v2]
On Thu, 28 Mar 2024 16:27:44 GMT, Ajit Ghaisas wrote: >> Andy Goryachev has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains three additional >> commits since the last revision: >> >> - more files >> - Merge branch 'master' into 8328754.overrides.test >> - 8328754: Fix missing @Overrides in test > > Marked as reviewed by aghaisas (Reviewer). thank you @aghaisas @karthikpandelu - PR Comment: https://git.openjdk.org/jfx/pull/1427#issuecomment-2025710520
Re: RFR: 8328754: Fix missing @Overrides in test [v2]
On Thu, 28 Mar 2024 14:54:47 GMT, Andy Goryachev wrote: >> Fixing missing @ OVERRIDES in tests. >> >> This is still a trivial change since all the spots are identified by the IDE. > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - more files > - Merge branch 'master' into 8328754.overrides.test > - 8328754: Fix missing @Overrides in test Marked as reviewed by kpk (Committer). - PR Review: https://git.openjdk.org/jfx/pull/1427#pullrequestreview-1966699088
Re: RFR: 8328754: Fix missing @Overrides in test [v2]
On Thu, 28 Mar 2024 14:54:47 GMT, Andy Goryachev wrote: >> Fixing missing @ OVERRIDES in tests. >> >> This is still a trivial change since all the spots are identified by the IDE. > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - more files > - Merge branch 'master' into 8328754.overrides.test > - 8328754: Fix missing @Overrides in test Marked as reviewed by aghaisas (Reviewer). - PR Review: https://git.openjdk.org/jfx/pull/1427#pullrequestreview-1966693901
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 08:49:44 GMT, drmarmac wrote: >> This PR removes potentially incorrect usages of Stream.peek(). >> The changed code should be covered by the tests that are already present. > > drmarmac has updated the pull request incrementally with two additional > commits since the last revision: > > - Preallocate in SelectedIndicesList.set() > - Remove unused import modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java line 40: > 38: import java.util.List; > 39: import java.util.function.IntPredicate; > 40: import java.util.stream.Collectors; and thank you for removing the unused import ... JDK-8289845 - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1543225967
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 08:49:44 GMT, drmarmac wrote: >> This PR removes potentially incorrect usages of Stream.peek(). >> The changed code should be covered by the tests that are already present. > > drmarmac has updated the pull request incrementally with two additional > commits since the last revision: > > - Preallocate in SelectedIndicesList.set() > - Remove unused import a minor question: would it be much easier to understand if the code was written in conventional procedural style? a straightforward `for` loop? - PR Comment: https://git.openjdk.org/jfx/pull/1430#issuecomment-2025544489
Re: RFR: 8328577: Toolbar's overflow button overlaps the items
On Thu, 28 Mar 2024 11:31:14 GMT, eduardsdv wrote: > This change fixes the calculation of which nodes go to the toolbar and which > go to the overflow menu. > It is now determined before the nodes are removed from the scene graph. > This is important because the values returned by > ``Node.prefWidth(..)``/``Node.prefHeight(..)`` may depend on whether the node > is added to the scene graph. > > Furthermore I corrected the ``hasOveflow`` value passed to the > ``organizeOverflow(double, boolean)`` in ``correctOverflow(double)``. The proposed solution still does not work - using the sample code in the ticket, try resizing back and forth around the button with -- text:  a couple of notes: 1. do you think we should add a test? 2. should there be a call to applyCss() somewhere? - PR Comment: https://git.openjdk.org/jfx/pull/1434#issuecomment-2025507990 PR Comment: https://git.openjdk.org/jfx/pull/1434#issuecomment-2025510592
Re: RFR: 8328754: Fix missing @Overrides in test [v2]
On Thu, 28 Mar 2024 12:12:07 GMT, Karthik P K wrote: >> Andy Goryachev has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains three additional >> commits since the last revision: >> >> - more files >> - Merge branch 'master' into 8328754.overrides.test >> - 8328754: Fix missing @Overrides in test > > Changes looks good. > Found few file where `@Override` can be added for `start` method. > > 1. `BigGlyphIDTest.java` > 2. `INVISIBLE_GLYPH_IDTest.java` > 3. `LoadFonts.java` good catch, thank you @karthikpandelu ! please re-review - PR Comment: https://git.openjdk.org/jfx/pull/1427#issuecomment-2025420842
Re: RFR: 8328754: Fix missing @Overrides in test [v2]
> Fixing missing @ OVERRIDES in tests. > > This is still a trivial change since all the spots are identified by the IDE. Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - more files - Merge branch 'master' into 8328754.overrides.test - 8328754: Fix missing @Overrides in test - Changes: - all: https://git.openjdk.org/jfx/pull/1427/files - new: https://git.openjdk.org/jfx/pull/1427/files/4b7ac579..20f2b09c Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1427=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1427=00-01 Stats: 1109 lines in 114 files changed: 852 ins; 21 del; 236 mod Patch: https://git.openjdk.org/jfx/pull/1427.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1427/head:pull/1427 PR: https://git.openjdk.org/jfx/pull/1427
Integrated: 8328811: Fix missing @Overrides in demos
On Fri, 22 Mar 2024 15:54:42 GMT, Andy Goryachev wrote: > Fixing missing @Overrides in demo apps. > > This is still a trivial change since all the spots are identified by the IDE. This pull request has now been integrated. Changeset: 9ca8e51e Author:Andy Goryachev URL: https://git.openjdk.org/jfx/commit/9ca8e51ed3cae9d6381c42fd6e39316150b40cbc Stats: 530 lines in 82 files changed: 442 ins; 0 del; 88 mod 8328811: Fix missing @Overrides in demos Reviewed-by: aghaisas - PR: https://git.openjdk.org/jfx/pull/1426
Integrated: 8307980: Rotate Transformation never invalidates inverseCache
On Wed, 6 Mar 2024 15:51:16 GMT, Lukasz Kostyra wrote: > Fixed as described in the issue + added tests to check for this scenario with > all Transform sub classes. Since the test scenario slightly exceeding the > regular parametrized testing of `TransformOperationsTest` I decided to > implement the tests in a separate class. > > I didn't manage to reproduce the issue with other Transform sub classes, so > it seems to only affect `Rotate`. Also checked by looking at the code, only > `Rotate` was affected by this bug. As such, without 08ba284 only > `testTransformInverseCache_Rotate` fails, while others succeed. With the fix, > all tests pass. > > Ran the whole test suite afterwards and didn't notice any changes to test > results after introducing the fix. This pull request has now been integrated. Changeset: b3f5a789 Author:Lukasz Kostyra URL: https://git.openjdk.org/jfx/commit/b3f5a7896830ae3fa9abf2c684f6b9279f4b926b Stats: 119 lines in 2 files changed: 119 ins; 0 del; 0 mod 8307980: Rotate Transformation never invalidates inverseCache Reviewed-by: kcr - PR: https://git.openjdk.org/jfx/pull/1392
Re: RFR: 8325591: [Mac] DRAG_DONE reports null transferMode when destination is external
On Fri, 16 Feb 2024 22:35:49 GMT, Martin Fox wrote: > At the end of a drag operation the Mac Glass code sends out a DRAG_DONE event > using the operation mask tracked in the GlassDragSource to determine the > final transfer mode. That mask is only updated when a window in the JavaFX > app is the drop destination. If the drag moves to an external destination the > mask is set to NONE. If the drag terminates in the external destination that > NONE forms the basis of the transfer mode sent via the DRAG_DONE event. > > At the very end of the drag the OS calls the NSDraggingSource > (GlassDraggingSource) with the final drag operation. This PR issues the > DRAG_DONE from that callback so it can get the final transfer mode correct > for both internal and external destinations. LGTM - Marked as reviewed by lkostyra (Committer). PR Review: https://git.openjdk.org/jfx/pull/1371#pullrequestreview-1966127796
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 08:49:44 GMT, drmarmac wrote: >> This PR removes potentially incorrect usages of Stream.peek(). >> The changed code should be covered by the tests that are already present. > > drmarmac has updated the pull request incrementally with two additional > commits since the last revision: > > - Preallocate in SelectedIndicesList.set() > - Remove unused import Marked as reviewed by kpk (Committer). - PR Review: https://git.openjdk.org/jfx/pull/1430#pullrequestreview-1966109923
Re: RFR: 8307980: Rotate Transformation never invalidates inverseCache [v3]
On Thu, 28 Mar 2024 11:23:02 GMT, Lukasz Kostyra wrote: >> Fixed as described in the issue + added tests to check for this scenario >> with all Transform sub classes. Since the test scenario slightly exceeding >> the regular parametrized testing of `TransformOperationsTest` I decided to >> implement the tests in a separate class. >> >> I didn't manage to reproduce the issue with other Transform sub classes, so >> it seems to only affect `Rotate`. Also checked by looking at the code, only >> `Rotate` was affected by this bug. As such, without 08ba284 only >> `testTransformInverseCache_Rotate` fails, while others succeed. With the >> fix, all tests pass. >> >> Ran the whole test suite afterwards and didn't notice any changes to test >> results after introducing the fix. > > Lukasz Kostyra has updated the pull request incrementally with one additional > commit since the last revision: > > Add missing EOF newline Marked as reviewed by kcr (Lead). - PR Review: https://git.openjdk.org/jfx/pull/1392#pullrequestreview-1966034023
Re: RFR: 8328754: Fix missing @Overrides in test
On Fri, 22 Mar 2024 15:55:35 GMT, Andy Goryachev wrote: > Fixing missing @ OVERRIDES in tests. > > This is still a trivial change since all the spots are identified by the IDE. Changes looks good. Found few file where `@Override` can be added for `start` method. 1. `BigGlyphIDTest.java` 2. `INVISIBLE_GLYPH_IDTest.java` 3. `LoadFonts.java` - PR Review: https://git.openjdk.org/jfx/pull/1427#pullrequestreview-1965991395
Re: RFR: 8328754: Fix missing @Overrides in test
On Fri, 22 Mar 2024 15:55:35 GMT, Andy Goryachev wrote: > Fixing missing @ OVERRIDES in tests. > > This is still a trivial change since all the spots are identified by the IDE. Looks good. - Marked as reviewed by aghaisas (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1427#pullrequestreview-1965863547
RFR: 8328577: Toolbar's overflow button overlaps the items
This change fixes the calculation of which nodes go to the toolbar and which go to the overflow menu. It is now determined before the nodes are removed from the scene graph. This is important because the values returned by ``Node.prefWidth(..)``/``Node.prefHeight(..)`` may depend on whether the node is added to the scene graph. Furthermore I corrected the ``hasOveflow`` value passed to the ``organizeOverflow(double, boolean)`` in ``correctOverflow(double)``. - Commit messages: - JDK-8328577: Toolbar's overflow button overlaps the items Changes: https://git.openjdk.org/jfx/pull/1434/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1434=00 Issue: https://bugs.openjdk.org/browse/JDK-8328577 Stats: 27 lines in 1 file changed: 19 ins; 3 del; 5 mod Patch: https://git.openjdk.org/jfx/pull/1434.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1434/head:pull/1434 PR: https://git.openjdk.org/jfx/pull/1434
Re: RFR: 8328811: Fix missing @Overrides in demos
On Fri, 22 Mar 2024 15:54:42 GMT, Andy Goryachev wrote: > Fixing missing @Overrides in demo apps. > > This is still a trivial change since all the spots are identified by the IDE. Looks good. - Marked as reviewed by aghaisas (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1426#pullrequestreview-1965855717
Re: RFR: 8307980: Rotate Transformation never invalidates inverseCache [v3]
> Fixed as described in the issue + added tests to check for this scenario with > all Transform sub classes. Since the test scenario slightly exceeding the > regular parametrized testing of `TransformOperationsTest` I decided to > implement the tests in a separate class. > > I didn't manage to reproduce the issue with other Transform sub classes, so > it seems to only affect `Rotate`. Also checked by looking at the code, only > `Rotate` was affected by this bug. As such, without 08ba284 only > `testTransformInverseCache_Rotate` fails, while others succeed. With the fix, > all tests pass. > > Ran the whole test suite afterwards and didn't notice any changes to test > results after introducing the fix. Lukasz Kostyra has updated the pull request incrementally with one additional commit since the last revision: Add missing EOF newline - Changes: - all: https://git.openjdk.org/jfx/pull/1392/files - new: https://git.openjdk.org/jfx/pull/1392/files/cd8fdde8..17bfefc0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1392=02 - incr: https://webrevs.openjdk.org/?repo=jfx=1392=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1392.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1392/head:pull/1392 PR: https://git.openjdk.org/jfx/pull/1392
Re: RFR: 8324939: Editable TableView loses focus after commit
On Wed, 20 Mar 2024 10:55:56 GMT, Jose Pereda wrote: > This PR fixes the issue that after committing an edit on a > ListView/TreeView/TableView/TreeTableView control, the control might lose the > focus unexpectedly. > > For that, it refactors the > `ControlUtils::requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild` method, > in order to check if the control (`ListView`, `TreeView`, `TableView`, > `TreeTableView`) should request the focus _before_ the actual focus owner > (which could be the control added to the cell to edit its content, like a > `TextField`) is removed from the cell, so the `Control::requestFocus` call, > if needed, can be still invoked after the edit commit is done (as it was done > before). > > By adding `ControlUtils::controlShouldRequestFocusIfCurrentFocusOwnerIsChild` > the `Cell::commitEdit` implementations can now query if the control should > have the focus, after `super.commitEdit(newValue);` but before firing the > `CellEditEvent` and calling `updateItem()`, and if the result is true, then > request focus after the edit commit ends (like it was done before). > > Two new tests per control have been included, to verify that the focus > remains at the control, one for edit cancel (this passes before and after the > proposed changes), one for edit commit (this fails before and passes after > including the proposed fix). modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeViewTest.java line 2362: > 2360: assertTrue(treeView.isFocused()); > 2361: > 2362: VirtualFlowTestUtils.BLOCK_STAGE_LOADER_DISPOSE = true; This should not be needed since you are creating a `stageLoader` above. This seems to be only used when no `stageLoader` was created before - PR Review Comment: https://git.openjdk.org/jfx/pull/1411#discussion_r1542737994
Re: RFR: 8328746: Remove unused imports in demo apps
On Thu, 21 Mar 2024 21:50:37 GMT, Andy Goryachev wrote: > Using Eclipse IDE to remove unused imports in **demo apps** (3D, Ensemble, > etc.) and update the copyright year to 2024. Using wildcard for more than 10 > static imports. > > > -- > > This is a trivial change (though fairly large), 1 reviewer is probably enough. Leaving it unspecified would be ok but some kind of soft recommendation could also help in my opinion. So there will be a direction for someone who is looking for a recommended way to add imports and have consistency class files. > > static imports (sorted alphabetically) > > one blank line > > ordinary imports (sorted alphabetically) > > No wildcard imports (I favor an exception for static imports). > > +1 for something like this. +1 on this and no wildcard imports other than static imports. - PR Comment: https://git.openjdk.org/jfx/pull/1420#issuecomment-2024715556
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Thu, 28 Mar 2024 06:07:24 GMT, Karthik P K wrote: >> You don't need to return a list, you create it ahead of time like was done >> in line 167 >> >> List indices = new ArrayList<>(); >> >> and the add the elements in `forEach`. > >> Why do the double-iteration pattern here and not do the `peek` operation in >> a `forEach` like in the other 2 places? > > Yes, if the usage of `forEach` in previous 2 places are correct then I would > like to see similar change here as well. I changed this to the pre-allocated pattern like in the other places. An initial capacity of `indices.length + 1` optimizes the worst case (all given indices need to be set) which probably occurs quite often in real-world usage. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1542535713
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
On Thu, 28 Mar 2024 08:49:44 GMT, drmarmac wrote: >> This PR removes potentially incorrect usages of Stream.peek(). >> The changed code should be covered by the tests that are already present. > > drmarmac has updated the pull request incrementally with two additional > commits since the last revision: > > - Preallocate in SelectedIndicesList.set() > - Remove unused import I also removed a now unused import in ControlUtils.java. - PR Comment: https://git.openjdk.org/jfx/pull/1430#issuecomment-2024697576
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]
> This PR removes potentially incorrect usages of Stream.peek(). > The changed code should be covered by the tests that are already present. drmarmac has updated the pull request incrementally with two additional commits since the last revision: - Preallocate in SelectedIndicesList.set() - Remove unused import - Changes: - all: https://git.openjdk.org/jfx/pull/1430/files - new: https://git.openjdk.org/jfx/pull/1430/files/1b285b5b..aed8e7e1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1430=02 - incr: https://webrevs.openjdk.org/?repo=jfx=1430=01-02 Stats: 9 lines in 2 files changed: 3 ins; 2 del; 4 mod Patch: https://git.openjdk.org/jfx/pull/1430.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1430/head:pull/1430 PR: https://git.openjdk.org/jfx/pull/1430
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Wed, 27 Mar 2024 23:21:34 GMT, Nir Lisker wrote: >> `forEach` is void, so we can not return a list afterwards. > > You don't need to return a list, you create it ahead of time like was done in > line 167 > > List indices = new ArrayList<>(); > > and the add the elements in `forEach`. > Why do the double-iteration pattern here and not do the `peek` operation in a > `forEach` like in the other 2 places? Yes, if the usage of `forEach` in previous 2 places are correct then I would like to see similar change here as well. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1542358972
Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]
On Wed, 27 Mar 2024 23:24:51 GMT, Marius Hanl wrote: >>> In the java.util.stream package >>> [docs](https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/package-summary.html#SideEffects) >>> it is mentioned that `forEach()` method operates only via side-effects. So >>> do you think we should avoid using `forEach()` here and iterate the >>> generated list separately to clear selected index? >> >> `forEach` is used correctly here. From the docs: >>> With the exception of terminal operations >>> [forEach](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Stream.html#forEach(java.util.function.Consumer)) >>> and >>> [forEachOrdered](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Stream.html#forEachOrdered(java.util.function.Consumer)), >>> side-effects of behavioral parameters may not always be executed when the >>> stream implementation can optimize away the execution of behavioral >>> parameters without affecting the result of the computation. >> >>> Another idea is to use `toList()`, which is a very efficient operation and >>> then iterate over it. >> >> That's still 2 iterations. If the code is not performance-critical it >> doesn't matter. > >> That's still 2 iterations. > > Yes, but one advantage here: > We currently do `final List removed = new > ArrayList<>(c.getRemovedSize());`, > where we allocate a list with a size, that is probably too big since we > filter the removed items. > So with `toList`, we at least get back a list with the correct size. > But true, that we technically iterate twice then. It probably does not matter > too much. > `forEach` is used correctly here. From the docs: > > > With the exception of terminal operations > > [forEach](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Stream.html#forEach(java.util.function.Consumer)) > > and > > [forEachOrdered](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Stream.html#forEachOrdered(java.util.function.Consumer)), > > side-effects of behavioral parameters may not always be executed when the > > stream implementation can optimize away the execution of behavioral > > parameters without affecting the result of the computation. > Thanks for pointing this out. - PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1542359758
