Group next/prev or up/down keys in help dialog

Instead of putting two navigation keys on their own lines in the help
dialog, cluster them by sibling relationship (up/down, next/prev) and
show the pair on one line:

  n / p : Next page / previous page
  ] / [ : Next file / previous file

To improve clustering of identical actions and make it easier to
locate a task each group is now sorted by help text, rather than
by key stroke.

Change-Id: I21d0d93558d276a78127b110c6afc77852ab8ba9

gerrit-gwtexpui/src/main/java/com/google/gwtexpui/globalkey/client/KeyCommand.java

@@ -27,11 +27,14 @@ public abstract class KeyCommand implements KeyPressHandler {
   public static final int M_SHIFT = 8 << 16;
 
   public static boolean same(final KeyCommand a, final KeyCommand b) {
-    return a.getClass() == b.getClass() && a.helpText.equals(b.helpText);
+    return a.getClass() == b.getClass()
+        && a.helpText.equals(b.helpText)
+        && a.sibling == b.sibling;
   }
 
   final int keyMask;
   private final String helpText;
+  KeyCommand sibling;
 
   public KeyCommand(final int mask, final int key, final String help) {
     this(mask, (char) key, help);

gerrit-gwtexpui/src/main/java/com/google/gwtexpui/globalkey/client/KeyCommandSet.java

@@ -52,6 +52,17 @@ public class KeyCommandSet implements KeyPressHandler {
     return map.isEmpty();
   }
 
+  public void add(KeyCommand a, KeyCommand b) {
+    add(a);
+    add(b);
+    pair(a, b);
+  }
+
+  public void pair(KeyCommand a, KeyCommand b) {
+    a.sibling = b;
+    b.sibling = a;
+  }
+
   public void add(final KeyCommand k) {
     assert !map.containsKey(k.keyMask)
          : "Key " + k.describeKeyStroke().asString()

gerrit-gwtexpui/src/main/java/com/google/gwtexpui/globalkey/client/KeyHelpPopup.java

@@ -17,17 +17,17 @@ package com.google.gwtexpui.globalkey.client;
 import com.google.gwt.event.dom.client.ClickEvent;
 import com.google.gwt.event.dom.client.ClickHandler;
 import com.google.gwt.event.dom.client.KeyCodes;
-import com.google.gwt.event.dom.client.KeyPressEvent;
-import com.google.gwt.event.dom.client.KeyPressHandler;
 import com.google.gwt.event.dom.client.KeyDownEvent;
 import com.google.gwt.event.dom.client.KeyDownHandler;
+import com.google.gwt.event.dom.client.KeyPressEvent;
+import com.google.gwt.event.dom.client.KeyPressHandler;
 import com.google.gwt.user.client.DOM;
 import com.google.gwt.user.client.ui.Anchor;
 import com.google.gwt.user.client.ui.FlowPanel;
 import com.google.gwt.user.client.ui.FocusPanel;
 import com.google.gwt.user.client.ui.Grid;
-import com.google.gwt.user.client.ui.HasHorizontalAlignment;
 import com.google.gwt.user.client.ui.HTMLTable.CellFormatter;
+import com.google.gwt.user.client.ui.HasHorizontalAlignment;
 import com.google.gwtexpui.safehtml.client.SafeHtml;
 import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder;
 import com.google.gwtexpui.user.client.PluginSafePopupPanel;
@@ -36,8 +36,10 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
+import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.Map;
 
 
 public class KeyHelpPopup extends PluginSafePopupPanel implements
@@ -164,13 +166,17 @@ public class KeyHelpPopup extends PluginSafePopupPanel implements
   private int formatKeys(final Grid lists, int row, final int col,
       final KeyCommandSet set, final SafeHtml prefix) {
     final CellFormatter fmt = lists.getCellFormatter();
-    final int initialRow = row;
     final List<KeyCommand> keys = sort(set);
     if (lists.getRowCount() < row + keys.size()) {
       lists.resizeRows(row + keys.size());
     }
+
+    Map<KeyCommand, Integer> rows = new HashMap<KeyCommand, Integer>();
     FORMAT_KEYS: for (int i = 0; i < keys.size(); i++) {
       final KeyCommand k = keys.get(i);
+      if (rows.containsKey(k)) {
+        continue;
+      }
 
       if (k instanceof CompoundKeyCommand) {
         final SafeHtmlBuilder b = new SafeHtmlBuilder();
@@ -181,7 +187,7 @@ public class KeyHelpPopup extends PluginSafePopupPanel implements
 
       for (int prior = 0; prior < i; prior++) {
         if (KeyCommand.same(keys.get(prior), k)) {
-          final int r = initialRow + prior;
+          final int r = rows.get(keys.get(prior));
           final SafeHtmlBuilder b = new SafeHtmlBuilder();
           b.append(SafeHtml.get(lists, r, col + 0));
           b.append(" ");
@@ -195,23 +201,29 @@ public class KeyHelpPopup extends PluginSafePopupPanel implements
           }
           b.append(k.describeKeyStroke());
           SafeHtml.set(lists, r, col + 0, b);
+          rows.put(k, r);
           continue FORMAT_KEYS;
         }
       }
 
+      SafeHtmlBuilder b = new SafeHtmlBuilder();
+      String t = k.getHelpText();
       if (prefix != null) {
-        final SafeHtmlBuilder b = new SafeHtmlBuilder();
         b.append(prefix);
         b.append(" ");
         b.append(KeyConstants.I.thenOtherKey());
         b.append(" ");
-        b.append(k.describeKeyStroke());
-        SafeHtml.set(lists, row, col + 0, b);
-      } else {
-        SafeHtml.set(lists, row, col + 0, k.describeKeyStroke());
       }
+      b.append(k.describeKeyStroke());
+      if (k.sibling != null) {
+        b.append(" / ").append(k.sibling.describeKeyStroke());
+        t += " / " + k.sibling.getHelpText();
+        rows.put(k.sibling, row);
+      }
+      SafeHtml.set(lists, row, col + 0, b);
       lists.setText(row, col + 1, ":");
-      lists.setText(row, col + 2, k.getHelpText());
+      lists.setText(row, col + 2, t);
+      rows.put(k, row);
 
       fmt.addStyleName(row, col + 0, KeyResources.I.css().helpKeyStroke());
       fmt.addStyleName(row, col + 1, KeyResources.I.css().helpSeparator());
@@ -226,12 +238,7 @@ public class KeyHelpPopup extends PluginSafePopupPanel implements
     Collections.sort(keys, new Comparator<KeyCommand>() {
       @Override
       public int compare(KeyCommand arg0, KeyCommand arg1) {
-        if (arg0.keyMask < arg1.keyMask) {
+        return arg0.getHelpText().compareTo(arg1.getHelpText());
-          return -1;
-        } else if (arg0.keyMask > arg1.keyMask) {
-          return 1;
-        }
-        return 0;
       }
     });
     return keys;

gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java

@@ -224,8 +224,9 @@ class FileTable extends FlowPanel {
       this.list = list;
       table.setWidth("");
 
-      keysNavigation.add(new PrevKeyCommand(0, 'k', Util.C.patchTablePrev()));
+      keysNavigation.add(
-      keysNavigation.add(new NextKeyCommand(0, 'j', Util.C.patchTableNext()));
+          new PrevKeyCommand(0, 'k', Util.C.patchTablePrev()),
+          new NextKeyCommand(0, 'j', Util.C.patchTableNext()));
       keysNavigation.add(new OpenKeyCommand(0, 'o', Util.C.patchTableOpenDiff()));
       keysNavigation.add(new OpenKeyCommand(0, KeyCodes.KEY_ENTER,
           Util.C.patchTableOpenDiff()));

gerrit-gwtui/src/main/java/com/google/gerrit/client/change/RelatedChangesTab.java

@@ -170,9 +170,9 @@ class RelatedChangesTab {
       table.setWidth("");
 
       keysNavigation.setName(Gerrit.C.sectionNavigation());
-      keysNavigation.add(new PrevKeyCommand(0, 'K',
+      keysNavigation.add(
-          Resources.C.previousChange()));
+          new PrevKeyCommand(0, 'K', Resources.C.previousChange()),
-      keysNavigation.add(new NextKeyCommand(0, 'J', Resources.C.nextChange()));
+          new NextKeyCommand(0, 'J', Resources.C.nextChange()));
       keysNavigation.add(new OpenKeyCommand(0, 'O', Resources.C.openChange()));
     }
 

gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PagedSingleListScreen.java

@@ -67,15 +67,13 @@ public abstract class PagedSingleListScreen extends Screen {
 
     table = new ChangeTable2() {
       {
-        keysNavigation.add(new DoLinkCommand(0, 'p', Util.C
+        keysNavigation.add(
-            .changeTablePagePrev(), prev));
+            new DoLinkCommand(0, 'p', Util.C.changeTablePagePrev(), prev),
-        keysNavigation.add(new DoLinkCommand(0, 'n', Util.C
+            new DoLinkCommand(0, 'n', Util.C.changeTablePageNext(), next));
-            .changeTablePageNext(), next));
 
-        keysNavigation.add(new DoLinkCommand(0, '[', Util.C
+        keysNavigation.add(
-            .changeTablePagePrev(), prev));
+            new DoLinkCommand(0, '[', Util.C.changeTablePagePrev(), prev),
-        keysNavigation.add(new DoLinkCommand(0, ']', Util.C
+            new DoLinkCommand(0, ']', Util.C.changeTablePageNext(), next));
-            .changeTablePageNext(), next));
 
         keysNavigation.add(new KeyCommand(0, 'R', Util.C.keyReloadSearch()) {
           @Override

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Header.java

@@ -118,9 +118,13 @@ class Header extends Composite {
         FileInfo nextInfo = index == files.length() - 1
             ? null
             : files.get(index + 1);
-        setupNav(prev, '[', PatchUtil.C.previousFileHelp(),
+        KeyCommand p = setupNav(prev, '[', PatchUtil.C.previousFileHelp(),
             index == 0 ? null : files.get(index - 1));
-        setupNav(next, ']', PatchUtil.C.nextFileHelp(), nextInfo);
+        KeyCommand n = setupNav(next, ']', PatchUtil.C.nextFileHelp(),
+            nextInfo);
+        if (p != null && n != null) {
+          keys.pair(p, n);
+        }
         nextPath = nextInfo != null ? nextInfo.path() : null;
       }
     });
@@ -183,25 +187,28 @@ class Header extends Composite {
     return p.toString();
   }
 
-  private void setupNav(InlineHyperlink link, int key, String help, FileInfo info) {
+  private KeyCommand setupNav(InlineHyperlink link, int key, String help, FileInfo info) {
     if (info != null) {
       final String url = url(info);
       link.setTargetHistoryToken(url);
       link.setTitle(FileInfo.getFileName(info.path()));
-      keys.add(new KeyCommand(0, key, help) {
+      KeyCommand k = new KeyCommand(0, key, help) {
         @Override
         public void onKeyPress(KeyPressEvent event) {
           Gerrit.display(url);
         }
-      });
+      };
+      keys.add(k);
       if (link == prev) {
         hasPrev = true;
       } else {
         hasNext = true;
       }
+      return k;
     } else {
       link.getElement().getStyle().setVisibility(Visibility.HIDDEN);
       keys.add(new UpToChangeCommand2(patchSetId, 0, key));
+      return null;
     }
   }
 

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide2.java

@@ -343,10 +343,12 @@ public class SideBySide2 extends Screen {
     super.registerKeys();
 
     keysNavigation.add(new UpToChangeCommand2(revision, 0, 'u'));
-    keysNavigation.add(new NoOpKeyCommand(0, 'j', PatchUtil.C.lineNext()));
+    keysNavigation.add(
-    keysNavigation.add(new NoOpKeyCommand(0, 'k', PatchUtil.C.linePrev()));
+        new NoOpKeyCommand(0, 'j', PatchUtil.C.lineNext()),
-    keysNavigation.add(new NoOpKeyCommand(0, 'n', PatchUtil.C.chunkNext2()));
+        new NoOpKeyCommand(0, 'k', PatchUtil.C.linePrev()));
-    keysNavigation.add(new NoOpKeyCommand(0, 'p', PatchUtil.C.chunkPrev2()));
+    keysNavigation.add(
+        new NoOpKeyCommand(0, 'n', PatchUtil.C.chunkNext2()),
+        new NoOpKeyCommand(0, 'p', PatchUtil.C.chunkPrev2()));
 
     keysAction = new KeyCommandSet(Gerrit.C.sectionActions());
     keysAction.add(new NoOpKeyCommand(0, 'o', PatchUtil.C.expandComment()));

gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/NavigationTable.java

@@ -94,8 +94,9 @@ public abstract class NavigationTable<RowItem> extends FancyFlexTable<RowItem> {
 
   protected NavigationTable(String itemHelpName) {
     this();
-    keysNavigation.add(new PrevKeyCommand(0, 'k', Util.M.helpListPrev(itemHelpName)));
+    keysNavigation.add(
-    keysNavigation.add(new NextKeyCommand(0, 'j', Util.M.helpListNext(itemHelpName)));
+        new PrevKeyCommand(0, 'k', Util.M.helpListPrev(itemHelpName)),
+        new NextKeyCommand(0, 'j', Util.M.helpListNext(itemHelpName)));
     keysNavigation.add(new OpenKeyCommand(0, 'o', Util.M.helpListOpen(itemHelpName)));
     keysNavigation.add(new OpenKeyCommand(0, KeyCodes.KEY_ENTER,
                                                   Util.M.helpListOpen(itemHelpName)));