On Mon, 22 Jan 2024 06:29:40 GMT, Prasanta Sadhukhan <[email protected]>
wrote:
>> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTreeUI.java line
>> 1338:
>>
>>> 1336: if(rendererPane != null) {
>>> 1337: if (!tree.isVisible()) {
>>> 1338: rendererPane.removeAll();
>>
>> if( -> if (
>>
>> Is it actually necessary to check visibility ?
>>
>> But more importantly, in the case of the test in the bug, how and when does
>> this uninstallComponents() method get called ? It isn't clear to me how it
>> changes the reported behaviour of that test.
>>
>> I thought uninstallComponents() is just part of uninstallUI() like when
>> tearing down and freeing the tree, or at least switching L&F .. whereas the
>> submitter seems to be implying nodeChanged() is the problem
>
> Yes, the submitter mentioned during nodeChanged() but I mentioned it before
> [in this
> comment](https://github.com/openjdk/jdk/pull/17458#issuecomment-1897684665)
> that the comment present, implied it's not an issue, if the component is not
> removed if UI is not changed (as it is in the case when tree is not
> visible)...I am not sure why it was added and when but should we ignore that
> comment and try to remove the component even if tree is not visible (and UI
> is not changing)?
>
> I went with the thought that the coder meant what he said and I freed the
> components at the end (as you told during tearing down) so that there is no
> leak..
My point is not about whether this is the only case we need to address, I am
asking what harm could come from removing the isVisible() test here ?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1464089825
