From b0769bd805b494b4d06d213180d565170ab2ed0a Mon Sep 17 00:00:00 2001 From: dragonmacher <48328597+dragonmacher@users.noreply.github.com> Date: Thu, 11 Feb 2021 13:29:14 -0500 Subject: [PATCH] GP-672 - fixed 'wait forever' bug --- .../visualization/DefaultGraphDisplay.java | 23 +---- .../graph/visualization/SetLayoutTask.java | 43 +++++++--- .../docking/action/ToggleDockingAction.java | 5 +- .../builder/MultiStateActionBuilder.java | 29 ++++--- .../docking/menu/MultiStateDockingAction.java | 83 ++++++++++++++----- .../MultipleActionDockingToolbarButton.java | 13 ++- 6 files changed, 120 insertions(+), 76 deletions(-) diff --git a/Ghidra/Features/GraphServices/src/main/java/ghidra/graph/visualization/DefaultGraphDisplay.java b/Ghidra/Features/GraphServices/src/main/java/ghidra/graph/visualization/DefaultGraphDisplay.java index 41a21ce1e2..d2d345fcc9 100644 --- a/Ghidra/Features/GraphServices/src/main/java/ghidra/graph/visualization/DefaultGraphDisplay.java +++ b/Ghidra/Features/GraphServices/src/main/java/ghidra/graph/visualization/DefaultGraphDisplay.java @@ -379,7 +379,7 @@ public class DefaultGraphDisplay implements GraphDisplay { new MultiStateActionBuilder("Arrangement", ACTION_OWNER) .description("Arrangement: " + layoutActionStates.get(0).getName()) .toolBarIcon(DefaultDisplayGraphIcons.LAYOUT_ALGORITHM_ICON) - .fireFirstAction(false) + .useCheckboxForIcons(true) .onActionStateChanged((s, t) -> layoutChanged(s.getName())) .addStates(layoutActionStates) .buildAndInstallLocal(componentProvider); @@ -701,11 +701,6 @@ public class DefaultGraphDisplay implements GraphDisplay { } } - /** - * get a {@code List} of {@code ActionState} buttons for the - * configured layout algorithms - * @return a {@code List} of {@code ActionState} buttons - */ private List> getLayoutActionStates() { String[] names = layoutTransitionManager.getLayoutNames(); List> actionStates = new ArrayList<>(); @@ -718,20 +713,10 @@ public class DefaultGraphDisplay implements GraphDisplay { return actionStates; } - /** - * respond to a change in the layout name - * @param layoutName the name of the layout algorithm to apply - */ private void layoutChanged(String layoutName) { - if (layoutTransitionManager != null) { - new TaskLauncher(new SetLayoutTask(viewer, layoutTransitionManager, layoutName), null, - 1000); - } + TaskLauncher.launch(new SetLayoutTask(viewer, layoutTransitionManager, layoutName)); } - /** - * show the dialog with generated filters - */ private void showFilterDialog() { if (filterDialog == null) { if (vertexFilters == null) { @@ -743,10 +728,6 @@ public class DefaultGraphDisplay implements GraphDisplay { componentProvider.getTool().showDialog(filterDialog); } - /** - * add or remove the satellite viewer - * @param context information about the event - */ private void toggleSatellite(ActionContext context) { boolean selected = ((AbstractButton) context.getSourceObject()).isSelected(); graphDisplayProvider.setDefaultSatelliteState(selected); diff --git a/Ghidra/Features/GraphServices/src/main/java/ghidra/graph/visualization/SetLayoutTask.java b/Ghidra/Features/GraphServices/src/main/java/ghidra/graph/visualization/SetLayoutTask.java index a3745d21f7..dbf54fc9ed 100644 --- a/Ghidra/Features/GraphServices/src/main/java/ghidra/graph/visualization/SetLayoutTask.java +++ b/Ghidra/Features/GraphServices/src/main/java/ghidra/graph/visualization/SetLayoutTask.java @@ -17,6 +17,7 @@ package ghidra.graph.visualization; import java.util.concurrent.CountDownLatch; +import org.jgrapht.Graph; import org.jungrapht.visualization.VisualizationModel; import org.jungrapht.visualization.VisualizationViewer; import org.jungrapht.visualization.layout.event.LayoutStateChange.*; @@ -24,6 +25,7 @@ import org.jungrapht.visualization.layout.model.LayoutModel; import ghidra.service.graph.AttributedEdge; import ghidra.service.graph.AttributedVertex; +import ghidra.util.Msg; import ghidra.util.Swing; import ghidra.util.exception.CancelledException; import ghidra.util.task.*; @@ -48,8 +50,8 @@ public class SetLayoutTask extends Task { @Override public void run(TaskMonitor monitor) throws CancelledException { - // add a callback for when/if the user cancels the layout, use a variable cause - // monitor uses a weak listener list and it would othewise get garbage collected. + // add a callback for when/if the user cancels the layout, use a variable because + // monitor uses a weak listener list and it would otherwise get garbage collected. CancelledListener cancelListener = this::taskCancelled; monitor.addCancelledListener(cancelListener); @@ -60,16 +62,32 @@ public class SetLayoutTask extends Task { Listener listener = this::layoutStateChanged; support.addLayoutStateChangeListener(listener); - // start the layout - needs to be done on swing thread to prevent issues and intermediate + // start the layout - needs to be done on swing thread to prevent issues and intermediate // paints - should be changed in the future to not require it to be on the swing thread. Swing.runNow(() -> layoutTransitionManager.setLayout(layoutName)); - // some of the layouts are done on the calling thread and some aren't. If they are on + waitForLayoutTransition(model); + + support.removeLayoutStateChangeListener(listener); + monitor.removeCancelledListener(cancelListener); + } + + private void waitForLayoutTransition( + VisualizationModel model) { + + Graph graph = model.getGraph(); + if (graph.vertexSet().isEmpty()) { + // note: the underlying graph API will not notify us of the layout state change if the + // graph is empty, so do not wait. + return; + } + + // some of the layouts are done on the calling thread and some aren't. If they are on // the calling thread, then by now, we already got the "done" callback and the "taskDone" // countdown latch has been triggered and won't wait. If, however, the layout has been // diverted to another thread, we want to wait until the layout is completed // There are two ways the latch will be triggered, the layout is completed or the user - // cancles the layout. + // cancels the layout. try { taskDone.await(); } @@ -77,17 +95,17 @@ public class SetLayoutTask extends Task { model.getLayoutAlgorithm().cancel(); } - // clean up the listeners - support.removeLayoutStateChangeListener(listener); - monitor.removeCancelledListener(cancelListener); } /** - * Notfication when the layout algorithm starts and stops. - * @param e the event. If the event.active is true, then the - * algorithm is starting, if false, the algorithm is done. + * Notification when the layout algorithm starts and stops + * @param e the event. If the event.active is true, then the algorithm is starting, if false, + * the algorithm is done. */ private void layoutStateChanged(Event e) { + + Msg.debug(this, "layoutStatechanged(): " + e); + if (!e.active) { // algorithm is done, release the latch taskDone.countDown(); @@ -98,6 +116,9 @@ public class SetLayoutTask extends Task { * Callback if the user cancels the layout */ private void taskCancelled() { + + Msg.debug(this, "taskCancelled()"); + // release the latch and tell the layout algorithm to cancel. taskDone.countDown(); viewer.getVisualizationModel().getLayoutAlgorithm().cancel(); diff --git a/Ghidra/Framework/Docking/src/main/java/docking/action/ToggleDockingAction.java b/Ghidra/Framework/Docking/src/main/java/docking/action/ToggleDockingAction.java index e16802706e..9b7c1e9ecf 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/action/ToggleDockingAction.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/action/ToggleDockingAction.java @@ -19,7 +19,6 @@ import javax.swing.JButton; import javax.swing.JMenuItem; import docking.*; -import docking.menu.DockingCheckboxMenuItemUI; public abstract class ToggleDockingAction extends DockingAction implements ToggleDockingActionIf { private boolean isSelected; @@ -59,9 +58,7 @@ public abstract class ToggleDockingAction extends DockingAction implements Toggl @Override protected JMenuItem doCreateMenuItem() { - DockingCheckBoxMenuItem menuItem = new DockingCheckBoxMenuItem(isSelected); - menuItem.setUI((DockingCheckboxMenuItemUI) DockingCheckboxMenuItemUI.createUI(menuItem)); - return menuItem; + return new DockingCheckBoxMenuItem(isSelected); } @Override diff --git a/Ghidra/Framework/Docking/src/main/java/docking/action/builder/MultiStateActionBuilder.java b/Ghidra/Framework/Docking/src/main/java/docking/action/builder/MultiStateActionBuilder.java index 783c9fdea8..86d8f9806c 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/action/builder/MultiStateActionBuilder.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/action/builder/MultiStateActionBuilder.java @@ -34,8 +34,8 @@ public class MultiStateActionBuilder extends AbstractActionBuilder, ActionContext, MultiStateActionBuilder> { private BiConsumer, EventTrigger> actionStateChangedCallback; + private boolean useCheckboxForIcons; private boolean performActionOnButtonClick = false; - private boolean fireFirstAction = true; private List> states = new ArrayList<>(); @@ -80,6 +80,20 @@ public class MultiStateActionBuilder extends return self(); } + /** + * Overrides the default icons for actions shown in popup menu of the multi-state action. By + * default, the popup menu items will use the icons as provided by the {@link ActionState}. + * By passing true to this method, icons will not be used in the popup menu. Instead, a + * checkbox icon will be used to show the active action state. + * + * @param b true to use a checkbox + * @return this MultiActionDockingActionBuilder (for chaining) + */ + public MultiStateActionBuilder useCheckboxForIcons(boolean b) { + this.useCheckboxForIcons = b; + return self(); + } + /** * Add an action state * @@ -115,17 +129,6 @@ public class MultiStateActionBuilder extends return self(); } - /** - * controls whether the first action added will automatically fire an event or not - * - * @param fireFirstAction do fire an action on the first action. Defaults to {@code true} - * @return this MultiActionDockingActionBuilder (for chaining) - */ - public MultiStateActionBuilder fireFirstAction(boolean fireFirstAction) { - this.fireFirstAction = fireFirstAction; - return self(); - } - @Override public MultiStateDockingAction build() { validate(); @@ -145,7 +148,6 @@ public class MultiStateActionBuilder extends } } }; - action.setFireFirstEvent(fireFirstAction); for (ActionState actionState : states) { action.addActionState(actionState); @@ -153,6 +155,7 @@ public class MultiStateActionBuilder extends decorateAction(action); action.setPerformActionOnPrimaryButtonClick(performActionOnButtonClick); + action.setUseCheckboxForIcons(useCheckboxForIcons); return action; } diff --git a/Ghidra/Framework/Docking/src/main/java/docking/menu/MultiStateDockingAction.java b/Ghidra/Framework/Docking/src/main/java/docking/menu/MultiStateDockingAction.java index b7e4e15321..4fc055dff0 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/menu/MultiStateDockingAction.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/menu/MultiStateDockingAction.java @@ -53,9 +53,9 @@ public abstract class MultiStateDockingAction extends DockingAction { private int currentStateIndex = 0; private MultiActionDockingActionIf multiActionGenerator; private MultipleActionDockingToolbarButton multipleButton; - private boolean fireFirstEvent = true; private boolean performActionOnPrimaryButtonClick = true; + private boolean useCheckboxForIcons; // A listener that will get called when the button (not the popup) is clicked. Toolbar // actions do not use this listener. @@ -127,6 +127,18 @@ public abstract class MultiStateDockingAction extends DockingAction { } } + /** + * Overrides the default icons for actions shown in popup menu of the multi-state action. By + * default, the popup menu items will use the icons as provided by the {@link ActionState}. + * By passing true to this method, icons will not be used in the popup menu. Instead, a + * checkbox icon will be used to show the active action state. + * + * @param useCheckboxForIcons true to use a checkbox + */ + public void setUseCheckboxForIcons(boolean useCheckboxForIcons) { + this.useCheckboxForIcons = useCheckboxForIcons; + } + @Override public final void actionPerformed(ActionContext context) { if (!performActionOnPrimaryButtonClick) { @@ -137,21 +149,6 @@ public abstract class MultiStateDockingAction extends DockingAction { doActionPerformed(context); } - /** - * @return {@code true} if the first action automatically fire its event - */ - public boolean isFireFirstEvent() { - return fireFirstEvent; - } - - /** - * set the flag to fire an event on the first action - * @param fireFirstEvent whether to fire the event - */ - public void setFireFirstEvent(boolean fireFirstEvent) { - this.fireFirstEvent = fireFirstEvent; - } - /** * This is the callback to be overridden when the child wishes to respond to user button * presses that are on the button and not the drop-down. This will only be called if @@ -175,9 +172,17 @@ public abstract class MultiStateDockingAction extends DockingAction { } protected List getStateActions() { + ActionState selectedState = actionStates.get(currentStateIndex); List actions = new ArrayList<>(actionStates.size()); for (ActionState actionState : actionStates) { - actions.add(new ActionStateAction(actionState)); + + //@formatter:off + boolean isSelected = actionState == selectedState; + DockingActionIf a = useCheckboxForIcons ? + new ActionStateToggleAction(actionState, isSelected) : + new ActionStateAction(actionState, isSelected); + actions.add(a); + //@formatter:on } return actions; } @@ -199,7 +204,7 @@ public abstract class MultiStateDockingAction extends DockingAction { */ public void addActionState(ActionState actionState) { actionStates.add(actionState); - if (actionStates.size() == 1 && fireFirstEvent) { + if (actionStates.size() == 1) { setCurrentActionState(actionState); } } @@ -209,9 +214,7 @@ public abstract class MultiStateDockingAction extends DockingAction { throw new IllegalArgumentException("You must provide at least one ActionState"); } actionStates = new ArrayList<>(newStates); - if (fireFirstEvent) { - setCurrentActionState(actionStates.get(0)); - } + setCurrentActionState(actionStates.get(0)); } public T getCurrentUserData() { @@ -235,7 +238,7 @@ public abstract class MultiStateDockingAction extends DockingAction { } throw new AssertException( - "Attempted to set an action state by a user type not " + "contained herein: " + t); + "Attempted to set an action state by a user type not contained herein: " + t); } public void setCurrentActionState(ActionState actionState) { @@ -318,13 +321,47 @@ public abstract class MultiStateDockingAction extends DockingAction { //================================================================================================== // Inner Classes //================================================================================================== + + private class ActionStateToggleAction extends ToggleDockingAction { + + private final ActionState actionState; + + private ActionStateToggleAction(ActionState actionState, boolean isSelected) { + super(actionState.getName(), "multiStateAction"); + + this.actionState = actionState; + + setSelected(isSelected); + + setMenuBarData( + new MenuData(new String[] { actionState.getName() })); + HelpLocation helpLocation = actionState.getHelpLocation(); + if (helpLocation != null) { + setHelpLocation(helpLocation); + } + } + + @Override + public String getInceptionInformation() { + // we want the debug info for these internal actions to be that of the outer class + return MultiStateDockingAction.this.getInceptionInformation(); + } + + @Override + public void actionPerformed(ActionContext context) { + setCurrentActionStateWithTrigger(actionState, EventTrigger.GUI_ACTION); + } + + } + private class ActionStateAction extends DockingAction { private final ActionState actionState; - private ActionStateAction(ActionState actionState) { + private ActionStateAction(ActionState actionState, boolean isSelected) { super(actionState.getName(), "multiStateAction"); this.actionState = actionState; + setMenuBarData( new MenuData(new String[] { actionState.getName() }, actionState.getIcon())); HelpLocation helpLocation = actionState.getHelpLocation(); diff --git a/Ghidra/Framework/Docking/src/main/java/docking/menu/MultipleActionDockingToolbarButton.java b/Ghidra/Framework/Docking/src/main/java/docking/menu/MultipleActionDockingToolbarButton.java index 8409488aed..ceef1a3092 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/menu/MultipleActionDockingToolbarButton.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/menu/MultipleActionDockingToolbarButton.java @@ -30,7 +30,7 @@ import docking.*; import docking.action.*; import docking.widgets.EmptyBorderButton; import docking.widgets.label.GDHtmlLabel; -import ghidra.util.SystemUtilities; +import ghidra.util.Swing; import resources.ResourceManager; public class MultipleActionDockingToolbarButton extends EmptyBorderButton { @@ -80,7 +80,8 @@ public class MultipleActionDockingToolbarButton extends EmptyBorderButton { * method will effectively let the user click anywhere on the button or its drop-down arrow * to show the popup menu. During normal operation, the user can only show the popup by * clicking the drop-down arrow. - * + * + * @param performActionOnButtonClick true to perform the action when the button is clicked */ public void setPerformActionOnButtonClick(boolean performActionOnButtonClick) { entireButtonShowsPopupMenu = !performActionOnButtonClick; @@ -160,7 +161,11 @@ public class MultipleActionDockingToolbarButton extends EmptyBorderButton { return manager.getActiveComponentProvider(); } - /** Show a popup containing all the actions below the button */ + /** + * Show a popup containing all the actions below the button + * @param listener for the created popup menu + * @return the popup menu that was shown + */ JPopupMenu showPopup(PopupMenuListener listener) { JPopupMenu menu = new JPopupMenu(); List actionList = multipleAction.getActionList(getActionContext()); @@ -307,7 +312,7 @@ public class MultipleActionDockingToolbarButton extends EmptyBorderButton { // will update the focused window after we click. We need the focus to be // correct before we show, since our menu is built with actions based upon the // focused dude. - SystemUtilities.runSwingLater(() -> popupMenu = showPopup(PopupMouseListener.this)); + Swing.runLater(() -> popupMenu = showPopup(PopupMouseListener.this)); e.consume(); model.setPressed(false);