RFR: JDK-8293480: IGV: Update Bytecode and ControlFlow Component immediately when opening a new graph [v6]

[Christian Hagedorn]

On Fri, 16 Sep 2022 14:58:04 GMT, Tobias Holenstein <tholenstein at openjdk.org> wrote:

>> The `BytecodeViewTopComponent` and `ControlFlowTopComponent` represent information depending on which graph is opened in `EditorTopComponent`. Previously `BytecodeViewTopComponent` and `ControlFlowTopComponent` did not update their contents immediately when a new graph from another group was opened in `EditorTopComponent`. They were also not updated when switching between two tabs of an open graph. 
>> `OutlineTopComponent` had the same problem to update the selected graphs according to the `EditorTopComponent`
>> 
>> <img width="1291" alt="Update" src="https://user-images.githubusercontent.com/71546117/188860123-825a67fd-aab3-4500-8a47-8fa56eb9aeed.png">
>> 
>> **Analysis**
>> `BytecodeViewTopComponent`, `ControlFlowTopComponent` and `OutlineTopComponent` each represent information that depends on the `InputGraph` of the currently or last active `EditorTopComponent`. This information is made available globally by adding a new `InputGraphProvider` to the `Lookup` of `EditorTopComponent` each time the `InputGraph` changes. `BytecodeViewTopComponent`, `ControlFlowTopComponent` and `OutlineTopComponent` implement a `LookupListener` that calls `resultChanged(LookupEvent lookupEvent)` whenever a `InputGraphProvider` changes in in the lookup of `Utilities.actionsGlobalContext()`. When such a change happens the last active `InputGraphProvider` is retrieved from the `LookupHistory` by the listening components. 
>> 
>> **Problem** 
>> First, we missed to `fire()` a `diagramChangedEvent` in the constructor of `EditorTopComponent` which trigger to add the `InputGraphProvider` to the `Lookup`
>> 
>> Second, `Utilities.actionsGlobalContext()` returns a `Lookup` of the active (focused) `TopComponent's` `Lookup`. Unfortunately, when the last `EditorTopComponent` the `LookupListener` does not get called and there is no way to call is manually. 
>> 
>> **New Approach**
>> We extends the `LookupHistory` class such that we can add a `ChangedListener` that gets called whenever the last active `InputGraphProvider` is cached. So instead of listening to changes in `Utilities.actionsGlobalContext()` and then consulting the `LookupHistory`, now  `BytecodeViewTopComponent`, `ControlFlowTopComponent` and `OutlineTopComponent` listen to changes in the `LookupHistory` directly. This way we can now call `terminate` in the `LookupHistory` whenever we close a `EditorTopComponent`, which directly notifies the listeners.
>
> Tobias Holenstein has updated the pull request incrementally with four additional commits since the last revision:
> 
>  - Fix for: parent is null for DiffGraph
>  - remove DiagramProvider
>  - add ChangedListener to LookupHistory
>  - update OutlineTopComponent graph closed

Otherwise, looks good!

src/utils/IdealGraphVisualizer/Coordinator/src/main/java/com/sun/hotspot/igv/coordinator/OutlineTopComponent.java line 230:

> 228:         // before selecting it.
> 229:         SwingUtilities.invokeLater(() -> {
> 230:             GraphNode[] selection = new  GraphNode[]{};

Space; for an empty array you could also use:
Suggestion:

            GraphNode[] selection = new GraphNode[0];

src/utils/IdealGraphVisualizer/Util/src/main/java/com/sun/hotspot/igv/util/LookupHistory.java line 54:

> 52:             result.addLookupListener(this);
> 53:             last = Utilities.actionsGlobalContext().lookup(klass);
> 54:             this.fire();

You don't need to explicitly use `this`.
Suggestion:

            fire();

src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/DiagramViewModel.java line 102:

> 100:                 group.getChangedEvent().removeListener(groupContentChangedListener);
> 101:             }
> 102:             this.group = newModel.group;

Suggestion:

            group = newModel.group;

src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/EditorInputGraphProvider.java line 44:

> 42:     public EditorInputGraphProvider() {
> 43:         editor = null;
> 44:     }

Is this constructor used? Otherwise, it can be removed.

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

> 188:         content.add(rangeSliderModel);
> 189:         graphContent = new InstanceContent();
> 190:         this.associateLookup(new ProxyLookup(scene.getLookup(), new AbstractLookup(graphContent), new AbstractLookup(content)));

Suggestion:

        associateLookup(new ProxyLookup(scene.getLookup(), new AbstractLookup(graphContent), new AbstractLookup(content)));

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

> 193:             setDisplayName(getDiagram().getName());
> 194:             setToolTipText(getDiagram().getGraph().getGroup().getName());
> 195:             graphContent.set(Collections.singletonList(new EditorInputGraphProvider(EditorTopComponent.this)), null);

Suggestion:

            graphContent.set(Collections.singletonList(new EditorInputGraphProvider(this)), null);

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

Marked as reviewed by chagedorn (Reviewer).

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