On Tue, 24 Mar 2026 01:26:56 GMT, chuckyschluz <[email protected]> wrote:
>> Fixes [JDK-8311505](https://bugs.openjdk.org/browse/JDK-8311505) >> >> This PR uses `set(int index, int end, boolean isSet)` in >> `javafx.scene.control.MultipleSelectionModelBase` to avoid excessive calls >> to `indexOf` when deselecting continuous ranges of rows. >> >> `TableView`: >> >> Item Count | Master Select | Fixed Select | Select Improv. | Master Deselect >> | Fixed Deselect | Deselect Improv. >> -- | -- | -- | -- | -- | -- | -- >> 16,384 | 7ms | 7ms | — | 83ms | 4ms | ~20.8x >> 32,768 | 13ms | 12ms | ~1.1x | 331ms | 5ms | ~66.2x >> 65,536 | 25ms | 17ms | ~1.5x | 1,283ms | 8ms | ~160.4x >> 131,072 | 63ms | 28ms | ~2.3x | 5,142ms | 13ms | ~395.5x >> 262,144 | 195ms | 58ms | ~3.4x | 20,526ms | 23ms | ~892.4x >> >> `TreeTableView`: >> >> Item Count | Master Select | Fixed Select | Select Improv. | Master Deselect >> | Fixed Deselect | Deselect Improv. >> -- | -- | -- | -- | -- | -- | -- >> 16,384 | 310ms | 313ms | — | 81ms | 1ms | ~81.0x >> 32,768 | 1,451ms | 1,448ms | — | 315ms | 3ms | ~105.0x >> 65,536 | 6,505ms | 6,629ms | — | 1,247ms | 6ms | ~207.8x >> 131,072 | 37,034ms | 39,452ms | — | 5,045ms | 11ms | ~458.6x >> 262,144 | 260,019ms | 256,854ms | — | 20,025ms | 22ms | ~910.2x >> >> Passes all unit tests: >> >> `.\gradlew.bat :controls:test` >> `.\gradlew.bat :systemTests:test` >> >> Thank you! > > chuckyschluz has updated the pull request incrementally with one additional > commit since the last revision: > > address reviewer comments modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewTest.java line 6521: > 6519: sm1.selectRange(3, 7); > 6520: sm2.selectIndices(3, 4, 5, 6); > 6521: assertEquals(sm1.getSelectedIndices(), List.of(3, 4, 5, 6)); Changes look good, but you mixed the `assertEquals` parameters. The first parameter should be the expected value. So the value we 100% know should be in the second parameter. This is also important for the test message if the test failed. Note: `assertEquals(sm1.getSelectedIndices(), sm2.getSelectedIndices());` can stay the same, just talking about the newly added asserts with the `List`. While here, if you think we can split this test up into 2 tests as well (like for `ListView`), feel free to do so! But also fine for me as it is right now :) ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2100#discussion_r2980123919
