RFR: JDK-8293364: IGV: Refactor Action in EditorTopComponent and fix minor bugs [v8]
Christian Hagedorn
chagedorn at openjdk.org
Mon Sep 19 10:14:58 UTC 2022
On Mon, 19 Sep 2022 07:35:57 GMT, Tobias Holenstein <tholenstein at openjdk.org> wrote:
>> Refactor the Actions in EditorTopComponent (com.sun.hotspot.igv.view.actions). Move Action specific code from EditorTopComponent to the corresponding Action.
>>
>> # Refactoring of com.sun.hotspot.igv.view.actions and EditorTopComponent
>> - Created a new `ExportGraph` Action and moved corresponding functions `exportToSVG(..)` and `exportToPDF(..)` to new `ExportGraph.java`
>> - Moved key bindings for satellite-view (pressing S) from `EditorTopComponent` to `OverviewAction.java`
>> - Moved Action specific code from `EditorTopComponent` to the corresponding `XXXAction.java`
>> - Changed `PrevDiagramAction`, `ExpandDiffAction`, `ExtractAction`, `HideAction`, `NextDiagramAction`, `ReduceDiffAction` and `ShowAllAction` to be context aware `ContextAction<DiagramViewModel>` actions and use more modern `@ActionRegistration` to move away from manually defining actions in `layer.xml`
>> - new `addContextListener` / `removeContextListener` function in `ContextAction<T>` enables context aware actions to define to which `ChangedEvent` they want to react to
>>
>> # Fixing minor Bugs
>> - "Show empty blocks in control-flow graph view" is selected by default but only enabled in CFG view.
>> This is distracting for the eye when we are not in CFG:
>> <img width="192" alt="cfg_before" src="https://user-images.githubusercontent.com/71546117/188463472-b928075f-a687-46d6-a871-35f287e943c7.png">
>> Now "Show empty blocks in control-flow graph view" is not selected anymore when disabled (greyed out)
>> <img width="192" alt="cfg_node_disable" src="https://user-images.githubusercontent.com/71546117/188463809-c998b03e-cc2f-48c1-8f5f-135edffa9c8c.png">
>> But still gets selected by default when enabled
>> <img width="197" alt="cfg_now" src="https://user-images.githubusercontent.com/71546117/188463853-33e6e7cb-b8ff-4c75-a522-63fd858b42ef.png">
>>
>> - "Extract current set of selected nodes", "Hide selected nodes" and "show all nodes" were always enabled, even when they didn't effect anything.
>> <img width="99" alt="selection_before" src="https://user-images.githubusercontent.com/71546117/188464555-45ef06af-b247-46b0-aaa2-4c2397566f8c.png">
>> Now "Extract current set of selected nodes", "Hide selected nodes" are disabled (greyed out) when no nodes are selected. And "show all nodes" is disabled (greyed out) when all nodes are already visible.
>> <img width="91" alt="selection_now" src="https://user-images.githubusercontent.com/71546117/188464589-61310b13-ba58-45a9-b2d8-d3be0da582ab.png">
>>
>> - "Reduce the difference selection" got stuck when at the last graphs in the group because it got greyed out.
>> <img width="130" alt="reduce_stuck" src="https://user-images.githubusercontent.com/71546117/188465260-6fca7ca6-3e8e-4bd9-9b70-4ac19d79eff5.png">
>> duce the difference selection"
>> Now "Reduce the difference selection" works as expected:
>> <img width="152" alt="reduce_now" src="https://user-images.githubusercontent.com/71546117/188465314-bce441ba-20d6-4e3d-ba4e-ff1f4a79cd9d.png">
>
> Tobias Holenstein has updated the pull request incrementally with one additional commit since the last revision:
>
> undo removing variable generated by form editor
I only have some code style specific comments. Otherwise, good cleanup!
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/DiagramViewModel.java line 188:
> 186: public void setHideDuplicates(boolean b) {
> 187: InputGraph currentGraph = getFirstGraph();
> 188: if (b) {
I suggest to rename `b` to `hideDuplicates`.
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/EditorTopComponent.java line 171:
> 169: }
> 170:
> 171: };
Could be changed into using a lambda:
Suggestion:
ChangedListener<DiagramViewModel> diagramChangedListener = source -> {
updateDisplayName();
Collection<Object> list = new ArrayList<>();
list.add(new EditorInputGraphProvider(EditorTopComponent.this));
graphContent.set(list, null);
diagramProvider.getChangedEvent().fire();
};
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/EditorTopComponent.java line 268:
> 266:
> 267: public DiagramViewModel getModel() {
> 268: return scene.getModel();
Suggestion:
return scene.getModel();
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/EditorTopComponent.java line 275:
> 273: }
> 274:
> 275: public void selectionMode(boolean b) {
I suggest to rename it to `setSelectionMode` as it otherwise suggests being a getter.
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/EditorTopComponent.java line 306:
> 304:
> 305: public static EditorTopComponent getActive() {
> 306: TopComponent topComponent = EditorTopComponent.getRegistry().getActivated();
Can be simplified to:
Suggestion:
TopComponent topComponent = getRegistry().getActivated();
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/EditorTopComponent.java line 379:
> 377:
> 378: @Override
> 379: public void componentOpened() { }
Is this empty override required? Otherwise, the method can be removed.
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/actions/EnableBlockLayoutAction.java line 40:
> 38: * @author Thomas Wuerthinger
> 39: */
> 40: public class EnableBlockLayoutAction extends AbstractAction implements PropertyChangeListener {
`EnableBlockLayoutAction`, `EnableCFGLayoutAction` and `EnableSeaLayoutAction` are very similar and could share some code. You could add a super class `EnableLayoutAction` (or any other name that fits) for them and put everything that's shared into it.
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/actions/EnableBlockLayoutAction.java line 42:
> 40: public class EnableBlockLayoutAction extends AbstractAction implements PropertyChangeListener {
> 41:
> 42: EditorTopComponent editor;
Should be made `private` (and also `final`):
Suggestion:
private final EditorTopComponent editor;
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/actions/ExpandPredecessorsAction.java line 44:
> 42: EditorTopComponent editor = EditorTopComponent.getActive();
> 43: if (editor != null) {
> 44: Set<Figure> oldSelection = editor.getModel().getSelectedFigures();
This code looks very similar to the code in `ExpandSuccessorAction`. The only difference is `f.getSuccessors()` vs `f.getPredecessors()`. Moreover, the only other difference between this class and `ExpandSuccessorAction` seems to be `getName()`. I suggest to create a super class `ExpandAdjacentAction` (or another name that fits) for these two classes and have a common `expandAdjacent()` method (or another name that fits) for this code that just takes the predecessors or successors as parameter.
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/actions/ReduceDiffAction.java line 99:
> 97: model.getViewPropertiesChangedEvent().removeListener(this);
> 98: model.getHiddenNodesChangedEvent().removeListener(this);
> 99: }
These three methods seem to be shared among all subclasses. Is it possible to move them to the super class?
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/actions/ReduceDiffAction.java line 104:
> 102: public Action createContextAwareInstance(Lookup actionContext) {
> 103: return this;
> 104: }
It seems you are not really creating instances anymore by looking at all the new overrides of `createContextAwareInstance()` which just return `this`. Is this method still required or can the usages be replaced by directly using the caller object?
-------------
Changes requested by chagedorn (Reviewer).
PR: https://git.openjdk.org/jdk/pull/10170
More information about the hotspot-compiler-dev
mailing list