[GitHub] [netbeans] neilcsmith-net commented on a diff in pull request #5361: Various database fixes

["neilcsmith-net (via GitHub)" <gi...@apache.org>]

neilcsmith-net commented on code in PR #5361:
URL: https://github.com/apache/netbeans/pull/5361#discussion_r1088769727


##########
ide/db/src/org/netbeans/modules/db/explorer/action/ConnectAction.java:
##########
@@ -62,63 +63,63 @@
 import org.openide.awt.ActionReference;
 import org.openide.awt.ActionRegistration;
 import org.openide.awt.ActionState;
-import org.openide.awt.StatusDisplayer;
+import org.openide.util.ContextAwareAction;
 import org.openide.util.HelpCtx;
 import org.openide.util.Lookup;
 import org.openide.util.Mutex;
 import org.openide.util.NbBundle;
+import org.openide.util.RequestProcessor;
+import org.openide.util.WeakListeners;
 
 @ActionRegistration(
         displayName = "#Connect", 
-        lazy = false,
+        lazy = true,
+        asynchronous = true,
         enabledOn = @ActionState(type = DatabaseConnection.class, useActionInstance = true)
 )
 @ActionID(category = "Database", id = "netbeans.db.explorer.action.Connect")
 @ActionReference(path = "Databases/Explorer/Connection/Actions", position = 100)
-public class ConnectAction extends BaseAction {
+// Note: the action ought to be asynchronous=true, but that does not work well with lazy = true (and with lazy = false does not work at all)

Review Comment:
   This comment seems to say the opposite to the changes you've made?!



##########
ide/db/src/org/netbeans/modules/db/explorer/action/ConnectAction.java:
##########
@@ -62,63 +63,63 @@
 import org.openide.awt.ActionReference;
 import org.openide.awt.ActionRegistration;
 import org.openide.awt.ActionState;
-import org.openide.awt.StatusDisplayer;
+import org.openide.util.ContextAwareAction;
 import org.openide.util.HelpCtx;
 import org.openide.util.Lookup;
 import org.openide.util.Mutex;
 import org.openide.util.NbBundle;
+import org.openide.util.RequestProcessor;
+import org.openide.util.WeakListeners;
 
 @ActionRegistration(
         displayName = "#Connect", 
-        lazy = false,
+        lazy = true,
+        asynchronous = true,

Review Comment:
   A pet hate pattern of mine - IMO actions should always be invoked on the EDT and then explicitly async when needed.
   
   Are there implications to changing the behaviour of an existing action here?



##########
ide/db/src/org/netbeans/modules/db/explorer/action/ConnectAction.java:
##########
@@ -161,10 +162,18 @@ public void exceptionOccurred(Exception exc) {
 
         private static HelpCtx CONNECT_ACTION_HELPCTX = new HelpCtx(ConnectAction.class);
 
+        /*
         public void showDialog(final ConnectionNode model, boolean showDialog) {
             DatabaseConnection dbcon = model.getLookup().lookup(DatabaseConnection.class);
-            showDialog(dbcon, showDialog);
+            // in headless mode EDT makes no sense; but we should not block the main thread.
+            // in EDT, the nested dialog will make a nested event loop, so OK.
+            if (GraphicsEnvironment.isHeadless()) {
+                RequestProcessor.getDefault().post(() -> showDialog(dbcon, showDialog));
+            } else {
+                showDialog(dbcon, showDialog);
+            }
         }
+        */

Review Comment:
   Is this meant to be left in?



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org

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