Re: [Rev 02] RFR: 8242489: ChoiceBox: initially toggle not sync'ed to selection
On Thu, 16 Apr 2020 15:04:53 GMT, Jeanette Winzenburg wrote: >> Macroscopic issue is that initially, the toggle is not sync'ed to the >> selection state. Root reason is an missing else >> block when updating toggle selection state (see report for details). >> Fixed by introducing the else block and removing all follow-up errors that >> tried to amend the consequences of the >> incorrect selection state >> - removed listener to selected item >> - removed toggle selection update in showing listener >> >> The former also fixed the memory leak when replacing the selectionModel plus >> an unreported NPE when the selectionModel >> is null initially. >> Added tests that failed before the fix and passed after. As there had been >> no tests around toggle state, so added some >> to verify that the change doesn't break. Enhanced shim/skin to allow access >> to popup for testing. Removed the >> informally ignored test part for memory leak. > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > ChoiceBox/-Skin/-SelectionTest > > - added code comments to clarify test/implemenation intention Marked as reviewed by arapte (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/177
Re: [Rev 02] RFR: 8242489: ChoiceBox: initially toggle not sync'ed to selection
> Macroscopic issue is that initially, the toggle is not sync'ed to the > selection state. Root reason is an missing else > block when updating toggle selection state (see report for details). > Fixed by introducing the else block and removing all follow-up errors that > tried to amend the consequences of the > incorrect selection state > - removed listener to selected item > - removed toggle selection update in showing listener > > The former also fixed the memory leak when replacing the selectionModel plus > an unreported NPE when the selectionModel > is null initially. > Added tests that failed before the fix and passed after. As there had been no > tests around toggle state, so added some > to verify that the change doesn't break. Enhanced shim/skin to allow access > to popup for testing. Removed the > informally ignored test part for memory leak. Jeanette Winzenburg has updated the pull request incrementally with one additional commit since the last revision: ChoiceBox/-Skin/-SelectionTest - added code comments to clarify test/implemenation intention - Changes: - all: https://git.openjdk.java.net/jfx/pull/177/files - new: https://git.openjdk.java.net/jfx/pull/177/files/0d3ce123..01aa7901 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/177/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/177/webrev.01-02 Stats: 5 lines in 2 files changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/177.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/177/head:pull/177 PR: https://git.openjdk.java.net/jfx/pull/177
