[GitHub] [netbeans] eirikbakke commented on a change in pull request #652: Initial implementation of @ActionState support

[GitBox <gi...@apache.org>]

eirikbakke commented on a change in pull request #652:
URL: https://github.com/apache/netbeans/pull/652#discussion_r479722069



##########
File path: openide.awt/src/org/openide/awt/ContextAction.java
##########
@@ -181,36 +317,90 @@ public Performer(
             ContextActionEnabler<Data> e
         ) {
             Map<Object, Object> map = new HashMap<Object, Object>();
-            map.put("delegate", p);
-            map.put("enabler", e);
+            map.put("delegate", p); // NOI18N
+            map.put("enabler", e);  // NOI18N
             this.delegate = map;
         }
-
-        @SuppressWarnings("unchecked")
-        public void actionPerformed(
-            ActionEvent ev, List<? extends Data> data, Lookup.Provider everything
-        ) {
-            Object obj = delegate.get("delegate"); // NOI18N
-            if (obj instanceof ContextActionPerformer) {
-                ContextActionPerformer<Data> perf = (ContextActionPerformer<Data>)obj;
-                perf.actionPerformed(ev, data);
-                return;
+        
+        void clear() {
+            stopListeners();
+            Reference r = instDelegate;
+            instDelegate = null;
+            if (r != null) {
+                Object o = r.get();
+                if (o instanceof Performer) {
+                    ((Performer)o).clear();
+                }
             }
-            if (obj instanceof Performer) {
-                Performer<Data> perf = (Performer<Data>)obj;
-                perf.actionPerformed(ev, data, everything);
-                return;
+        }
+        
+        void attach(ContextAction a) {
+            this.owner = new WeakReference<>(a);
+        }
+        
+        /**
+         * Creates a delegate. 
+         * @param everything
+         * @param data
+         * @return 
+         */
+        Object delegate(Lookup.Provider everything, List<?> data) {
+            return delegate0(everything, data, true);
+        }
+        
+        private Object delegate0(Lookup.Provider everything, List<?> data, boolean getAction) {
+            Object d = instDelegate != null ? instDelegate.get() : null;

Review comment:
       There seems to be a bug here, which I bumped into this month in my NetBeans Platform application. It seems that delegate0 will use a cached copy of the instDelegate--but what if the contents of the lookup (the data parameter) has changed?
   
   I see there's a clear() method that's supposed to reset instDelegate in certain cases, but I'm not sure how it works--and it does not seem to be called in all cases where the lookup has changed.
   
   The bug is insidious because instDelegate is a weak reference, so the buggy state disappears on garbage collection.
   
   In my platform application, the action in question is declared like this:
   
   ```
   @ActionID(category = "Ultorg", id = "com.ultorg.actionids.FieldsAction")
   @ActionRegistration(displayName = "#CTL_FieldsAction")
   @Messages("CTL_FieldsAction=Fields & Joins...")
   public class FieldsAction implements ActionListener {
     private final ToolboxInvoker invoker;
   
     public FieldsAction(ToolboxInvoker invoker) {
       Preconditions.checkNotNull(invoker);
       this.invoker = invoker;
     }
   
     @Override
     public void actionPerformed(ActionEvent e) {
       invoker.invoke();
     }
   }
   ```
   
   The ToolboxInvoker is exposed in the lookup of a TopComponent subclass. Invoking the action from the keyboard will occasionally trigger the wrong FieldsAction instance. Once this happens, it will usually happen again and again if the keyboard shortcut is pressed again, until a garbage collection happens (tested by adding a keyboard shortcut to the Garbage Collect action). The cause of the bug seems to be incorrect state kept around in the instDelegate variable (confirmed in the debugger).
   
   Is this the same as the bug described in https://issues.apache.org/jira/browse/NETBEANS-1985 ?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org

For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists