On Thu, 11 Jan 2024 14:04:36 GMT, Johan Vos <j...@openjdk.org> wrote:
>> Johan Vos has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Revert some of the conditional bindings. >> Clear menu construction when an menuitem that is a menu needs to be removed >> Add a test for the latter > > A few things about the latest commit: > > 1. The usage of the `active` property caused regression: the memoryLeak test > that was introduced in the fix for JDK-8318841 now failed. A number of > listeners were hard referenced from the `active` property. > The complexity is that there are different entry points by which a listener > should be removed. What we try to do in this PR, is making sure we remove > listeners after a defocus/focus operation that would otherwise stay > referenced. A focus operation will result in GlassSystemMenu.setMenus() being > called. > I reverted some of the dependencies on the `active` property, and kept those > that were directly created as a consequence of the setMenus call. > > 2. I added a test that demonstrates the issue when not removing menu's > completely inside the listener, as reported by @jperedadnr and this issue is > fixed as well now. > > 3. All tests now pass, but I noticed that in some cases, the systemtests do > not correctly work with the application lifecycle management (see > https://mail.openjdk.org/pipermail/openjfx-dev/2024-January/044516.html). For > now, I consider this anomaly to be independent from JDK-8319779 @johanvos **ticket** We found a bug, which is also fixed by this PR. This is the ticket: https://bugs.openjdk.org/browse/JDK-8323787 It's an IndexOutOfBounds exception, related to the visible flag/listeners. **unittest** We wrote a unit test, which goes green with this PR. Can you do us a favor, and add the test from the ticket to your PR? Then we can be sure, that this issue will be solved and never reappears. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1283#issuecomment-1893444554
