Re: CheckBoxTreeItem behavior
On 18/11/2022 00:10, Nir Lisker wrote: Hi, I have been working on fixing some issues with the behavior of CheckBoxTreeItem. I stumbled across this situation: When a parent is de/selected, all of its children are set to the same state. However, a CheckBoxTreeItem can be set to indeterminate programmatically (the control itself does not allow indeterminate). Should all children also be set to an indeterminate state? If so, this will put the tree at an "invalid" state where leaf nodes can be indeterminate as well. OTOH, if not, then we are again at an invalid state if all children have the same state, but the parent doesn't. I think the indeterminate state is best left up completely to the owner of the control. You may need information that is not part of the tree to actually determine if something is indeterminate or not. A tree may be filtered, or a tree may represent only "nodes" while "leaves" are displayed in a 2nd control as a list. This means that in theory, parent nodes could be indeterminate even if all its children are in the same state (checked/unchecked) due to filtering, and leaf nodes could be indeterminate if they represent a directory while the file selection is displayed in a separate control. In Backup software, a directory may be partially selected if it has a filter associated with it (like *.java) even if that directory is empty or has only Java files currently... Some UI's will even allow you to click on an indeterminate parent node to check it (checking all children), click again to uncheck it (unchecked all children), and click a 3rd time to put it back to an indeterminate state (restoring all children to the state they had before the first click). --John
Re: RFR: 8260528: Clean glass-gtk sizing and positioning code [v27]
On Thu, 17 Nov 2022 15:42:29 GMT, Johan Vos wrote:
>> Thiago Milczarek Sayao has refreshed the contents of this pull request, and
>> previous commits have been removed. The incremental views will show
>> differences compared to the previous content of the PR. The pull request
>> contains one new commit since the last revision:
>>
>> 8260528: Clean glass-gtk sizing and positioning code
>
> modules/javafx.graphics/src/main/native-glass/gtk/glass_window.cpp line 713:
>
>> 711:
>> 712: static int geometry_get_window_width(const WindowGeometry
>> *windowGeometry) {
>> 713: return (windowGeometry->final_width.type != BOUNDSTYPE_WINDOW)
>
> minor: wouldn't it be more readable (here and below) if the test was
> reversed? ( `(windowGeometry->final_width.type != BOUNDSTYPE_WINDOW`)
Those functions already existed. Do you prefer to keep as it was or to invert
the `if`?
> modules/javafx.graphics/src/main/native-glass/gtk/glass_window.cpp line 891:
>
>> 889:
>> 890: if (get_frame_extents_property(&top, &left, &bottom, &right)) {
>> 891: if ((top + right + bottom + left) != 0) {
>
> Are we 100% sure the extents can never be negative values?
I'm 99.99% sure :)
> modules/javafx.graphics/src/main/native-glass/gtk/glass_window.cpp line 1047:
>
>> 1045:
>> 1046: if (moved) {
>> 1047: geometry_set_window_x(&geometry, x);
>
> this will set `windowGeometry->refx` where 2 lines below you set
> `windowGeometry.current_x`. Unless there is gravity involved, both refer to
> the same value, one being `int` and the other being `float` though. Is there
> another conceptual difference, or why do we have 2 values here?
I think you're right, I added `current_x/y` to check if a window moved, but
`ref_x/ref_y` might contain the same value (different type).
> modules/javafx.graphics/src/main/native-glass/gtk/glass_window.cpp line 1269:
>
>> 1267: }
>> 1268:
>> 1269: void WindowContextTop::to_front() {
>
> Do you use `raise` and `lower` here because the sibiling parameter is always
> `NULL` ?
Yes. But also `gdk_window_raise` and `gdk_window_lower` seems to better match
the `toFront()` and `toBack()` description.
See:
https://tronche.com/gui/x/xlib/window/XRestackWindows.html
https://tronche.com/gui/x/xlib/window/XRaiseWindow.html
gdk functions calls those Xlib functions internally.
-
PR: https://git.openjdk.org/jfx/pull/915
Re: RFR: 8260528: Clean glass-gtk sizing and positioning code [v27]
On Wed, 16 Nov 2022 21:16:37 GMT, Johan Vos wrote: >> Thiago Milczarek Sayao has refreshed the contents of this pull request, and >> previous commits have been removed. The incremental views will show >> differences compared to the previous content of the PR. The pull request >> contains one new commit since the last revision: >> >> 8260528: Clean glass-gtk sizing and positioning code > > I'll do more testing and a detailed review, but first tests look good. @johanvos I replied the review comments but grouped them and It didn't trigger an email. - PR: https://git.openjdk.org/jfx/pull/915
RFR: 8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest
The change moves Locale setting in the test to @BeforeClass and @AfterClass calls. @BeforeClass method call stores current default VM locale and applies Locale.US, while @AfterClass method restores old VM locale after all tests are completed. I tested it both on Mac and Windows, in both cases Locale is changed, restored properly and tests pass. - Commit messages: - 8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest Changes: https://git.openjdk.org/jfx/pull/954/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=954&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8265828 Stats: 16 lines in 1 file changed: 13 ins; 3 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/954.diff Fetch: git fetch https://git.openjdk.org/jfx pull/954/head:pull/954 PR: https://git.openjdk.org/jfx/pull/954
Re: RFR: 8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest
On Thu, 17 Nov 2022 16:59:09 GMT, Lukasz Kostyra wrote:
> The change moves Locale setting in the test to `@BeforeClass` and
> `@AfterClass` calls. `@BeforeClass` method call stores current default VM
> locale and applies Locale.US, while `@AfterClass` method restores old VM
> locale after all tests are completed.
>
> I tested it both on Mac and Windows, in both cases Locale is changed,
> restored properly and tests pass.
Looks good.
modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java
line 96:
> 94: }
> 95:
> 96: @BeforeClass public static void setupBeforeAll() {
Minor: we usually put annotations on a separate line, although some files (like
this one) put the `@Test` annotation on the same line, splitting them is
preferred. I'll approve it as-is, and reapprove if you decide to change (I'll
leave it up to you).
-
Marked as reviewed by kcr (Lead).
PR: https://git.openjdk.org/jfx/pull/954
Re: RFR: 8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest
On Thu, 17 Nov 2022 16:59:09 GMT, Lukasz Kostyra wrote:
> The change moves Locale setting in the test to `@BeforeClass` and
> `@AfterClass` calls. `@BeforeClass` method call stores current default VM
> locale and applies Locale.US, while `@AfterClass` method restores old VM
> locale after all tests are completed.
>
> I tested it both on Mac and Windows, in both cases Locale is changed,
> restored properly and tests pass.
modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java
line 60:
> 58: private static final DateTimeFormatter aFormatter =
> DateTimeFormatter.ofPattern("dd MM HH mm ss");
> 59: private static final DateTimeFormatter aParser =
> DateTimeFormatter.ofPattern(" MM dd hh mm ss a");
> 60: private static Locale oldLocale;
Isn't the creation of the DateTimeFormatter using the default locale? If so,
this should probably be done after the locale is set.
-
PR: https://git.openjdk.org/jfx/pull/954
Re: RFR: 8297213: Robot capture tests should move mouse to corner of screen
On Fri, 18 Nov 2022 00:48:11 GMT, Andy Goryachev wrote:
> Added Util.parkCursor(Robot) method to move the cursor to lower left corner
> (while avoiding dock, tray, or active corners).
Looks good with one inline comment about a redundant call to `parkCursor`.
tests/system/src/test/java/test/robot/javafx/scene/layout/RegionBackgroundImageUITest.java
line 50:
> 48: @Before
> 49: public void before() {
> 50: Util.parkCursor(getRobot());
This is redundant, since `RegionBackgroundImageUITest` extends from
`VisuaTestBase`, which already calls `parkCursor`. You can remove the `before`
method.
-
PR: https://git.openjdk.org/jfx/pull/955
Re: RFR: 8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest
On Fri, 18 Nov 2022 16:00:33 GMT, Marius Hanl wrote:
>> The change moves Locale setting in the test to `@BeforeClass` and
>> `@AfterClass` calls. `@BeforeClass` method call stores current default VM
>> locale and applies Locale.US, while `@AfterClass` method restores old VM
>> locale after all tests are completed.
>>
>> I tested it both on Mac and Windows, in both cases Locale is changed,
>> restored properly and tests pass.
>
> modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java
> line 60:
>
>> 58: private static final DateTimeFormatter aFormatter =
>> DateTimeFormatter.ofPattern("dd MM HH mm ss");
>> 59: private static final DateTimeFormatter aParser =
>> DateTimeFormatter.ofPattern(" MM dd hh mm ss a");
>> 60: private static Locale oldLocale;
>
> Isn't the creation of the DateTimeFormatter using the default locale? If so,
> this should probably be done after the locale is set.
This is a good point. Moving the initialization of those two fields to the
`setupBeforeAll` method seems safest.
-
PR: https://git.openjdk.org/jfx/pull/954
Re: RFR: 8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest
On Thu, 17 Nov 2022 16:59:09 GMT, Lukasz Kostyra wrote: > The change moves Locale setting in the test to `@BeforeClass` and > `@AfterClass` calls. `@BeforeClass` method call stores current default VM > locale and applies Locale.US, while `@AfterClass` method restores old VM > locale after all tests are completed. > > I tested it both on Mac and Windows, in both cases Locale is changed, > restored properly and tests pass. Pending response to comment raised by Marius. - Changes requested by kcr (Lead). PR: https://git.openjdk.org/jfx/pull/954
Re: RFR: 8297213: Robot capture tests should move mouse to corner of screen [v2]
On Fri, 18 Nov 2022 14:37:34 GMT, Kevin Rushforth wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8297213: review comments
>
> tests/system/src/test/java/test/robot/javafx/scene/layout/RegionBackgroundImageUITest.java
> line 50:
>
>> 48: @Before
>> 49: public void before() {
>> 50: Util.parkCursor(getRobot());
>
> This is redundant, since `RegionBackgroundImageUITest` extends from
> `VisuaTestBase`, which already calls `parkCursor`. You can remove the
> `before` method.
missed that, thanks!
-
PR: https://git.openjdk.org/jfx/pull/955
Re: RFR: 8297213: Robot capture tests should move mouse to corner of screen [v2]
> Added Util.parkCursor(Robot) method to move the cursor to lower left corner > (while avoiding dock, tray, or active corners). Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: 8297213: review comments - Changes: - all: https://git.openjdk.org/jfx/pull/955/files - new: https://git.openjdk.org/jfx/pull/955/files/55a022c5..0e525b2f Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=955&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=955&range=00-01 Stats: 8 lines in 1 file changed: 0 ins; 8 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/955.diff Fetch: git fetch https://git.openjdk.org/jfx pull/955/head:pull/955 PR: https://git.openjdk.org/jfx/pull/955
Re: RFR: 8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest
On Thu, 17 Nov 2022 16:59:09 GMT, Lukasz Kostyra wrote:
> The change moves Locale setting in the test to `@BeforeClass` and
> `@AfterClass` calls. `@BeforeClass` method call stores current default VM
> locale and applies Locale.US, while `@AfterClass` method restores old VM
> locale after all tests are completed.
>
> I tested it both on Mac and Windows, in both cases Locale is changed,
> restored properly and tests pass.
modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java
line 60:
> 58: private static final DateTimeFormatter aFormatter =
> DateTimeFormatter.ofPattern("dd MM HH mm ss");
> 59: private static final DateTimeFormatter aParser =
> DateTimeFormatter.ofPattern(" MM dd hh mm ss a");
> 60: private static Locale oldLocale;
I wonder how many other tests we have that depend on specific Locale? Perhaps
we need to apply the same treatment to:
- LocalDateStringConverterTest
- LocalTimeStringConverterTest
-
PR: https://git.openjdk.org/jfx/pull/954
Re: RFR: 8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest
On Fri, 18 Nov 2022 15:45:46 GMT, Kevin Rushforth wrote:
>> The change moves Locale setting in the test to `@BeforeClass` and
>> `@AfterClass` calls. `@BeforeClass` method call stores current default VM
>> locale and applies Locale.US, while `@AfterClass` method restores old VM
>> locale after all tests are completed.
>>
>> I tested it both on Mac and Windows, in both cases Locale is changed,
>> restored properly and tests pass.
>
> modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java
> line 96:
>
>> 94: }
>> 95:
>> 96: @BeforeClass public static void setupBeforeAll() {
>
> Minor: we usually put annotations on a separate line, although some files
> (like this one) put the `@Test` annotation on the same line, splitting them
> is preferred. I'll approve it as-is, and reapprove if you decide to change
> (I'll leave it up to you).
Since I have to make some changes to this PR, I will update this as well.
-
PR: https://git.openjdk.org/jfx/pull/954
Re: RFR: 8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest
On Fri, 18 Nov 2022 16:17:55 GMT, Kevin Rushforth wrote:
>> modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java
>> line 60:
>>
>>> 58: private static final DateTimeFormatter aFormatter =
>>> DateTimeFormatter.ofPattern("dd MM HH mm ss");
>>> 59: private static final DateTimeFormatter aParser =
>>> DateTimeFormatter.ofPattern(" MM dd hh mm ss a");
>>> 60: private static Locale oldLocale;
>>
>> Isn't the creation of the DateTimeFormatter using the default locale? If so,
>> this should probably be done after the locale is set.
>
> This is a good point. Moving the initialization of those two fields to the
> `setupBeforeAll` method seems safest.
That is a fair point.
I'll have to change the code a bit, as `implementations()` method is called
before a `@BeforeClass`-tagged method (which is probably why originally
`Locale.setDefault()` was called there) and `aFormatter`/`aParser` are already
used there, expected to be initialized.
-
PR: https://git.openjdk.org/jfx/pull/954
Re: RFR: 8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest
On Fri, 18 Nov 2022 16:50:21 GMT, Andy Goryachev wrote:
>> The change moves Locale setting in the test to `@BeforeClass` and
>> `@AfterClass` calls. `@BeforeClass` method call stores current default VM
>> locale and applies Locale.US, while `@AfterClass` method restores old VM
>> locale after all tests are completed.
>>
>> I tested it both on Mac and Windows, in both cases Locale is changed,
>> restored properly and tests pass.
>
> modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java
> line 60:
>
>> 58: private static final DateTimeFormatter aFormatter =
>> DateTimeFormatter.ofPattern("dd MM HH mm ss");
>> 59: private static final DateTimeFormatter aParser =
>> DateTimeFormatter.ofPattern(" MM dd hh mm ss a");
>> 60: private static Locale oldLocale;
>
> I wonder how many other tests we have that depend on specific Locale?
> Perhaps we need to apply the same treatment to:
> - LocalDateStringConverterTest
> - LocalTimeStringConverterTest
I could also change those, as they use `DateTimeFormatter` as well which uses
`Locale` underneath as discussed above. @kevinrushforth what do you think?
-
PR: https://git.openjdk.org/jfx/pull/954
Re: RFR: 8297213: Robot capture tests should move mouse to corner of screen [v2]
On Fri, 18 Nov 2022 16:44:06 GMT, Andy Goryachev wrote: >> Added Util.parkCursor(Robot) method to move the cursor to lower left corner >> (while avoiding dock, tray, or active corners). > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > 8297213: review comments Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.org/jfx/pull/955
Re: RFR: 8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest
On Fri, 18 Nov 2022 17:00:08 GMT, Lukasz Kostyra wrote: >> This is a good point. Moving the initialization of those two fields to the >> `setupBeforeAll` method seems safest. > > That is a fair point. > > I'll have to change the code a bit, as `implementations()` method is called > before a `@BeforeClass`-tagged method (which is probably why originally > `Locale.setDefault()` was called there) and `aFormatter`/`aParser` are > already used there, expected to be initialized. Maybe JUnit5 can help you here. You can check out my recently merged PR for an example for the usage of the parameterized api introduced in JUnit5. https://github.com/openjdk/jfx/pull/936 - PR: https://git.openjdk.org/jfx/pull/954
Re: RFR: 8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest
On Thu, 17 Nov 2022 16:59:09 GMT, Lukasz Kostyra wrote: > The change moves Locale setting in the test to `@BeforeClass` and > `@AfterClass` calls. `@BeforeClass` method call stores current default VM > locale and applies Locale.US, while `@AfterClass` method restores old VM > locale after all tests are completed. > > I tested it both on Mac and Windows, in both cases Locale is changed, > restored properly and tests pass. see inline comments - PR: https://git.openjdk.org/jfx/pull/954
Re: RFR: 8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest
On Fri, 18 Nov 2022 17:00:42 GMT, Lukasz Kostyra wrote:
>> modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java
>> line 60:
>>
>>> 58: private static final DateTimeFormatter aFormatter =
>>> DateTimeFormatter.ofPattern("dd MM HH mm ss");
>>> 59: private static final DateTimeFormatter aParser =
>>> DateTimeFormatter.ofPattern(" MM dd hh mm ss a");
>>> 60: private static Locale oldLocale;
>>
>> I wonder how many other tests we have that depend on specific Locale?
>> Perhaps we need to apply the same treatment to:
>> - LocalDateStringConverterTest
>> - LocalTimeStringConverterTest
>
> I could also change those, as they use `DateTimeFormatter` as well which uses
> `Locale` underneath as discussed above. @kevinrushforth what do you think?
Yes, it seems reasonable to include those tests as well, since those tests have
the same problem.
-
PR: https://git.openjdk.org/jfx/pull/954
Re: RFR: 8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest
On Fri, 18 Nov 2022 17:10:04 GMT, Marius Hanl wrote:
>> That is a fair point.
>>
>> I'll have to change the code a bit, as `implementations()` method is called
>> before a `@BeforeClass`-tagged method (which is probably why originally
>> `Locale.setDefault()` was called there) and `aFormatter`/`aParser` are
>> already used there, expected to be initialized.
>
> Maybe JUnit5 can help you here.
> You can check out my recently merged PR for an example for the usage of the
> parameterized api introduced in JUnit5.
> https://github.com/openjdk/jfx/pull/936
I guess they fixed this rather irritating "feature" in JUnit5. Another,
possibly less-intrusive, approach would be to initialize all of the static
fields in a `static { ... }` block (with a comment as to why you can't use an
`@BeforeClass` method.
-
PR: https://git.openjdk.org/jfx/pull/954
Integrated: 8297213: Robot capture tests should move mouse to corner of screen
On Fri, 18 Nov 2022 00:48:11 GMT, Andy Goryachev wrote: > Added Util.parkCursor(Robot) method to move the cursor to lower left corner > (while avoiding dock, tray, or active corners). This pull request has now been integrated. Changeset: 086dac0b Author:Andy Goryachev Committer: Kevin Rushforth URL: https://git.openjdk.org/jfx/commit/086dac0b12233f31db33bb9fc43d821724710f70 Stats: 76 lines in 7 files changed: 48 ins; 23 del; 5 mod 8297213: Robot capture tests should move mouse to corner of screen Reviewed-by: kcr - PR: https://git.openjdk.org/jfx/pull/955
Re: CheckBoxTreeItem behavior
I think that even if a node is set to indeterminate programmatically, its parents should be updated at the very least (unless it's set to be independent). Not updating the children seems reasonable. On Fri, Nov 18, 2022 at 11:12 AM John Hendrikx wrote: > > On 18/11/2022 00:10, Nir Lisker wrote: > > Hi, > > > > I have been working on fixing some issues with the behavior > > of CheckBoxTreeItem. I stumbled across this situation: > > > > When a parent is de/selected, all of its children are set to the same > > state. However, a CheckBoxTreeItem can be set to indeterminate > > programmatically (the control itself does not allow indeterminate). > > Should all children also be set to an indeterminate state? If so, this > > will put the tree at an "invalid" state where leaf nodes can be > > indeterminate as well. OTOH, if not, then we are again at an invalid > > state if all children have the same state, but the parent doesn't. > > I think the indeterminate state is best left up completely to the owner > of the control. You may need information that is not part of the tree to > actually determine if something is indeterminate or not. A tree may be > filtered, or a tree may represent only "nodes" while "leaves" are > displayed in a 2nd control as a list. This means that in theory, parent > nodes could be indeterminate even if all its children are in the same > state (checked/unchecked) due to filtering, and leaf nodes could be > indeterminate if they represent a directory while the file selection is > displayed in a separate control. In Backup software, a directory may be > partially selected if it has a filter associated with it (like *.java) > even if that directory is empty or has only Java files currently... > > Some UI's will even allow you to click on an indeterminate parent node > to check it (checking all children), click again to uncheck it > (unchecked all children), and click a 3rd time to put it back to an > indeterminate state (restoring all children to the state they had before > the first click). > > --John >
Re: RFR: 8256397: MultipleSelectionModel throws IndexOutOfBoundException [v5]
On Thu, 14 Jul 2022 08:05:25 GMT, Florian Kirmaier
wrote:
>> Fixing IndexOutOfBoundsException in the MultipleSelectionModelBase and added
>> a unit-test for it.
>> ticket: https://bugs.openjdk.java.net/browse/JDK-8256397
>> run test: `./gradlew --continue -PFULL_TEST=true controls:test --tests
>> "*MultipleSelectionModelImplTest*"`
>
> Florian Kirmaier has updated the pull request incrementally with one
> additional commit since the last revision:
>
> JDK_8256397
> Fixed more issues with the multiple selection model change events, and
> verified them with more unit-tests.
modules/javafx.controls/src/test/java/test/javafx/scene/control/MultipleSelectionModelImplTest.java
line 1456:
> 1454: testSelectedIndicesChangeEventsFactory(0, new int[]{2,3}, 1,
> new int[]{5,7},
> 1455: new int[][]{new int[]{1}, new int[]{5,7}});
> 1456:
Do we have a test that tries to select indexes outside of the model?
modules/javafx.controls/src/test/java/test/javafx/scene/control/MultipleSelectionModelImplTest.java
line 1467:
> 1465:
> 1466: testSelectedIndicesChangeEventsFactory(2, new int[]{3, 6,7}, 0,
> new int[]{1,4,5,8,9},
> 1467: new int[][]{new int[]{0,1},new int[]{4,5},new
> int[]{8,9}});
I wonder if this test covers all the possible code paths in the implementation.
To illustrate just a subset of scenarios that should be tested, let's consider
these scenarios (initial selection on the top line, change on the bottom line):
index 0 flipped, index 1 unchanged:
-I-
||-
index 1 unchanged
-|-
-|-
index 1 unchanged, index 2 flipped
-|-
-||
index 0 and 2 flipped
|--
--|
etc.
Perhaps it is possible to design a test that user random number generator to
generate initial state and the changes, apply these to both
MultipleSelectionModelBase and let's say a BitSet, and then compare the results
- doing this 10,000 times (a sort of fuzzing test). A model with 10 items
might be sufficient.
modules/javafx.controls/src/test/java/test/javafx/scene/control/MultipleSelectionModelImplTest.java
line 1478:
> 1476: listView.getItems().add("item-4");
> 1477: listView.getItems().add("item-5");
> 1478: listView.getItems().add("item-6");
minor:
.getItems().addAll(
"1",
"2",
..
);
-
PR: https://git.openjdk.org/jfx/pull/353
JavaFX snapshots in Maven
Hi, I am trying to run tests with the officially released WebKit and Media shared libraries using the -PSTUB_RUNTIME_OPENJFX option [1]. But it is failing in Gradle dependency verification with below error as checksums are missing from verification metadata. A problem occurred evaluating root project 'jfx'. > Dependency verification failed for configuration ':openjfxStubs' 5 artifacts failed verification: - javafx-20-ea+7.pom (org.openjfx:javafx:20-ea+7) from repository MavenRepo - javafx-media-20-ea+7-win.jar (org.openjfx:javafx-media:20-ea+7) from repository MavenRepo - javafx-media-20-ea+7.pom (org.openjfx:javafx-media:20-ea+7) from repository MavenRepo - javafx-web-20-ea+7-win.jar (org.openjfx:javafx-web:20-ea+7) from repository MavenRepo - javafx-web-20-ea+7.pom (org.openjfx:javafx-web:20-ea+7) from repository MavenRepo If the artifacts are trustworthy, you will need to update the gradle/verification-metadata.xml file by following the instructions at https://docs.gradle.org/7.3/userguide/dependency_verification.html#sec:troubleshooting-verification One way of resolving this issue would be to use SNAPSHOT dependencies. Gradle does not verify them as their checksums would always change [2]. Also developers will no longer need to manually check and specify the latest version for the -PSTUB_RUNTIME_OPENJFX option (after changes are made in the build file to always refer to the SNAPSHOT version when this option is specified). But currently in Maven only EA builds of JavaFX are provided. SNAPSHOT builds are not provided. There had been a discussion in the past to not provide SNAPSHOT builds due to some Gradle issue [3]. But since it has been more than 4 years since then, maybe the situation has changed. Is providing SNAPSHOT builds a possibility now ? This will be convenient for developers especially for those who are not actively working on the sources of WebKit and Media libraries. [1] : https://github.com/openjdk/jfx/blob/master/WEBKIT-MEDIA-STUBS.md#officially-released-libraries [2] : https://docs.gradle.org/7.3/userguide/dependency_verification.html#sub:verification-metadata [3] : https://mail.openjdk.org/pipermail/openjfx-dev/2018-July/022107.html -- Anirvan
Re: CheckBoxTreeItem behavior
Yeah, I didn't consider the other direction, that makes sense. --John On 18/11/2022 18:28, Nir Lisker wrote: I think that even if a node is set to indeterminate programmatically, its parents should be updated at the very least (unless it's set to be independent). Not updating the children seems reasonable. On Fri, Nov 18, 2022 at 11:12 AM John Hendrikx wrote: On 18/11/2022 00:10, Nir Lisker wrote: > Hi, > > I have been working on fixing some issues with the behavior > of CheckBoxTreeItem. I stumbled across this situation: > > When a parent is de/selected, all of its children are set to the same > state. However, a CheckBoxTreeItem can be set to indeterminate > programmatically (the control itself does not allow indeterminate). > Should all children also be set to an indeterminate state? If so, this > will put the tree at an "invalid" state where leaf nodes can be > indeterminate as well. OTOH, if not, then we are again at an invalid > state if all children have the same state, but the parent doesn't. I think the indeterminate state is best left up completely to the owner of the control. You may need information that is not part of the tree to actually determine if something is indeterminate or not. A tree may be filtered, or a tree may represent only "nodes" while "leaves" are displayed in a 2nd control as a list. This means that in theory, parent nodes could be indeterminate even if all its children are in the same state (checked/unchecked) due to filtering, and leaf nodes could be indeterminate if they represent a directory while the file selection is displayed in a separate control. In Backup software, a directory may be partially selected if it has a filter associated with it (like *.java) even if that directory is empty or has only Java files currently... Some UI's will even allow you to click on an indeterminate parent node to check it (checking all children), click again to uncheck it (unchecked all children), and click a 3rd time to put it back to an indeterminate state (restoring all children to the state they had before the first click). --John
CheckBoxTreeItem behavior - independent property
Hi, Another issue I stumbled across is the usage of the Independent property on CheckBoxTreeItem. The docs read: A BooleanProperty used to represent the independent state of this > CheckBoxTreeItem. The independent state is used to represent whether > changes to a single CheckBoxTreeItem should influence the state of its > parent and children. By default, the independent property is false, which means that when a > CheckBoxTreeItem has state changes to the selected or indeterminate > properties, the state of related CheckBoxTreeItems will possibly be > changed. If the independent property is set to true, the state of related > CheckBoxTreeItems will never change. It makes it clear that changes to this checkbox don't influence its children/parents, and in terms of usage, it means that clicking on the checkbox doesn't change the state of its parents/children. However: 1. Does it also include stopping the propagation of selection updates through it? If an independent item has a child and a parent, and the parent is selected, does the item stop its child from being selected? I think that the answer is yes, but I want to make sure the independent property wasn't just meant for direct clicks or direct changes to its own properties alone. 2. Do the parents/children affect this item? The docs only mention one direction: item -> parents/children, it doesn't mention parents/children -> item. As it stands currently, the item seems to be affected by other selections. I find it odd because this doesn't make the item really independent, and I can't think of a use case for this one-sided independent behavior. In any case, I think that the documentation should clarify these points, so I would like to know what the behavior should be. - Nir
