Allow deleted and/or uncommented files to be skipped in reviews

Add two checkboxes to the user diff preferences.  These
buttons enable/disable skipping deleted or uncommented files.
Skipping is with respect to the prev/next links above and
below the diff.

Bug: issue 584
Change-Id: I105ee3115f806f1637997e95e9e9931b4107a69e

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

@@ -17,10 +17,12 @@ package com.google.gerrit.client.changes;
 import com.google.gerrit.client.Gerrit;
 import com.google.gerrit.client.patches.PatchScreen;
 import com.google.gerrit.client.ui.InlineHyperlink;
+import com.google.gerrit.client.ui.ListenableAccountDiffPreference;
 import com.google.gerrit.client.ui.NavigationTable;
 import com.google.gerrit.client.ui.PatchLink;
 import com.google.gerrit.common.data.PatchSetDetail;
 import com.google.gerrit.reviewdb.Patch;
+import com.google.gerrit.reviewdb.Patch.ChangeType;
 import com.google.gerrit.reviewdb.Patch.Key;
 import com.google.gerrit.reviewdb.Patch.PatchType;
 import com.google.gwt.core.client.GWT;
@@ -53,16 +55,22 @@ public class PatchTable extends Composite {
   private MyTable myTable;
   private String savePointerId;
   private List<Patch> patchList;
+  private ListenableAccountDiffPreference listenablePrefs;
 
   private List<ClickHandler> clickHandlers;
   private boolean active;
   private boolean registerKeys;
 
-  public PatchTable() {
+  public PatchTable(ListenableAccountDiffPreference prefs) {
+    listenablePrefs = prefs;
     myBody = new FlowPanel();
     initWidget(myBody);
   }
 
+  public PatchTable() {
+    this(new ListenableAccountDiffPreference());
+  }
+
   public int indexOf(Patch.Key patch) {
     for (int i = 0; i < patchList.size(); i++) {
       if (patchList.get(i).getKey().equals(patch)) {
@@ -166,9 +174,13 @@ public class PatchTable extends Composite {
    * @return a link to the previous file in this patch set, or null.
    */
   public InlineHyperlink getPreviousPatchLink(int index, PatchScreen.Type patchType) {
-    if (0 < index)
-      return createLink(index - 1, patchType, SafeHtml.asis(Util.C
+    for(index--; index > -1; index--) {
+      InlineHyperlink link = createLink(index, patchType, SafeHtml.asis(Util.C
           .prevPatchLinkIcon()), null);
+      if (link != null) {
+        return link;
+      }
+    }
     return null;
   }
 
@@ -176,9 +188,13 @@ public class PatchTable extends Composite {
    * @return a link to the next file in this patch set, or null.
    */
   public InlineHyperlink getNextPatchLink(int index, PatchScreen.Type patchType) {
-    if (index < patchList.size() - 1)
-      return createLink(index + 1, patchType, null, SafeHtml.asis(Util.C
+    for(index++; index < patchList.size(); index++) {
+      InlineHyperlink link = createLink(index, patchType, null, SafeHtml.asis(Util.C
           .nextPatchLinkIcon()));
+      if (link != null) {
+        return link;
+      }
+    }
     return null;
   }
 
@@ -192,6 +208,14 @@ public class PatchTable extends Composite {
   private PatchLink createLink(int index, PatchScreen.Type patchType,
       SafeHtml before, SafeHtml after) {
     Patch patch = patchList.get(index);
+    if (( listenablePrefs.get().isSkipDeleted() &&
+          patch.getChangeType().equals(ChangeType.DELETED) )
+        ||
+        ( listenablePrefs.get().isSkipUncommented() &&
+          patch.getCommentCount() == 0 ) ) {
+      return null;
+    }
+
     Key thisKey = patch.getKey();
     PatchLink link;
     if (patchType == PatchScreen.Type.SIDE_BY_SIDE
@@ -240,6 +264,14 @@ public class PatchTable extends Composite {
     }
   }
 
+  public ListenableAccountDiffPreference getPreferences() {
+    return listenablePrefs;
+  }
+
+  public void setPreferences(ListenableAccountDiffPreference prefs) {
+    listenablePrefs = prefs;
+  }
+
   private class MyTable extends NavigationTable<Patch> {
     private static final int C_PATH = 2;
     private static final int C_DRAFT = 3;
@@ -756,5 +788,4 @@ public class PatchTable extends Composite {
       return System.currentTimeMillis() - start > 200;
     }
   }
-
 }

gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java

@@ -135,6 +135,7 @@ public abstract class PatchScreen extends Screen implements
 
   /** The index of the file we are currently looking at among the fileList */
   private int patchIndex;
+  private ListenableAccountDiffPreference prefs;
 
   /** Keys that cause an action on this screen */
   private KeyCommandSet keysNavigation;
@@ -167,9 +168,10 @@ public abstract class PatchScreen extends Screen implements
     idSideB = diffSideB != null ? diffSideB : id.getParentKey();
     this.patchIndex = patchIndex;
 
-    ListenableAccountDiffPreference prefs =
-      new ListenableAccountDiffPreference();
-    prefs.addValueChangeHandler(new ValueChangeHandler<AccountDiffPreference>() {
+    prefs = fileList != null ? fileList.getPreferences() :
+                               new ListenableAccountDiffPreference();
+    prefs.addValueChangeHandler(
+        new ValueChangeHandler<AccountDiffPreference>() {
           @Override
           public void onValueChange(ValueChangeEvent<AccountDiffPreference> event) {
             update(event.getValue());
@@ -328,11 +330,9 @@ public abstract class PatchScreen extends Screen implements
             public void onSuccess(PatchSetDetail result) {
               patchSetDetail = result;
               if (fileList == null) {
-                fileList = new PatchTable();
+                fileList = new PatchTable(prefs);
                 fileList.display(result);
                 patchIndex = fileList.indexOf(patchKey);
-                topNav.display(patchIndex, getPatchScreenType(), fileList);
-                bottomNav.display(patchIndex, getPatchScreenType(), fileList);
               }
               refresh(true);
             }
@@ -355,6 +355,10 @@ public abstract class PatchScreen extends Screen implements
   public void registerKeys() {
     super.registerKeys();
     contentTable.setRegisterKeys(contentTable.isVisible());
+    if (regNavigation != null) {
+      regNavigation.removeHandler();
+      regNavigation = null;
+    }
     regNavigation = GlobalKey.add(this, keysNavigation);
   }
 
@@ -386,6 +390,7 @@ public abstract class PatchScreen extends Screen implements
   }
 
   private void onResult(final PatchScript script, final boolean isFirst) {
+
     final Change.Key cid = script.getChangeId();
     final String path = PatchTable.getDisplayFileName(patchKey);
     String fileName = path;
@@ -446,6 +451,12 @@ public abstract class PatchScreen extends Screen implements
     settingsPanel.setEnabled(true);
     lastScript = script;
 
+    if (fileList != null) {
+      topNav.display(patchIndex, getPatchScreenType(), fileList);
+      bottomNav.display(patchIndex, getPatchScreenType(), fileList);
+    }
+    registerKeys();
+
     // Mark this file reviewed as soon we display the diff screen
     if (Gerrit.isSignedIn() && isFirst) {
       settingsPanel.getReviewedCheckBox().setValue(true);
@@ -478,7 +489,7 @@ public abstract class PatchScreen extends Screen implements
     public void onKeyPress(final KeyPressEvent event) {
       if (fileList == null || fileList.isAttached()) {
         final PatchSet.Id psid = patchKey.getParentKey();
-        fileList = new PatchTable();
+        fileList = new PatchTable(prefs);
         fileList.setSavePointerId("PatchTable " + psid);
         Util.DETAIL_SVC.patchSetDetail(psid,
             new GerritCallback<PatchSetDetail>() {

gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScriptSettingsPanel.java

@@ -80,6 +80,13 @@ public class PatchScriptSettingsPanel extends Composite implements
   @UiField
   CheckBox reviewed;
 
+  @UiField
+  CheckBox skipDeleted;
+
+  @UiField
+  CheckBox skipUncommented;
+
+
   @UiField
   Button update;
 
@@ -199,6 +206,8 @@ public class PatchScriptSettingsPanel extends Composite implements
     intralineDifference.setValue(dp.isIntralineDifference());
     whitespaceErrors.setValue(dp.isShowWhitespaceErrors());
     showTabs.setValue(dp.isShowTabs());
+    skipDeleted.setValue(dp.isSkipDeleted());
+    skipUncommented.setValue(dp.isSkipUncommented());
   }
 
   @UiHandler("update")
@@ -216,6 +225,8 @@ public class PatchScriptSettingsPanel extends Composite implements
     dp.setIntralineDifference(intralineDifference.getValue());
     dp.setShowWhitespaceErrors(whitespaceErrors.getValue());
     dp.setShowTabs(showTabs.getValue());
+    dp.setSkipDeleted(skipDeleted.getValue());
+    dp.setSkipUncommented(skipUncommented.getValue());
 
     listenablePrefs.set(dp);
 

gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScriptSettingsPanel.ui.xml

@@ -115,12 +115,28 @@ limitations under the License.
       </g:CheckBox>
     </td>
 
+    <td rowspan='2'>
+      <g:CheckBox
+          ui:field='skipUncommented'
+          text='Skip Uncommented Files'
+          tabIndex='9'>
+        <ui:attribute name='text'/>
+      </g:CheckBox>
+      <br/>
+      <g:CheckBox
+          ui:field='skipDeleted'
+          text='Skip Deleted Files'
+          tabIndex='10'>
+        <ui:attribute name='text'/>
+      </g:CheckBox>
+    </td>
+
     <td valign='bottom' rowspan='2'>
       <g:Button
           ui:field='update'
           text='Update'
           styleName='{style.updateButton}'
-          tabIndex='9'>
+          tabIndex='11'>
         <ui:attribute name='text'/>
       </g:Button>
     </td>
@@ -129,7 +145,7 @@ limitations under the License.
       <g:CheckBox
           ui:field='reviewed'
           text='Reviewed'
-          tabIndex='10'>
+          tabIndex='12'>
         <ui:attribute name='text'/>
       </g:CheckBox>
     </td>

gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/AccountDiffPreference.java

@@ -96,6 +96,12 @@ public class AccountDiffPreference {
   @Column(id = 9)
   protected short context;
 
+  @Column(id = 10)
+  protected boolean skipDeleted;
+
+  @Column(id = 11)
+  protected boolean skipUncommented;
+
   protected AccountDiffPreference() {
   }
 
@@ -112,6 +118,8 @@ public class AccountDiffPreference {
     this.showWhitespaceErrors = p.showWhitespaceErrors;
     this.intralineDifference = p.intralineDifference;
     this.showTabs = p.showTabs;
+    this.skipDeleted = p.skipDeleted;
+    this.skipUncommented = p.skipUncommented;
     this.context = p.context;
   }
 
@@ -185,4 +193,20 @@ public class AccountDiffPreference {
     assert 0 <= context || context == WHOLE_FILE_CONTEXT;
     this.context = context;
   }
+
+  public boolean isSkipDeleted() {
+    return skipDeleted;
+  }
+
+  public void setSkipDeleted(boolean skip) {
+    skipDeleted = skip;
+  }
+
+  public boolean isSkipUncommented() {
+    return skipUncommented;
+  }
+
+  public void setSkipUncommented(boolean skip) {
+    skipUncommented = skip;
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java

@@ -32,7 +32,7 @@ import java.util.List;
 /** A version of the database schema. */
 public abstract class SchemaVersion {
   /** The current schema version. */
-  private static final Class<? extends SchemaVersion> C = Schema_43.class;
+  private static final Class<? extends SchemaVersion> C = Schema_44.class;
 
   public static class Module extends AbstractModule {
     @Override

gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_44.java

@@ -0,0 +1,25 @@
+// Copyright (C) 2010 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.schema;
+
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+
+public class Schema_44 extends SchemaVersion {
+  @Inject
+  Schema_44(Provider<Schema_43> prior) {
+    super(prior);
+  }
+}