RFR: JDK-8288750: IGV: Improve Shortcuts [v5]
Christian Hagedorn
chagedorn at openjdk.org
Fri Jun 24 12:21:01 UTC 2022
On Fri, 24 Jun 2022 10:44:15 GMT, Tobias Holenstein <tholenstein at openjdk.org> wrote:
>> *Improvement of keyboard shortcuts in IGV under macOS:*.
>> Certain keyboard/mouse shortcuts do not work under macOS. E.g. `Ctrl + left-click` to select multiple nodes. The reason is that this keyboard shortcut is hardwired as a right-click under macOS and cannot be easily changed in the operating system. In general, the macOS user manual recommends using "Command/Meta" as a modifier key instead of "Control."
>>
>> *Fixed focus of the Graph Tab:*.
>> In IGV, shortcuts are linked to a component. Components are for example a Graph Tab, "Outline", "Filters", "Bytecode", "Control Flow" and "Properties". Shortcuts only work if the linked component is in focus. The focus can be changed with `Ctrl + TAB` or by clicking into the TAB component. The Graph Tab did not get the focus back when the user clicked on it. This needed to be fixed.
>>
>> *Fixing QuickSearch:*
>> Netbeans' QuickSearchAction is a global component of which only one common instance exists. IGV used a workaround to repaint the search bar in a new graphics tab. On macOS, the search bar doubled in size with each new Graph Tab. In addition, keyboard shortcuts for the search bar did not work. This issue was fixed by adding the search bar whenever the tab gained focus, and removing it (by default) when a new tab gained focus. This way, no workaround is required, and the size and ability to use a keyboard shortcut are fixed.
>>
>> *Adding new actions to expand/shrink the difference selection:*.
>> The user can expand/reduce the difference selection by moving the beginning/end of the selection with the mouse.
>> 
>> This is something many users didn't know. Therefore two new buttons should make it more clear for the user that this functionality exists.
>> 
>> By adding these button we can now also add keyboard shortcuts to expand/reduce the difference selection.
>>
>> **Fixed shortcuts for:**
>> - Add a single node in the graph to selection (`Ctrl/Cmd + left-click`)
>> - Add a multiple node in the graph to selection (`Ctrl/Cmd + left-click-drag`)
>> - Zoom in and out (`Ctrl/Cmd + mouse-wheel`)
>>
>> **Added new shortcuts for:**
>> - Search (`Ctrl/Cmd - I` and `Ctrl/Cmd - F`)
>> - Undo (`Ctrl/Cmd - Z`)
>> - Redo (`Ctrl/Cmd - Y` and `Ctrl/Cmd - Shift - Z`)
>> - Show Next Graph (`Ctrl/Cmd - RIGHT`)
>> - Expand the difference selection (`Ctrl/Cmd - UP` and `Ctrl/Cmd - Shift - RIGHT`)
>> - Reduce the difference selection (`Ctrl/Cmd - DOWN` and `Ctrl/Cmd - Shift - LEFT`)
>> - Show Previous Graph (`Ctrl/Cmd - LEFT`)
>> - Show satellite view (`Hold S`)
>
> Tobias Holenstein has updated the pull request incrementally with one additional commit since the last revision:
>
> added missing files
That's a nice improvement, thanks for fixing these and adding new shortcuts together with icons! I've tried the shortcuts out and they seem to work fine (tested on Ubuntu 20.04). It makes the workflow a lot easier.
I only have some minor code style comments, otherwise it looks good - but I'm not very familiar with the IGV code.
src/utils/IdealGraphVisualizer/Coordinator/src/main/java/com/sun/hotspot/igv/coordinator/actions/ImportAction.java line 171:
> 169:
> 170: @Override
> 171: protected boolean asynchronous() { return false; }
Suggestion:
protected boolean asynchronous() {
return false;
}
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/EditorTopComponent.java line 347:
> 345: centerPanel.getActionMap().put("showSatellite",
> 346: new AbstractAction("showSatellite") {
> 347: @Override public void actionPerformed(ActionEvent e) {
Suggestion:
@Override
public void actionPerformed(ActionEvent e) {
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/EditorTopComponent.java line 356:
> 354: centerPanel.getActionMap().put("showScene",
> 355: new AbstractAction("showScene") {
> 356: @Override public void actionPerformed(ActionEvent e) {
Suggestion:
@Override
public void actionPerformed(ActionEvent e) {
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/EditorTopComponent.java line 372:
> 370: centerPanel.add(SATELLITE_STRING, satelliteComponent);
> 371:
> 372:
New line can be removed.
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/actions/CustomSelectAction.java line 50:
> 48: }
> 49:
> 50: protected int getModifierMask () {
Suggestion:
protected int getModifierMask() {
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/actions/CustomSelectAction.java line 55:
> 53:
> 54: @Override
> 55: public State mousePressed (Widget widget, WidgetMouseEvent event) {
Suggestion:
public State mousePressed(Widget widget, WidgetMouseEvent event) {
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/actions/ExpandDiffAction.java line 41:
> 39: }
> 40:
> 41: public ExpandDiffAction(Lookup lookup) {
`lookup` seems unused. Can be removed?
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/actions/OverviewAction.java line 43:
> 41: public OverviewAction() {
> 42: putValue(AbstractAction.SMALL_ICON, new ImageIcon(ImageUtilities.loadImage(iconResource())));
> 43: putValue(Action.SHORT_DESCRIPTION, "Show satellite view of whole graph (hold S-KEY");
Suggestion:
putValue(Action.SHORT_DESCRIPTION, "Show satellite view of whole graph (hold S-KEY)");
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/actions/ShrinkDiffAction.java line 40:
> 38: }
> 39:
> 40: public ShrinkDiffAction(Lookup lookup) {
`lookup` seems unused. Can be removed?
src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/actions/ShrinkDiffAction.java line 67:
> 65: int nfp = fp;
> 66: int nsp = (fp < sp) ? sp - 1 : sp;
> 67: model.setPositions(nfp, nsp);
`nfp` can be inlined directly. Maybe you want to rename the variables instead of using abbreviations. Same in `ExpandDiffAction`.
-------------
Marked as reviewed by chagedorn (Reviewer).
PR: https://git.openjdk.org/jdk/pull/9260
More information about the hotspot-compiler-dev
mailing list