RFR: JDK-8290010: IGV: Fix UndoRedo Action
Tobias Hartmann
thartmann at openjdk.org
Mon Oct 24 10:40:47 UTC 2022
On Fri, 21 Oct 2022 11:53:31 GMT, Tobias Holenstein <tholenstein at openjdk.org> wrote:
> # Problem
> - The history of the Undo Action did not start at zero when opening a new graph.
> - In the "Show sea of nodes" view you could press Undo once before doing any changes.
> - In the "Show control view graph" you could even go back many times when opening a new graph, ending up in wired stated of the graph.
> - Selecting a node should not be added to the history. You could select X different nodes in a row and then go back X times in history without that anything changed.
>
> # Overview UndoRedo
> Different `ChangedEvent<DiagramViewModel>` events are fired in `DiagramViewModel` that trigger the `DiagramScene` to update its state. After those updates `DiagramScene` calls `addUndo()` to save the new state of the `DiagramViewModel` as well as the scrolling position of the `DiagramScene` in a `DiagramUndoRedo` object. `undo()` / `redo()` can then be used to restore the saved state.
>
> One problem was that selecting nodes triggered a `ChangedEvent<DiagramViewModel>` event that caused a recording to the history but the selection itself was not part of the saved state. Another problem was that switching between different views (CFG view, sea-of-nodes view) as well as applying filters also triggered an `addUndo()` - The recording of filters and view switches is however not implemented.
>
> # Solution
> We now only record when the graph itself changes (not the view) - like when opening a new graph or creating a difference graph. And we also record changes in the selection of the visible nodes.
>
> We refactored the `ChangedEvent<DiagramViewModel>` events in `DiagramViewModel`:
> - `graphChangedEvent` now is fired when the graph changes and triggers an `addUndo()`
> - `hiddenNodesChangedEvent` is now fired when the selection of visible nodes changes and also triggers an `addUndo()`
>
> `DiagramUndoRedo` previously stored a deep copy of the `DiagramViewModel` for every datapoint in the history. Besides using a lot of memory, most of the stored objects were redundant or not used. Now, we introduced a new `ModelState` object that stores the id of the visible nodes in `Set<Integer> hiddenNodes` as well as the opened graph. For the graph we only need `int firstPos` and `int secondPos` which indicate the position of the difference graph within the group. If we don't use a difference graph `firstPos` == `secondPos`.
I gave this a quick test and clicking on a different phase and then reverting back and forth to the previous phase via the undo/redo buttons does not work anymore after doing it twice.
-------------
Changes requested by thartmann (Reviewer).
PR: https://git.openjdk.org/jdk/pull/10813
More information about the hotspot-compiler-dev
mailing list