On Fri, 12 May 2023 18:13:46 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Even though I don't particularly like register*Listener() because of its >> asymmetric nature (when it comes to removing listeners), here we do need to >> create weak listeners that get unregistered upon `dispose()`. We need weak >> listeners because TreeTableView does not explicitly "disconnect" unused >> cells (e.g. refresh()), and we need `dispose()` due to skin life cycle. >> >> So, in this particular case, I think this change should be ok. > > Sorry, I think we may be misunderstanding each other. I'm specifically > talking about the two listeners added in `TreeTableRowSkin`'s constructor on > `indexProperty` and `treeItemProperty`. These are part of the `TreeTableRow` > which is also discarded when `refresh` is called. > > 1) The test passes if these changes are reverted. This is a clear indication > that either the test is insufficient, or that your assumption, that these > must be weak, is incorrect. If you can construct a test that requires these > listeners to be weak, then I think the changes are warranted. > > 2) At the risk of stating the obvious, the listeners in the constructor added > using `ListenerHelper` are also correctly removed in `dispose`. > `ListenerHelper` does this in a similar way to the `register` methods. > > 3) The `indexProperty` and `treeItemProperty` listeners are attached to the > `TreeTableRow`, not the `TreeTableView`. The refresh function replaces the > entire row, including the skin. There is no need to use weak listeners for > this purpose. Compare this to the listeners that are attached to the > `TreeTableView` or the `VirtualFlow`; these have a much longer lifecycle and > can survive multiple refreshes and row factory replacements, and hence should > be weak (for now). you are right, of course. and I should know - ListenerHelper is my baby. I guess the fear of another regression overtook me. reverting. thank you for your patience and persistence! ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1192803129
