apache
Cleanup ContextAction and fix TerminalAction behavior
The core problem reported by #4199 is most probably introduced by 562cfade954ea27308de01738813d050d1b57ce1. That change made the action stateful and in turn the Performer must follow that state ties. The concrete case are ContextAwareAction instances, which are created per lookup.
See the commits:
- e1dd39de3a236df5cb11152371e284769f54c3a5
- 1a3b72455838230df66a82e3c006587af3e1b03a
- 32b711aa059f922b31d85b99ffae3833fd99ee00
for details.
While testing this change two further problems were observed:
- The title of the terminal windows does not properly update if it was set to the empty string once (18af4c98929899e2c451a42e3ff4974b4e8596fd)
- The target for the context menu action is not the tab the context menu was opened over, but the currently active tab. This does not match behavior of the IDE in general and thus fixed (0fd9f039efe60669f5e037cf170ac7f1c52a14fa)
Closes: #4199
@eirikbakke could you please have a look at this and see if this fixes the issue in your application? @neilcsmith-net your comment:
I say partially fix the title issue, as there seems to be a secondary issue with the context not changing to an unfocused tab when invoking the menu on it?
should be fixed by 0fd9f03
Thank you for your work on this! I'll see if I can reproduce NETBEANS-4754 without the patch (and without my old hacky patch), and then see if it disappears with this PR.
@eirikbakke I added a test, that uses an ActionListener as basis in addition to the ContextAwareAction test, to cover that case also. Of course this still needs to be validated in "in the wild". Thank you for that.
I'll have to run with my old patch removed for a while before testing this one, as I'm waiting for the NETBEANS-4754 bug to randomly reappear on the latest NetBeans version. Hopefully I'll find a reliable way to reproduce it again... and then I can test if this PR fixes it.
Is there any benefit to keeping the delegate in a weak reference now there's a 1-to-1 mapping of context action to performer? Having any GC dependent code requires thought and care, and if we can get rid of it now and not have GC affected behaviour, then all the better.
Consider a normal ContextAwareAction, this means a ContextAwareAction without state. In that case the IDE can spin up a huge number of ContextAction/Performer combinations. The instDelegate is only created when the action is actually being invoked. If that action now is "big" (for example because their target is huge), having it in a WeakReference allows it to be removed.
Consider a normal
ContextAwareAction, this means aContextAwareActionwithout state. In that case the IDE can spin up a huge number ofContextAction/Performercombinations. TheinstDelegateis only created when the action is actually being invoked. If that action now is "big" (for example because their target is huge),
Well, "big" could mean a lot, including "big" to compute. But what do you mean by "target"? If that's in the lookup context then surely it's held by a strong reference anyway? Isn't the performer chain meant to invalidate and clear anyway when that reference goes from the context?
I'm happy with the change you have, and it's probably the safer option. I retain a concern we're potentially masking issues with GC effects.
@matthiasblaesing is it possible to give me some time (~ 1-2 weeks) so I can try to supply more tests based on IdealGraphVisualizer stateful graph actions, which were one of the reasons for introducing better action state ?
@matthiasblaesing is it possible to give me some time (~ 1-2 weeks) so I can try to supply more tests based on IdealGraphVisualizer stateful graph actions, which were one of the reasons for introducing better action state ?
Sure - no problem.
The changes look OK to me, reviewing each commit by itself. But the logic is complicated enough that I feel like correctness can only be ensured by manually testing it for some time.
(I'm still waiting for https://issues.apache.org/jira/browse/NETBEANS-4754 to reappear without this PR or my earlier patch first. I have not yet been able to reproduce it... but I remember it was dependent on timing, memory conditions, and the phase of the moon...)
(I'm still waiting for https://issues.apache.org/jira/browse/NETBEANS-4754 to reappear without this PR or my earlier patch first. I have not yet been able to reproduce it... but I remember it was dependent on timing, memory conditions, and the phase of the moon...)
That bothers me somewhat given how reliably reproducible the bug with the terminal action is. Be good to know if you get to the bottom of the steps that trigger it.
Yeah, I'm not sure why I can't reproduce it anymore. I had reproduction steps written up from 2020, and I tried building the same version of NetBeans and my platform app, and use the same Java version and laptop. Maybe I made a mistake somewhere... I'll have to try again.
You need to have a situation where the same action is invoked with different lookups. For the TerminalAction this is trivial as the actions are created everytime a popup menu is created for the terminals. Thus it is nearly instantatious.
https://github.com/apache/netbeans/blob/b332b296700219ecd1d6ee6255124f3486171af3/ide/terminal.nb/src/org/netbeans/modules/terminal/ioprovider/Terminal.java#L1038-L1097
with
https://github.com/apache/netbeans/blob/b332b296700219ecd1d6ee6255124f3486171af3/platform/openide.util.ui/src/org/openide/util/Utilities.java#L1819-L1899
@matthiasblaesing I have not been able to make the original NETBEANS-4754 bug resurface yet. I was hoping it would pop up randomly after I removed my old patch, but I will need to go back to some more targeted attempts at reproducing it (including finding the same old Java version etc.).
Having heared no objections, I plan to merge this early next week. If anyone wants to object, please do so now.
@matthiasblaesing I'd merge earlier if for NB23 given freeze was meant to be this week???
I'm switch my working IDE and platform app to running this patch now, for testing purposes. I don't mind the patch being merged; even if something like NETBEANS-4754 shows up again, it'll be easier to debug again with the cleaned-up code.
@neilcsmith-net thanks for the reminder. Ok, I'll give it another 24hours and merge then.
this did also repair the "pin tab" feature. I can now open multiple terminals, pin them and when NB is restarted it will restore them again -> great fix!
I can also report that after 3.5 months of usage, in my platform app and my working IDE, I have no encountered no new problems relating to this patch (which replaced my older hack for fixing the elusive NETBEANS-4754).