RFR: JDK-8293364: IGV: Refactor Action in EditorTopComponent and fix minor bugs [v5]

[Roberto Castañeda Lozano]

On Thu, 15 Sep 2022 09:06:25 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 refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains two new commits since the last revision:
> 
>  - Update Copyright year
>  - Update OutlineTopComponent.java
>    
>    Revert "Update OutlineTopComponent.java"
>    
>    This reverts commit 65e0651730983e12c032bb89564c3ef93aa34dbe.
>    
>    revert whitespace change

I have tested the changeset again and did not find any issues, keyboard shortcuts and contextual enabling/disabling of toolbar buttons works as expected. I only have a few style comments, please consider addressing them before integration.

src/utils/IdealGraphVisualizer/Coordinator/src/main/java/com/sun/hotspot/igv/coordinator/actions/ImportAction.java line 52:

> 50: import org.openide.util.RequestProcessor;
> 51: import org.openide.util.Utilities;
> 52: import org.openide.util.actions.CallableSystemAction;

As discussed earlier, it would really be preferable to leave cleanups of this kind to a separate RFE, especially if the file is not changed otherwise. It would make reviewing easier, especially on large changesets like this one. Why not postpone all import reordering to https://github.com/openjdk/jdk/pull/10197? Similarly for e.g. the changes in `DiagramScene.java`.

src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/EditorTopComponent.java line 431:

> 429:         setLayout(new java.awt.BorderLayout());
> 430: 
> 431:     }// </editor-fold>//GEN-END:initComponents

Did you check whether, after moving this generated code, it can still be updated using NetBeans' Form Editor?

src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/actions/ExportAction.java line 42:

> 40:  *
> 41:  * @author Thomas Wuerthinger
> 42:  */

Please keep the original `@autor` tag, here and in all other cases where it is removed.

-------------

Marked as reviewed by rcastanedalo (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10170