On Wed, 8 Oct 2025 01:20:41 GMT, Sergey Bylokhov <[email protected]> wrote:
> The bug happens when List.select(int) is used in multiple selection mode. It > only appears when the List is added to a Frame, because in that case, the > selection is handled by the platform peer, and the internal logic in the List > class is skipped. > > To fix this, the code now uses > [addSelectionInterval](https://github.com/openjdk/jdk/blob/910bb68e5191f830ff6f3dff5753e4e5f6214a7b/src/java.desktop/share/classes/javax/swing/ListSelectionModel.java#L93) > instead of setSelectedIndex. This method works correctly for multiple > selection mode when using JList as the delegate on macOS. > > I added DeselectionUnitTest and SelectionUnitTest tests for some selection > and deselection cases. At first, it was one big test, but I split it because > it got too large. > > **Notes on invalid index handling** > According to the java.awt.List > [spec](https://github.com/openjdk/jdk/blob/0e5655e6680762a99b5aecb58369b880ea913565/src/java.desktop/share/classes/java/awt/List.java#L573), > using invalid indexes is undefined behavior. For now, I have decided not to > validate these cases fully in this patch, but I did add a check in the > `LWListPeer.select()` to handle them more safely. > > The previously used setSelectedIndex method > [ignored](https://github.com/openjdk/jdk/blob/0e5655e6680762a99b5aecb58369b880ea913565/src/java.desktop/share/classes/javax/swing/JList.java#L2229) > indexes greater than the list size, unlike addSelectionInterval. So I added > an explicit check to skip all indexes larger than the list size. > > Later, I discovered that passing a negative index not only causes an > exception (which is acceptable, since it's undefined behavior), but also > leaves the peer in an inconsistent state. This happens because > setSkipStateChangedEvent(false) at the end of LWListPeer.select() is not > called if an exception is thrown. > > To prevent this, I added a check to skip all negative values as well. As a > result, the peer now cleanly ignores all out-of-range indexes. > > **Description of how invalid indexes are handled on other platforms:** > - The shared code in java.awt.List stores elements and selection separately, > so it accepts invalid indexes. This can cause exceptions if the selection and > data become out of sync. > - On Windows, all invalid indexes are ignored, except for the special value > -1, which is used to select or deselect all elements. This happens because > the indexes are passed to the win api without validation. > - XAWT uses the same logic as the shared code, so it can throw the same > exceptions if the data... The list loses its ability to handle multiple selection after its added to a frame, do I understand the problem correctly? The test in the bug report first selects items and then adds the list to the frame. According to the tests, it's not a pre-requisite. I edited the subject of the issue and removed “may” because the issue always happens. If the answer to my question above is “yes”, the word “multiple” should be added to the subject. There's a lot of code duplication between the tests, this is understandable since the tests are closely related. I suggest creating a folder for these three tests and maybe creating a super-class for `*UnitTest.java` that contains the `Test` interface and the common utility methods `assertEquals`, etc. On the other hand, splitting the test code like will make reading the tests somewhat harder… However, I still suggest adding a folder, let's say `ListSelection`, and putting these three tests into the folder. One should always run all the three tests, therefore grouping them in a folder is reasonable. src/java.desktop/macosx/classes/sun/lwawt/LWListPeer.java line 143: > 141: getDelegate().setSkipStateChangedEvent(true); > 142: getDelegate().getView().addSelectionInterval(index, > index); > 143: getDelegate().setSkipStateChangedEvent(false); Does it make sense to call `setSkipStateChangedEvent(false)` in a `finally` block? It will ensure the component is left in a consistent state. test/jdk/java/awt/List/DeselectionUnitTest.java line 27: > 25: import java.awt.List; > 26: > 27: /** Suggestion: /* Let's not use javadoc syntax for jtreg tags. test/jdk/java/awt/List/DeselectionUnitTest.java line 36: > 34: > 35: public static void main(String[] args) { > 36: // non-displayable list Suggestion: // Non-displayable list I'd start with a capital letter for consistency because all other comments in the test start with a capital letter. Here, *displayable* is in the sense of `Component.isDisplayable()`, right? test/jdk/java/awt/List/DeselectionUnitTest.java line 37: > 35: public static void main(String[] args) { > 36: // non-displayable list > 37: testSingleMode(null); For consistency, Suggestion: testNonDisplayable(DeselectionUnitTest::testSingleMode);; where private static void testDisplayable(Test test) { test.execute(null); } Then the comments become redundant as the method names explicitly mention which state is tested. test/jdk/java/awt/List/DeselectionUnitTest.java line 42: > 40: testEmptyListDeselection(null); > 41: > 42: // displayable list Suggestion: // Displayable list test/jdk/java/awt/List/DeselectionUnitTest.java line 72: > 70: list.add("Item1"); > 71: list.add("Item2"); > 72: list.add("Item3"); I'd pull this piece of code into `createList` to avoid having the same code in all the test methods. test/jdk/java/awt/List/DeselectionUnitTest.java line 161: > 159: System.err.println("Expected: " + expected); > 160: System.err.println("Actual: " + actual); > 161: throw new RuntimeException("Values are not equal"); I'd still include the value into the exception message. test/jdk/java/awt/List/SelectInvalid.java line 28: > 26: import jdk.test.lib.Platform; > 27: > 28: /** Suggestion: /* ------------- PR Review: https://git.openjdk.org/jdk/pull/27682#pullrequestreview-3412685687 PR Review Comment: https://git.openjdk.org/jdk/pull/27682#discussion_r2487686109 PR Review Comment: https://git.openjdk.org/jdk/pull/27682#discussion_r2487692539 PR Review Comment: https://git.openjdk.org/jdk/pull/27682#discussion_r2487747353 PR Review Comment: https://git.openjdk.org/jdk/pull/27682#discussion_r2487756315 PR Review Comment: https://git.openjdk.org/jdk/pull/27682#discussion_r2487749710 PR Review Comment: https://git.openjdk.org/jdk/pull/27682#discussion_r2487726792 PR Review Comment: https://git.openjdk.org/jdk/pull/27682#discussion_r2487733674 PR Review Comment: https://git.openjdk.org/jdk/pull/27682#discussion_r2487758525
