From c625da90a4bf060330fb540fb384af0df1d6dede Mon Sep 17 00:00:00 2001 From: ghidravore Date: Wed, 3 Nov 2021 16:20:43 -0400 Subject: [PATCH] GP-1440 fixing issue global actions shared across windows getting the wrong context --- .../main/java/docking/ActionToGuiMapper.java | 10 +- .../java/docking/ComponentPlaceholder.java | 17 ++- .../java/docking/DockingWindowManager.java | 37 +++--- .../docking/GlobalMenuAndToolBarManager.java | 120 +++++++++++++----- .../java/docking/WindowActionManager.java | 91 +++++-------- .../util/task/AbstractSwingUpdateManager.java | 2 +- 6 files changed, 157 insertions(+), 120 deletions(-) diff --git a/Ghidra/Framework/Docking/src/main/java/docking/ActionToGuiMapper.java b/Ghidra/Framework/Docking/src/main/java/docking/ActionToGuiMapper.java index 87d7fa763b..71d61d41ed 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/ActionToGuiMapper.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/ActionToGuiMapper.java @@ -100,7 +100,7 @@ public class ActionToGuiMapper { void update() { menuAndToolBarManager.update(); - contextChangedAll(); + contextChanged(); } void dispose() { @@ -117,12 +117,8 @@ public class ActionToGuiMapper { return menuBarMenuHandler; } - void contextChangedAll() { - menuAndToolBarManager.contextChangedAll(); - } - - void contextChanged(ComponentPlaceholder placeHolder) { - menuAndToolBarManager.contextChanged(placeHolder); + void contextChanged() { + menuAndToolBarManager.contextChanged(); } PopupActionManager getPopupActionManager() { diff --git a/Ghidra/Framework/Docking/src/main/java/docking/ComponentPlaceholder.java b/Ghidra/Framework/Docking/src/main/java/docking/ComponentPlaceholder.java index e3b48544e4..9008c949ae 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/ComponentPlaceholder.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/ComponentPlaceholder.java @@ -159,7 +159,7 @@ public class ComponentPlaceholder { return group; } - DetachedWindowNode getWindowNode() { + DetachedWindowNode getDetachedWindowNode() { Node node = compNode.parent; while (node != null) { if (node instanceof DetachedWindowNode) { @@ -170,6 +170,17 @@ public class ComponentPlaceholder { return null; } + WindowNode getWindowNode() { + Node node = compNode.parent; + while (node != null) { + if (node instanceof WindowNode) { + return (WindowNode) node; + } + node = node.parent; + } + return null; + } + WindowNode getTopLevelNode() { if (compNode == null) { return null; @@ -279,9 +290,9 @@ public class ComponentPlaceholder { // makes sure that the given window is not in an iconified state private void activateWindow() { - DetachedWindowNode windowNode = getWindowNode(); + DetachedWindowNode windowNode = getDetachedWindowNode(); if (windowNode != null) { - Window window = getWindowNode().getWindow(); + Window window = getDetachedWindowNode().getWindow(); if (window instanceof Frame) { Frame frame = (Frame) window; frame.setState(Frame.NORMAL); diff --git a/Ghidra/Framework/Docking/src/main/java/docking/DockingWindowManager.java b/Ghidra/Framework/Docking/src/main/java/docking/DockingWindowManager.java index 28a5a31214..f06f1712ef 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/DockingWindowManager.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/DockingWindowManager.java @@ -630,7 +630,7 @@ public class DockingWindowManager implements PropertyChangeListener, Placeholder placeholder.update(); scheduleUpdate(); - DetachedWindowNode wNode = placeholder.getWindowNode(); + DetachedWindowNode wNode = placeholder.getDetachedWindowNode(); if (wNode != null) { wNode.updateTitle(); } @@ -2225,19 +2225,16 @@ public class DockingWindowManager implements PropertyChangeListener, Placeholder } public void contextChanged(ComponentProvider provider) { - - if (provider == null) { - actionToGuiMapper.contextChangedAll(); // this updates the actions for all windows - return; + // if provider is specified, update its local menu and tool bar actions + if (provider != null) { + ComponentPlaceholder placeholder = getActivePlaceholder(provider); + if (placeholder != null) { + placeholder.contextChanged(); + } } - ComponentPlaceholder placeholder = getActivePlaceholder(provider); - if (placeholder == null) { - return; - } - - placeholder.contextChanged(); - actionToGuiMapper.contextChanged(placeholder); + // always update the global tool menu and tool bar actions + actionToGuiMapper.contextChanged(); } /** @@ -2326,12 +2323,16 @@ public class DockingWindowManager implements PropertyChangeListener, Placeholder } - void notifyContextListeners(ComponentPlaceholder placeHolder, ActionContext actionContext) { - - if (placeHolder == focusedPlaceholder) { - for (DockingContextListener listener : contextListeners) { - listener.contextChanged(actionContext); - } + /** + * This call will notify any context listeners that the context has changed. + * + *

Our {@link #contextChanged(ComponentProvider)} method will eventually call back into this + * method after any buffering has taken place. + * @param context the context + */ + void doContextChanged(ActionContext context) { + for (DockingContextListener listener : contextListeners) { + listener.contextChanged(context); } } diff --git a/Ghidra/Framework/Docking/src/main/java/docking/GlobalMenuAndToolBarManager.java b/Ghidra/Framework/Docking/src/main/java/docking/GlobalMenuAndToolBarManager.java index fd2096b8ce..60b793c70f 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/GlobalMenuAndToolBarManager.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/GlobalMenuAndToolBarManager.java @@ -21,7 +21,12 @@ import docking.action.DockingActionIf; import docking.menu.MenuGroupMap; import docking.menu.MenuHandler; import ghidra.util.Swing; +import ghidra.util.task.AbstractSwingUpdateManager; +import ghidra.util.task.SwingUpdateManager; +/** + * Class to manage all the global actions that show up on the main tool menubar or toolbar + */ public class GlobalMenuAndToolBarManager implements DockingWindowListener { private Map windowToActionManagerMap; @@ -29,6 +34,12 @@ public class GlobalMenuAndToolBarManager implements DockingWindowListener { private final MenuGroupMap menuGroupMap; private final DockingWindowManager windowManager; + // set the max delay low enough so that users can't interact with out-of-date actions before + // they get updated + private SwingUpdateManager updateManager = + new SwingUpdateManager(AbstractSwingUpdateManager.DEFAULT_MIN_DELAY, 500, + "Context Update Manager", () -> updateActions()); + public GlobalMenuAndToolBarManager(DockingWindowManager windowManager, MenuHandler menuHandler, MenuGroupMap menuGroupMap) { @@ -77,11 +88,15 @@ public class GlobalMenuAndToolBarManager implements DockingWindowListener { } public void dispose() { - for (WindowActionManager actionManager : windowToActionManagerMap.values()) { - actionManager.dispose(); - } - windowToActionManagerMap.clear(); - + // make sure this is on the swing thread to avoid clearing stuff while the swing update + // manager is firing its processContextChanged() call + Swing.runIfSwingOrRunLater(() -> { + updateManager.dispose(); + for (WindowActionManager actionManager : windowToActionManagerMap.values()) { + actionManager.dispose(); + } + windowToActionManagerMap.clear(); + }); } @Override @@ -113,13 +128,7 @@ public class GlobalMenuAndToolBarManager implements DockingWindowListener { @Override public void dockingWindowFocusChanged(WindowNode windowNode) { - //update global menus and toolbars for this window - ComponentPlaceholder lastFocused = windowNode.getLastFocusedProviderInWindow(); - WindowActionManager windowActionManager = windowToActionManagerMap.get(windowNode); - if (windowActionManager == null) { - return; - } - windowActionManager.contextChanged(lastFocused); + updateManager.updateLater(); } private void removeWindowActionManager(WindowNode windowNode) { @@ -135,8 +144,7 @@ public class GlobalMenuAndToolBarManager implements DockingWindowListener { new WindowActionManager(windowNode, menuHandler, windowManager, menuGroupMap); windowToActionManagerMap.put(windowNode, newActionManager); newActionManager.setActions(actionsForWindow); - ComponentPlaceholder lastFocused = windowNode.getLastFocusedProviderInWindow(); - newActionManager.contextChanged(lastFocused); + updateManager.updateLater(); } private List getActionsForWindow(WindowNode windowNode) { @@ -152,34 +160,80 @@ public class GlobalMenuAndToolBarManager implements DockingWindowListener { return actionsForWindow; } - public void contextChangedAll() { - Swing.runIfSwingOrRunLater(this::updateAllWindowActions); + public void contextChanged() { + // schedule an update for all the global actions + updateManager.updateLater(); } - private void updateAllWindowActions() { + private void updateActions() { + + // + // The focused window's actions must be notified after all other windows in order to + // prevent incorrect context updates. We will first update all non-focused windows, + // then the focused window and then finally tell the Docking Window Manager to update. + // + WindowNode focusedWindowNode = getFocusedWindowNode(); + Set focusedWindowActions = getWindowActions(focusedWindowNode); + + ActionContext globalContext = windowManager.getDefaultToolContext(); for (WindowNode windowNode : windowToActionManagerMap.keySet()) { - ComponentPlaceholder lastFocused = windowNode.getLastFocusedProviderInWindow(); - windowToActionManagerMap.get(windowNode).contextChanged(lastFocused); + if (windowNode == focusedWindowNode) { + continue; // the focused window will be called after this loop later + } + + WindowActionManager actionManager = windowToActionManagerMap.get(windowNode); + ActionContext localContext = getContext(windowNode); + actionManager.contextChanged(globalContext, localContext, focusedWindowActions); + } + + // now update the focused window's actions + WindowActionManager actionManager = windowToActionManagerMap.get(focusedWindowNode); + ActionContext focusedContext = getContext(focusedWindowNode); + if (actionManager != null) { + actionManager.contextChanged(globalContext, focusedContext, Collections.emptySet()); + } + + // update the docking window manager ; no focused context when no window is focused + if (focusedContext != null) { + windowManager.doContextChanged(focusedContext); } } - public void contextChanged(ComponentPlaceholder placeHolder) { - if (placeHolder == null) { - return; + private ActionContext getContext(WindowNode windowNode) { + if (windowNode == null) { + return null; } - WindowNode topLevelNode = placeHolder.getTopLevelNode(); - if (topLevelNode == null) { - return; + ActionContext context = null; + ComponentPlaceholder placeholder = windowNode.getLastFocusedProviderInWindow(); + if (placeholder != null) { + ComponentProvider provider = placeholder.getProvider(); + if (provider != null) { + context = provider.getActionContext(null); + } } - - if (topLevelNode.getLastFocusedProviderInWindow() != placeHolder) { - return; // actions in this window are not currently responding to this provider - } - - WindowActionManager windowActionManager = windowToActionManagerMap.get(topLevelNode); - if (windowActionManager != null) { - windowActionManager.contextChanged(placeHolder); + if (context == null) { + context = new ActionContext(); } + return context; } + + private Set getWindowActions(WindowNode windowNode) { + if (windowNode != null) { + WindowActionManager windowActionManager = windowToActionManagerMap.get(windowNode); + if (windowActionManager != null) { + return windowActionManager.getOriginalActions(); + } + } + return Collections.emptySet(); + } + + private WindowNode getFocusedWindowNode() { + ComponentPlaceholder focusedComponent = windowManager.getFocusedComponent(); + if (focusedComponent == null) { + return null; + } + return focusedComponent.getWindowNode(); + } + } diff --git a/Ghidra/Framework/Docking/src/main/java/docking/WindowActionManager.java b/Ghidra/Framework/Docking/src/main/java/docking/WindowActionManager.java index 356c38ede6..d518fbea97 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/WindowActionManager.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/WindowActionManager.java @@ -21,7 +21,6 @@ import javax.swing.JMenuBar; import docking.action.DockingActionIf; import docking.menu.*; -import ghidra.util.task.SwingUpdateManager; public class WindowActionManager { private Map actionToProxyMap; @@ -29,25 +28,18 @@ public class WindowActionManager { private ToolBarManager toolBarMgr; private final WindowNode node; - private final DockingWindowManager winMgr; private boolean disposed; - private ComponentPlaceholder placeHolderForScheduledActionUpdate; - private Runnable updateActionsRunnable = () -> processContextChanged(); - private SwingUpdateManager updateManager = - new SwingUpdateManager(500, 500, "Context Update Manager", updateActionsRunnable); - - public WindowActionManager(WindowNode node, MenuHandler menuBarHandler, + WindowActionManager(WindowNode node, MenuHandler menuBarHandler, DockingWindowManager winMgr, MenuGroupMap menuGroupMap) { this.node = node; - this.winMgr = winMgr; actionToProxyMap = new HashMap<>(); menuBarMgr = new MenuBarManager(menuBarHandler, menuGroupMap); toolBarMgr = new ToolBarManager(winMgr); } - public void setActions(List actionList) { + void setActions(List actionList) { menuBarMgr.clearActions(); toolBarMgr.clearActions(); actionToProxyMap.clear(); @@ -56,7 +48,7 @@ public class WindowActionManager { } } - public void addAction(DockingActionIf action) { + void addAction(DockingActionIf action) { if (action.getMenuBarData() != null || action.getToolBarData() != null) { DockingActionProxy proxyAction = new DockingActionProxy(action); actionToProxyMap.put(action, proxyAction); @@ -65,7 +57,7 @@ public class WindowActionManager { } } - public void removeAction(DockingActionIf action) { + void removeAction(DockingActionIf action) { DockingActionProxy proxyAction = actionToProxyMap.remove(action); if (proxyAction != null) { menuBarMgr.removeAction(proxyAction); @@ -73,11 +65,11 @@ public class WindowActionManager { } } - public DockingActionIf getToolbarAction(String actionName) { + DockingActionIf getToolbarAction(String actionName) { return toolBarMgr.getAction(actionName); } - public void update() { + void update() { JMenuBar menuBar = menuBarMgr.getMenuBar(); if (menuBar.getMenuCount() > 0) { node.setMenuBar(menuBar); @@ -87,9 +79,8 @@ public class WindowActionManager { node.validate(); } - public synchronized void dispose() { + void dispose() { disposed = true; - updateManager.dispose(); node.setToolBar(null); node.setMenuBar(null); actionToProxyMap.clear(); @@ -97,48 +88,32 @@ public class WindowActionManager { toolBarMgr.dispose(); } - synchronized void contextChanged(ComponentPlaceholder placeHolder) { + void contextChanged(ActionContext globalContext, ActionContext localContext, + Set excluded) { - if (!node.isVisible()) { + if (!node.isVisible() || disposed) { return; } - placeHolderForScheduledActionUpdate = placeHolder; - - // Typically, when we get one contextChanged, we get a flurry of contextChanged calls. - // In order to make the action updating be as responsive as possible and still be complete, - // we have chosen a policy that will reduce a flurry of contextChanged call into two - // actual calls - one that occurs immediately and one when the flurry times out. - updateManager.update(); - } - - private synchronized void processContextChanged() { - // - // This method is called from an invokeLater(), which means that we may be - // disposed while before this Swing call executes. - // - if (disposed) { - return; - } - - ActionContext localContext = getContext(); - ActionContext globalContext = winMgr.getDefaultToolContext(); - - // Update actions - make a copy so that we don't get concurrent modification exceptions - List list = new ArrayList<>(actionToProxyMap.values()); + // Update actions - make a copy so that we don't get concurrent modification + // exceptions during reentrant operations + List list = new ArrayList<>(actionToProxyMap.keySet()); for (DockingActionIf action : list) { - if (action.isValidContext(localContext)) { - action.setEnabled(action.isEnabledForContext(localContext)); + if (excluded.contains(action)) { + continue; } - else if (isValidDefaultToolContext(action, globalContext)) { - action.setEnabled(action.isEnabledForContext(globalContext)); + + DockingActionIf proxyAction = actionToProxyMap.get(action); + if (proxyAction.isValidContext(localContext)) { + proxyAction.setEnabled(proxyAction.isEnabledForContext(localContext)); + } + else if (isValidDefaultToolContext(proxyAction, globalContext)) { + proxyAction.setEnabled(proxyAction.isEnabledForContext(globalContext)); } else { - action.setEnabled(false); + proxyAction.setEnabled(false); } } - // Notify listeners if the context provider is the focused provider - winMgr.notifyContextListeners(placeHolderForScheduledActionUpdate, localContext); } private boolean isValidDefaultToolContext(DockingActionIf action, ActionContext toolContext) { @@ -146,15 +121,15 @@ public class WindowActionManager { action.isValidContext(toolContext); } - private ActionContext getContext() { - ComponentProvider provider = placeHolderForScheduledActionUpdate == null ? null - : placeHolderForScheduledActionUpdate.getProvider(); - - ActionContext context = provider == null ? null : provider.getActionContext(null); - - if (context == null) { - context = new ActionContext(); - } - return context; + /** + * Returns the set of actions for this window. + * + *

Note this returns the the original passed-in actions and not the proxy actions that the + * window uses. + * + * @return the set of actions for this window + */ + Set getOriginalActions() { + return new HashSet<>(actionToProxyMap.keySet()); } } diff --git a/Ghidra/Framework/Generic/src/main/java/ghidra/util/task/AbstractSwingUpdateManager.java b/Ghidra/Framework/Generic/src/main/java/ghidra/util/task/AbstractSwingUpdateManager.java index e1346873bd..cec7216759 100644 --- a/Ghidra/Framework/Generic/src/main/java/ghidra/util/task/AbstractSwingUpdateManager.java +++ b/Ghidra/Framework/Generic/src/main/java/ghidra/util/task/AbstractSwingUpdateManager.java @@ -55,7 +55,7 @@ public abstract class AbstractSwingUpdateManager { protected static final long NONE = 0; public static final int DEFAULT_MAX_DELAY = 30000; protected static final int MIN_DELAY_FLOOR = 10; - protected static final int DEFAULT_MIN_DELAY = 250; + public static final int DEFAULT_MIN_DELAY = 250; protected static final String DEFAULT_NAME = AbstractSwingUpdateManager.class.getSimpleName(); private static final WeakSet instances = WeakDataStructureFactory.createCopyOnReadWeakSet();