On Thu, 8 Jul 2021 11:59:05 GMT, Jeanette Winzenburg <faste...@openjdk.org> wrote:
>>> Hmm ... wondering whether we really want to widen the scope of this issue >>> >>> * it started with being focused on NPE on the change of property value, >>> for both Choice/ComboBox >>> >>> * turned out combo's skin also has a throwing listener to value >>> >>> * review spotted an additional failure candidates in ComboBox' editable >>> property >>> >>> * plus a sore spot in combo's skin (one of the locations where >>> list/combo selection is sync'ed, there are others ;) >>> >>> >>> the first two are naturally within the original scope, the third is near >>> enough (a property on one of the covered controls) to be included .. the >>> last is arguable, IMO - would tend to not include it here but open a >>> follow-up to then include _all_ sync issues in the skin (probably needs >>> more digging and definitely more testing). >>> >>> Thoughts? >> >> I reviewed the files and found those two places in files being edited in >> this PR were missing the newly added null check. It is logical to add null >> checks in all places in files being edited in this PR. >> Fixing review comments in the same PR or creating a follow-up issue is up to >> the author and reviewer's agreement. >> Let us not undo the change which is already done in this PR :) >> >> If we are sure that the similar fix is needed at other places (apart from >> the files which are being fixed in this PR), we can do it in a single OR >> multiple follow-up issue(s). > >> > the first two are naturally within the original scope, the third is near >> > enough (a property on one of the covered controls) to be included .. the >> > last is arguable, IMO - would tend to not include it here but open a >> > follow-up to then include _all_ sync issues in the skin (probably needs >> > more digging and definitely more testing). >> > Thoughts? >> >> I reviewed the files and found those two places in files being edited in >> this PR were missing the newly added null check. It is logical to add null >> checks in all places in files being edited in this PR. > > good point, except that it's not _all_ in the skin *grinning (there are two > in listeners to listView selectionModel properties) > >> Fixing review comments in the same PR or creating a follow-up issue is up to >> the author and reviewer's agreement. >> Let us not undo the change which is already done in this PR :) >> > > I like the _not undoing_ aspect :) - so the question is: should we find and > fix all in combo's skin in this issue or leave this as-is and create a > follow-up for the not yet included? > >> If we are sure that the similar fix is needed at other places (apart from >> the files which are being fixed in this PR), we can do it in a single OR >> multiple follow-up issue(s). > > probably was unclear: didn't mean other controls - suspect there are, which > we wait to bubble up sometime :) Good point. I have no problem fixing more NPEs which are directly related to setting the selectionModel to null. That might be also better then creating a lot of new bugs/follow-ups. Will have a look later on this. ------------- PR: https://git.openjdk.java.net/jfx/pull/557
