On Tue, 13 Oct 2020 15:03:26 GMT, Jeanette Winzenburg <[email protected]> wrote:
>> `TabPaneSkin` installs some listeners that are not removed when >> `TabPaneSkin` is changed. >> The fix converts listeners to WeakListeners and also removes them on dispose. >> >> There is a NPE check change needed in `isHosrizontal()`. Without this check >> it causes NPE if pulse is in progress while TabPaneSkin is getting disposed. >> >> `SkinMemoryLeakTest` already had a test which only needed to be enabled. >> Test fails before and passes after this change. > > no review yet, just a couple of quick comments from my experience on recent > cleanup of skins: > > - if NPE checks seem to be necessary, they always indicate an illegal state: > whatever a method is doing, it must not access the skin after dispose. > Usually it's the caller of the method that's misbehaving, it simply must not > happen. It's worth digging why _exactly_ that's happening and if/how it can > be solved without guarding against the null > - while the overall memory test is already done, we still must test every > single skin against side-effects (f.i. of listeners not doing any "late" > update due to not being yet removed - the NPE above could well be such a > case). Have a look at SkinCleanupTest for examples > - when changing listener wiring, it's often a good idea to test that they are > still doing there job (if not yet covered in the available tests) > > yeah, you don't get away with not writing tests *good-humored-grinning quick question: is this still in the lane for fx16? ------------- PR: https://git.openjdk.java.net/jfx/pull/318
