Fixes for keyboard (and mouse) navigation in one page review

Navigation in one page review had some issues. Following fixes done:
* mouse click focuses the row clicked to enable keyboard navigation
* next line navigation key (j) moves to next file if last line of
  the file reached
* previous line navigation key (k) moves to previous file if first line
  of the file reached
* also next/previous keys for chunk/comment (n/p/N/P) move to next/previous
  file when necessary
* publish comment key (r) focuses the cover message edit box, instead of
  jumping to Publish Comments page

Task-number: QTQAINFRA-856
Change-Id: Ica9676a641d1a5d7d18c545369ff0e14471129e4
Reviewed-by: Oswald Buddenhagen <oswald.buddenhagen@theqtcompany.com>
Reviewed-by: Ismo Haataja <ismo.haataja@digia.com>

2 files changed, 292 insertions, 85 deletions

diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/AbstractPatchContentTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/AbstractPatchContentTable.java
index 1a3dbe1e2c..c635b15648 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/AbstractPatchContentTable.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/AbstractPatchContentTable.java
@@ -49,6 +49,7 @@ import com.google.gwt.event.dom.client.FocusEvent;
 import com.google.gwt.event.dom.client.FocusHandler;
 import com.google.gwt.user.client.DOM;
 import com.google.gwt.user.client.Element;
+import com.google.gwt.user.client.Event;
 import com.google.gwt.user.client.History;
 import com.google.gwt.user.client.ui.Button;
 import com.google.gwt.user.client.ui.Focusable;
@@ -73,6 +74,15 @@ public abstract class AbstractPatchContentTable extends NavigationTable<Object>
     void onClick();
   }
 
+  public static enum Move {
+    LINE_FIRST,
+    LINE_LAST,
+    COMMENT_FIRST,
+    COMMENT_LAST,
+    CHUNK_FIRST,
+    CHUNK_LAST
+  }
+
   public static final int R_HEAD = 0;
   static final short FILE_SIDE_A = (short) 0;
   static final short FILE_SIDE_B = (short) 1;
@@ -92,6 +102,8 @@ public abstract class AbstractPatchContentTable extends NavigationTable<Object>
   private CommentLinkProcessor commentLinkProcessor;
   boolean isDisplayBinary;
   protected boolean isFileCommentBorderRowExist;
+  private boolean hasEdits;
+  private boolean hasComments;
 
   private class KeyNavigation extends ContentTableKeyNavigation {
     public KeyNavigation(Widget parent) {
@@ -180,6 +192,8 @@ public abstract class AbstractPatchContentTable extends NavigationTable<Object>
     table.setStyleName(Gerrit.RESOURCES.css().patchContentTable());
     delegates = new HashSet<Delegate>();
     isAllMode = false;
+    hasComments = false;
+    hasEdits = false;
   }
 
   abstract void createFileCommentEditorOnSideA();
@@ -294,17 +308,117 @@ public abstract class AbstractPatchContentTable extends NavigationTable<Object>
     idSideA = a;
     idSideB = b;
 
+    hasEdits = hasEdits(s);
+    hasComments = hasComments(s);
+
     render(s, d);
   }
 
   public boolean isOnFirstRow() {
-    int firstRow = findChunkStart(getCurrentRow());
-    return getCurrentRow() == firstRow || getCurrentRow() == 1 || getRowItem(getCurrentRow() - 1) == null;
+    for (int row = getCurrentRow() - 1; row > 0; row--) {
+      final Object o = getRowItem(row);
+      if (o instanceof PatchLine || o instanceof CommentList) {
+        return false;
+      }
+    }
+    return true;
   }
 
   public boolean isOnLastRow() {
-    int lastRow = findChunkEnd(getCurrentRow());
-    return getCurrentRow() == lastRow || getRowItem(getCurrentRow() + 1) == null;
+    for (int row = getCurrentRow() + 1; row < getMaxRows(); row++) {
+      final Object o = getRowItem(row);
+      if (o instanceof PatchLine || o instanceof CommentList) {
+        return false;
+      }
+    }
+    return true;
+  }
+
+  public void moveTo(Move move) {
+    switch (move) {
+      case LINE_FIRST:
+        moveToFirstLine();
+        break;
+      case LINE_LAST:
+        moveToLastLine();
+        break;
+      case CHUNK_FIRST:
+        moveToFirstChunk();
+        break;
+      case CHUNK_LAST:
+        moveToLastChunk();
+        break;
+      case COMMENT_FIRST:
+        moveToFirstComment();
+        break;
+      case COMMENT_LAST:
+        moveToLastComment();
+        break;
+    }
+  }
+
+  private void moveToFirstLine() {
+    movePointerTo(0, false);
+    for (int row = getCurrentRow(); row < getMaxRows(); row++) {
+      final Object o = getRowItem(row);
+      if (o instanceof PatchLine || o instanceof CommentList) {
+        movePointerTo(row);
+        return;
+      }
+    }
+  }
+
+  private void moveToFirstComment() {
+    moveToFirstLine();
+    for (int row = getCurrentRow(); row < getMaxRows(); row++) {
+      final Object o = getRowItem(row);
+      if (o instanceof CommentList) {
+        movePointerTo(row);
+        return;
+      }
+    }
+  }
+
+  private void moveToFirstChunk() {
+    moveToFirstLine();
+    for (int row = getCurrentRow(); row < getMaxRows(); row++) {
+      if (isChunk(row)) {
+        movePointerTo(row);
+        return;
+      }
+    }
+  }
+
+  private void moveToLastLine() {
+    movePointerTo(getMaxRows()-1, false);
+    for (int row = getCurrentRow(); row > 0; row--) {
+      final Object o = getRowItem(row);
+      if (o instanceof PatchLine || o instanceof CommentList) {
+        movePointerTo(row);
+        return;
+      }
+    }
+  }
+
+  private void moveToLastComment() {
+    moveToLastLine();
+    for (int row = getCurrentRow(); row > 0; row--) {
+      final Object o = getRowItem(row);
+      if (o instanceof CommentList) {
+        movePointerTo(row);
+        return;
+      }
+    }
+  }
+
+  private void moveToLastChunk() {
+    moveToLastLine();
+    for (int row = getCurrentRow(); row > 0; row--) {
+      if (isChunk(row)) {
+        movePointerTo(row);
+        return;
+      }
+    }
   }
 
   void setCommentLinkProcessor(CommentLinkProcessor commentLinkProcessor) {
@@ -335,6 +449,14 @@ public abstract class AbstractPatchContentTable extends NavigationTable<Object>
         || !script.getCommentDetail().getCommentsB().isEmpty();
   }
 
+  public boolean hasEdits() {
+    return hasEdits;
+  }
+
+  public boolean hasComments() {
+    return hasComments;
+  }
+
   // True if this change is a mode change or a pure rename/copy
   private boolean hasMeta(PatchScript script) {
     return !script.getPatchHeader().isEmpty();
@@ -412,6 +534,11 @@ public abstract class AbstractPatchContentTable extends NavigationTable<Object>
   public abstract void display(CommentDetail comments, boolean expandComments);
 
   @Override
+  protected MyFlexTable createFlexTable() {
+    return new DoubleClickFlexTable();
+  }
+
+  @Override
   protected Object getRowItemKey(final Object item) {
     return null;
   }
@@ -467,7 +594,7 @@ public abstract class AbstractPatchContentTable extends NavigationTable<Object>
     return end + 1 < table.getRowCount() ? end + 1 : end;
   }
 
-  public void moveToPrevChunk(int row) {
+  public boolean moveToPrevChunk(int row) {
     while (0 <= row && isChunk(row)) {
       row--;
     }
@@ -476,21 +603,13 @@ public abstract class AbstractPatchContentTable extends NavigationTable<Object>
         final int start = findChunkStart(row);
         movePointerTo(start, false);
         scrollIntoView(oneBefore(start), oneAfter(row));
-        return;
-      }
-    }
-
-    // No prior hunk found? Try to hit the first line in the file.
-    //
-    for (row = 0; row < table.getRowCount(); row++) {
-      if (getRowItem(row) != null) {
-        movePointerTo(row);
-        break;
+        return true;
       }
     }
+    return false;
   }
 
-  public void moveToNextChunk(int row) {
+  public boolean moveToNextChunk(int row) {
     final int max = table.getRowCount();
     while (row < max && isChunk(row)) {
       row++;
@@ -499,21 +618,13 @@ public abstract class AbstractPatchContentTable extends NavigationTable<Object>
       if (isChunk(row)) {
         movePointerTo(row, false);
         scrollIntoView(oneBefore(row), oneAfter(findChunkEnd(row)));
-        return;
-      }
-    }
-
-    // No next hunk found? Try to hit the last line in the file.
-    //
-    for (row = max - 1; row >= 0; row--) {
-      if (getRowItem(row) != null) {
-        movePointerTo(row);
-        break;
+        return true;
       }
     }
+    return false;
   }
 
-  public void moveToPrevComment(int row) {
+  public boolean moveToPrevComment(int row) {
     while (0 <= row && isComment(row)) {
       row--;
     }
@@ -521,21 +632,13 @@ public abstract class AbstractPatchContentTable extends NavigationTable<Object>
       if (isComment(row)) {
         movePointerTo(row, false);
         scrollIntoView(oneBefore(row), oneAfter(row));
-        return;
-      }
-    }
-
-    // No prior comment found? Try to hit the first line in the file.
-    //
-    for (row = 0; row < table.getRowCount(); row++) {
-      if (getRowItem(row) != null) {
-        movePointerTo(row);
-        break;
+        return true;
       }
     }
+    return false;
   }
 
-  public void moveToNextComment(int row) {
+  public boolean moveToNextComment(int row) {
     final int max = table.getRowCount();
     while (row < max && isComment(row)) {
       row++;
@@ -544,18 +647,10 @@ public abstract class AbstractPatchContentTable extends NavigationTable<Object>
       if (isComment(row)) {
         movePointerTo(row, false);
         scrollIntoView(oneBefore(row), oneAfter(row));
-        return;
-      }
-    }
-
-    // No next comment found? Try to hit the last line in the file.
-    //
-    for (row = max - 1; row >= 0; row--) {
-      if (getRowItem(row) != null) {
-        movePointerTo(row);
-        break;
+        return true;
       }
     }
+    return false;
   }
 
   public void moveToTop() {
@@ -576,6 +671,10 @@ public abstract class AbstractPatchContentTable extends NavigationTable<Object>
     }
   }
 
+  public void moveToActiveRow() {
+    movePointerTo(getCurrentRow());
+  }
+
   private boolean isComment(int row) {
     return getRowItem(row) instanceof CommentList;
   }
@@ -879,6 +978,44 @@ public abstract class AbstractPatchContentTable extends NavigationTable<Object>
         new ArrayList<PublishedCommentPanel>();
   }
 
+  protected class DoubleClickFlexTable extends MyFlexTable {
+    public DoubleClickFlexTable() {
+      sinkEvents(Event.ONDBLCLICK | Event.ONCLICK);
+    }
+
+    @Override
+    public void onBrowserEvent(final Event event) {
+      switch (DOM.eventGetType(event)) {
+        case Event.ONCLICK: {
+          // Find out which cell was actually clicked.
+          final Element td = getEventTargetCell(event);
+          if (td == null) {
+            break;
+          }
+          final int row = rowOf(td);
+          if (getRowItem(row) != null) {
+            movePointerTo(row);
+            for (Delegate delegate : delegates) {
+              delegate.onClick();
+            }
+            return;
+          }
+          break;
+        }
+        case Event.ONDBLCLICK: {
+          // Find out which cell was actually clicked.
+          Element td = getEventTargetCell(event);
+          if (td == null) {
+            return;
+          }
+          onCellDoubleClick(rowOf(td), columnOf(td));
+          return;
+        }
+      }
+      super.onBrowserEvent(event);
+    }
+  }
+
   private class PublishedCommentPanel extends CommentPanel implements
       ClickHandler {
     final PatchLineComment comment;
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/AllInOnePatchScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/AllInOnePatchScreen.java
index d15af28ab4..fcbab486c2 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/AllInOnePatchScreen.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/AllInOnePatchScreen.java
@@ -14,7 +14,6 @@
 
 package com.google.gerrit.client.patches;
 
-import com.google.gerrit.client.Dispatcher;
 import com.google.gerrit.client.ErrorDialog;
 import com.google.gerrit.client.Gerrit;
 import com.google.gerrit.client.changes.ChangeApi;
@@ -28,6 +27,7 @@ import com.google.gerrit.client.changes.SubmitFailureDialog;
 import com.google.gerrit.client.changes.SubmitInfo;
 import com.google.gerrit.client.changes.Util;
 import com.google.gerrit.client.patches.AbstractPatchContentTable.CommentList;
+import com.google.gerrit.client.patches.AbstractPatchContentTable.Move;
 import com.google.gerrit.client.projects.ConfigInfoCache;
 import com.google.gerrit.client.rpc.CallbackGroup;
 import com.google.gerrit.client.rpc.GerritCallback;
@@ -123,6 +123,9 @@ public class AllInOnePatchScreen extends AbstractPatchScreen implements
         keysNavigation.add(new NextFileCmd(0, 'w', PatchUtil.C.nextFileHelp()));
         keysNavigation.add(new PrevFileCmd(0, 'q', PatchUtil.C
             .previousFileHelp()));
+        keysNavigation.add(new NextFileCmd(0, ']', PatchUtil.C.nextFileHelp()));
+        keysNavigation.add(new PrevFileCmd(0, '[', PatchUtil.C
+            .previousFileHelp()));
         super.initializeKeys();
       }
     }
@@ -140,32 +143,40 @@ public class AllInOnePatchScreen extends AbstractPatchScreen implements
     @Override
     protected void onChunkNext() {
       if (contentTable != null) {
-        contentTable.ensurePointerVisible();
-        contentTable.moveToNextChunk(contentTable.getCurrentRow());
+        // Returns false if no more chunks found -> try next file
+        if ( !contentTable.moveToNextChunk(contentTable.getCurrentRow())) {
+          onFileNext(false, Move.CHUNK_FIRST);
+        }
       }
     }
 
     @Override
     protected void onChunkPrev() {
       if (contentTable != null) {
-        contentTable.ensurePointerVisible();
-        contentTable.moveToPrevChunk(contentTable.getCurrentRow());
+        // Returns false if no more chunks found -> try previous file
+        if ( !contentTable.moveToPrevChunk(contentTable.getCurrentRow())) {
+          onFilePrev(false, Move.CHUNK_LAST);
+        }
       }
     }
 
     @Override
     protected void onCommentNext() {
       if (contentTable != null) {
-        contentTable.ensurePointerVisible();
-        contentTable.moveToNextComment(contentTable.getCurrentRow());
+        // Returns false if no more comments found -> try next file
+        if (!contentTable.moveToNextComment(contentTable.getCurrentRow())) {
+          onFileNext(false, Move.COMMENT_FIRST);
+        }
       }
     }
 
     @Override
     protected void onCommentPrev() {
       if (contentTable != null) {
-        contentTable.ensurePointerVisible();
-        contentTable.moveToPrevComment(contentTable.getCurrentRow());
+        // Returns false if no more comments found -> try previous file
+        if (!contentTable.moveToPrevComment(contentTable.getCurrentRow())) {
+          onFilePrev(false, Move.COMMENT_LAST);
+        }
       }
     }
 
@@ -204,27 +215,45 @@ public class AllInOnePatchScreen extends AbstractPatchScreen implements
     }
 
     protected void onFileNext() {
+      onFileNext(true, Move.LINE_FIRST);
+    }
+
+    protected void onFileNext(boolean scrollTop, Move moveTo) {
       contentTable.hideCursor();
-      diff = getNextDiff();
-      contentTable = diff.getContentTable();
-      Window.scrollTo(0, diff.getAbsoluteTop());
-      contentTable.ensurePointerVisible();
+      final Diff diff = getNextDiff(moveTo);
+      if (diff != null) {
+        this.diff = diff;
+        contentTable = diff.getContentTable();
+        if (scrollTop) {
+          Window.scrollTo(0, diff.getAbsoluteTop());
+        }
+        contentTable.moveTo(moveTo);
+      }
       contentTable.showCursor();
     }
 
     protected void onFilePrev() {
+      onFilePrev(true, Move.LINE_FIRST);
+    }
+
+    protected void onFilePrev(boolean scrollTop, Move moveTo) {
       contentTable.hideCursor();
-      diff = getPrevDiff();
-      contentTable = diff.getContentTable();
-      Window.scrollTo(0, diff.getAbsoluteTop());
-      contentTable.ensurePointerVisible();
+      final Diff diff = getPrevDiff(moveTo);
+      if (diff != null) {
+        this.diff = diff;
+        contentTable = diff.getContentTable();
+        if (scrollTop) {
+          Window.scrollTo(0, diff.getAbsoluteTop());
+        }
+        contentTable.moveTo(moveTo);
+      }
       contentTable.showCursor();
     }
 
     @Override
     protected void onInsertComment() {
       if (contentTable != null) {
-        contentTable.ensurePointerVisible();
+        contentTable.moveToActiveRow();
         for (int row = contentTable.getCurrentRow(); 0 <= row; row--) {
           final Object item = contentTable.getRowItem(row);
           if (item instanceof PatchLine) {
@@ -242,15 +271,18 @@ public class AllInOnePatchScreen extends AbstractPatchScreen implements
     @Override
     protected void onNext() {
       if (contentTable != null) {
-        contentTable.ensurePointerVisible();
-        contentTable.onDown();
+        if (contentTable.isOnLastRow()) {
+          onFileNext(false, Move.LINE_FIRST);
+        } else {
+          contentTable.onDown();
+        }
       }
     }
 
     @Override
     protected void onOpen() {
       if (contentTable != null) {
-        contentTable.ensurePointerVisible();
+        contentTable.moveToActiveRow();
         contentTable.onOpenCurrent();
       }
     }
@@ -258,15 +290,20 @@ public class AllInOnePatchScreen extends AbstractPatchScreen implements
     @Override
     protected void onPrev() {
       if (contentTable != null) {
-        contentTable.ensurePointerVisible();
-        contentTable.onUp();
+        if (contentTable.isOnFirstRow()) {
+          onFilePrev(false, Move.LINE_LAST);
+        } else {
+          contentTable.onUp();
+        }
       }
     }
 
     @Override
     protected void onPublishComments() {
-      final PatchSet.Id id = patchKey.getParentKey();
-      Gerrit.display(Dispatcher.toPublish(id));
+      if (approvalButtons.iterator().hasNext()) {
+        Window.scrollTo(0, approvalButtons.iterator().next().getAbsoluteTop());
+      }
+      message.setFocus(true);
     }
   }
 
@@ -439,7 +476,7 @@ public class AllInOnePatchScreen extends AbstractPatchScreen implements
     this.id = id;
   }
 
-  private Diff getNextDiff() {
+  private Diff getNextDiff(Move moveTo) {
     if (keyNavigation.getDiff() == null) {
       if (!diffs.isEmpty()) {
         return diffs.get(0);
@@ -447,16 +484,33 @@ public class AllInOnePatchScreen extends AbstractPatchScreen implements
         return null;
       }
     } else {
-      int index = diffs.indexOf(keyNavigation.getDiff());
-      if (index + 1 < diffs.size()) {
-        return diffs.get(index + 1);
-      } else {
-        return diffs.get(0);
+      int index = diffs.indexOf(keyNavigation.getDiff()) + 1;
+      while (index < diffs.size()) {
+        final Diff diff = diffs.get(index);
+        if (diff.isVisible()) {
+          switch (moveTo) {
+            case CHUNK_FIRST:
+              if (diff.getContentTable().hasEdits()
+                  || diff.getContentTable().hasComments()) {
+                return diff;
+              }
+              break;
+            case COMMENT_FIRST:
+              if (diff.getContentTable().hasComments()) {
+                return diff;
+              }
+              break;
+            default:
+                return diff;
+          }
+        }
+        index++;
       }
+      return null;
     }
   }
 
-  private Diff getPrevDiff() {
+  private Diff getPrevDiff(Move moveTo) {
     if (keyNavigation.getDiff() == null) {
       if (!diffs.isEmpty()) {
         return diffs.get(0);
@@ -464,12 +518,28 @@ public class AllInOnePatchScreen extends AbstractPatchScreen implements
         return null;
       }
     } else {
-      int index = diffs.indexOf(keyNavigation.getDiff());
-      if (index - 1 >= 0) {
-        return diffs.get(index - 1);
-      } else {
-        return diffs.get(diffs.size() - 1);
+      int index = diffs.indexOf(keyNavigation.getDiff()) -1 ;
+      while (index >= 0) {
+        final Diff diff = diffs.get(index);
+        if (diff.isVisible()) {
+          switch (moveTo) {
+            case CHUNK_LAST:
+              if (diff.getContentTable().hasEdits()) {
+                return diff;
+              }
+              break;
+            case COMMENT_LAST:
+              if (diff.getContentTable().hasComments()) {
+                return diff;
+              }
+              break;
+            default:
+                return diff;
+          }
+        }
+        index--;
       }
+      return null;
     }
   }