Fix: "Diff All Side-by-Side" and "Diff All Unified"-buttons

When pressing the "Diff All Side-by-Side" or
"Diff All Unified"-button on the change screen, the
opened browser windows/tabs shows diffs using "Base"
as old version and the latest one as active patch set,
regardless what has been set using the
"Old Version History:" drop down menu and what is
currently active patch set.

Gerrit don't remember the base patch set in the URL,
making it impossible to copy-and-paste the URL to
co-workers to show them the same diff a user is
looking at.

This change fixes these behavior to make sure that
the opened new browser windows shows diffs using the
correct old patch set and active patch set by
updating these 2 buttons' click event handlers
in time and Gerrit can remember base patch set by
embeding it in URL. For example by default
'https://<gerrit site>/#/c/33090/1/<patch name>'
means comparing patch set 1 and 'Base' of change 33090.
When comparing patch set 3 and patch set 1, it
changes to '3..1' syntax as
'https://<gerrit site>/#/c/33090/3..1/<patch name>'.

Change-Id: I89d11e6038f01af48870bab98f14423a986df9cb

gerrit-gwtui/src/main/java/com/google/gerrit/client/Dispatcher.java

@@ -81,20 +81,34 @@ import com.google.gwtorm.client.KeyUtil;
 
 public class Dispatcher {
   public static String toPatchSideBySide(final Patch.Key id) {
-    return toPatch("", id);
+    return toPatch("", null, id);
+  }
+
+  public static String toPatchSideBySide(PatchSet.Id diffBase, Patch.Key id) {
+    return toPatch("", diffBase, id);
   }
 
   public static String toPatchUnified(final Patch.Key id) {
-    return toPatch("unified", id);
+    return toPatch("unified", null, id);
   }
 
-  private static String toPatch(String type, final Patch.Key id) {
+  public static String toPatchUnified(PatchSet.Id diffBase, Patch.Key id) {
+    return toPatch("unified", diffBase, id);
+  }
+
+  private static String toPatch(String type, PatchSet.Id diffBase, Patch.Key id) {
     PatchSet.Id ps = id.getParentKey();
     Change.Id c = ps.getParentKey();
-    if (type != null && !type.isEmpty()) {
+    StringBuilder p = new StringBuilder();
-      type = "," + type;
+    p.append("/c/").append(c).append("/");
+    if (diffBase != null) {
+      p.append(diffBase.get()).append("..");
     }
-    return "/c/" + c + "/" + ps.get() + "/" + KeyUtil.encode(id.get()) + type;
+    p.append(ps.get()).append("/").append(KeyUtil.encode(id.get()));
+    if (type != null && !type.isEmpty()) {
+      p.append(",").append(type);
+    }
+    return p.toString();
   }
 
   public static String toPatch(final PatchScreen.Type type, final Patch.Key id) {
@@ -387,10 +401,20 @@ public class Dispatcher {
       rest = "";
     }
 
-    PatchSet.Id ps = new PatchSet.Id(id, Integer.parseInt(psIdStr));
+    PatchSet.Id base;
+    PatchSet.Id ps;
+    int dotdot = psIdStr.indexOf("..");
+    if (1 <= dotdot) {
+      base = new PatchSet.Id(id, Integer.parseInt(psIdStr.substring(0, dotdot)));
+      ps = new PatchSet.Id(id, Integer.parseInt(psIdStr.substring(dotdot + 2)));
+    } else {
+      base = null;
+      ps = new PatchSet.Id(id, Integer.parseInt(psIdStr));
+    }
+
     if (!rest.isEmpty()) {
       Patch.Key p = new Patch.Key(ps, rest);
-      patch(token, p, 0, null, null, panel);
+      patch(token, base, p, 0, null, null, panel);
     } else {
       if (panel == null) {
         Gerrit.display(token, new ChangeScreen(ps));
@@ -415,24 +439,23 @@ public class Dispatcher {
     }.onSuccess();
   }
 
-  public static void patch(String token, final Patch.Key id,
+  public static void patch(String token, PatchSet.Id base, Patch.Key id,
-      final int patchIndex, final PatchSetDetail patchSetDetail,
+      int patchIndex, PatchSetDetail patchSetDetail,
-      final PatchTable patchTable, final PatchScreen.TopView topView) {
+      PatchTable patchTable, PatchScreen.TopView topView) {
-    patch(token, id, patchIndex, patchSetDetail, patchTable, topView, null);
+    patch(token, base, id, patchIndex, patchSetDetail, patchTable, topView, null);
   }
 
-  public static void patch(String token, final Patch.Key id,
+  public static void patch(String token, PatchSet.Id base, Patch.Key id,
-      final int patchIndex, final PatchSetDetail patchSetDetail,
+      int patchIndex, PatchSetDetail patchSetDetail,
-      final PatchTable patchTable, final String panelType) {
+      PatchTable patchTable, String panelType) {
-    patch(token, id, patchIndex, patchSetDetail, patchTable,
+    patch(token, base, id, patchIndex, patchSetDetail, patchTable,
         null, panelType);
   }
 
-  public static void patch(String token, final Patch.Key id,
+  public static void patch(String token, final PatchSet.Id baseId, final Patch.Key id,
       final int patchIndex, final PatchSetDetail patchSetDetail,
       final PatchTable patchTable, final PatchScreen.TopView topView,
       final String panelType) {
-
     final PatchScreen.TopView top =  topView == null ?
         Gerrit.getPatchScreenTopView() : topView;
 
@@ -455,7 +478,8 @@ public class Dispatcher {
                 patchIndex, //
                 patchSetDetail, //
                 patchTable, //
-                top //
+                top, //
+                baseId //
             );
           } else if ("unified".equals(panel)) {
             return new PatchScreen.Unified( //
@@ -463,7 +487,8 @@ public class Dispatcher {
                 patchIndex, //
                 patchSetDetail, //
                 patchTable, //
-                top //
+                top, //
+                baseId //
             );
           }
         }

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

@@ -165,8 +165,7 @@ class PatchSetComplexDisclosurePanel extends ComplexDisclosurePanel implements O
     if (!patchSet.getId().equals(diffBaseId)) {
       patchTable = new PatchTable();
       patchTable.setSavePointerId("PatchTable " + patchSet.getId());
-      patchTable.setPatchSetIdToCompareWith(diffBaseId);
+      patchTable.display(diffBaseId, detail);
-      patchTable.display(detail);
 
       actionsPanel = new FlowPanel();
       actionsPanel.setStyleName(Gerrit.RESOURCES.css().patchSetActions());
@@ -548,12 +547,10 @@ class PatchSetComplexDisclosurePanel extends ComplexDisclosurePanel implements O
   private void populateDiffAllActions(final PatchSetDetail detail) {
     final Button diffAllSideBySide = new Button(Util.C.buttonDiffAllSideBySide());
     diffAllSideBySide.addClickHandler(new ClickHandler() {
-
       @Override
       public void onClick(ClickEvent event) {
         for (Patch p : detail.getPatches()) {
-          Window.open(Window.Location.getPath() + "#"
+          openWindow(Dispatcher.toPatchSideBySide(diffBaseId, p.getKey()));
-              + Dispatcher.toPatchSideBySide(p.getKey()), "_blank", null);
         }
       }
     });
@@ -561,18 +558,21 @@ class PatchSetComplexDisclosurePanel extends ComplexDisclosurePanel implements O
 
     final Button diffAllUnified = new Button(Util.C.buttonDiffAllUnified());
     diffAllUnified.addClickHandler(new ClickHandler() {
-
       @Override
       public void onClick(ClickEvent event) {
         for (Patch p : detail.getPatches()) {
-          Window.open(Window.Location.getPath() + "#"
+          openWindow(Dispatcher.toPatchUnified(diffBaseId, p.getKey()));
-              + Dispatcher.toPatchUnified(p.getKey()), "_blank", null);
         }
       }
     });
     actionsPanel.add(diffAllUnified);
   }
 
+  private void openWindow(String token) {
+    String url = Window.Location.getPath() + "#" + token;
+    Window.open(url, "_blank", null);
+  }
+
   private void populateReviewAction() {
     final Button b = new Button(Util.C.buttonReview());
     b.addClickHandler(new ClickHandler() {
@@ -646,18 +646,15 @@ class PatchSetComplexDisclosurePanel extends ComplexDisclosurePanel implements O
         new GerritCallback<PatchSetDetail>() {
           @Override
           public void onSuccess(PatchSetDetail result) {
-
             if (patchSet.getId().equals(diffBaseId)) {
               patchTable.setVisible(false);
               actionsPanel.setVisible(false);
             } else {
-
               if (patchTable != null) {
                 patchTable.removeFromParent();
               }
               patchTable = new PatchTable();
-              patchTable.setPatchSetIdToCompareWith(diffBaseId);
+              patchTable.display(diffBaseId, result);
-              patchTable.display(result);
               body.add(patchTable);
 
               for (ClickHandler clickHandler : registeredClickHandler) {

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

@@ -55,6 +55,7 @@ public class PatchTable extends Composite {
   private Command onLoadCommand;
   private MyTable myTable;
   private String savePointerId;
+  private PatchSet.Id base;
   private List<Patch> patchList;
   private ListenableAccountDiffPreference listenablePrefs;
 
@@ -62,8 +63,6 @@ public class PatchTable extends Composite {
   private boolean active;
   private boolean registerKeys;
 
-  private PatchSet.Id patchSetIdToCompareWith;
-
   public PatchTable(ListenableAccountDiffPreference prefs) {
     listenablePrefs = prefs;
     myBody = new FlowPanel();
@@ -83,12 +82,13 @@ public class PatchTable extends Composite {
     return -1;
   }
 
-  public void display(PatchSetDetail detail) {
+  public void display(PatchSet.Id base, PatchSetDetail detail) {
+    this.base = base;
     this.detail = detail;
     this.patchList = detail.getPatches();
     myTable = null;
 
-    final DisplayCommand cmd = new DisplayCommand(patchList, patchSetIdToCompareWith);
+    final DisplayCommand cmd = new DisplayCommand(patchList, base);
     if (cmd.execute()) {
       cmd.initMeter();
       Scheduler.get().scheduleIncremental(cmd);
@@ -223,9 +223,9 @@ public class PatchTable extends Composite {
     PatchLink link;
     if (patchType == PatchScreen.Type.SIDE_BY_SIDE
         && patch.getPatchType() == Patch.PatchType.UNIFIED) {
-      link = new PatchLink.SideBySide("", thisKey, index, detail, this);
+      link = new PatchLink.SideBySide("", base, thisKey, index, detail, this);
     } else {
-      link = new PatchLink.Unified("", thisKey, index, detail, this);
+      link = new PatchLink.Unified("", base, thisKey, index, detail, this);
     }
     SafeHtmlBuilder text = new SafeHtmlBuilder();
     text.append(before);
@@ -275,14 +275,6 @@ public class PatchTable extends Composite {
     listenablePrefs = prefs;
   }
 
-  public PatchSet.Id getPatchSetIdToCompareWith() {
-    return patchSetIdToCompareWith;
-  }
-
-  public void setPatchSetIdToCompareWith(final PatchSet.Id psId) {
-    patchSetIdToCompareWith = psId;
-  }
-
   private class MyTable extends NavigationTable<Patch> {
     private static final int C_PATH = 2;
     private static final int C_DRAFT = 3;
@@ -381,13 +373,11 @@ public class PatchTable extends Composite {
 
       Widget nameCol;
       if (patch.getPatchType() == Patch.PatchType.UNIFIED) {
-        nameCol =
+        nameCol = new PatchLink.SideBySide(getDisplayFileName(patch), base,
-            new PatchLink.SideBySide(getDisplayFileName(patch), patch.getKey(),
+            patch.getKey(), row - 1, detail, PatchTable.this);
-                row - 1, detail, PatchTable.this);
       } else {
-        nameCol =
+        nameCol = new PatchLink.Unified(getDisplayFileName(patch), base,
-            new PatchLink.Unified(getDisplayFileName(patch), patch.getKey(),
+            patch.getKey(), row - 1, detail, PatchTable.this);
-                row - 1, detail, PatchTable.this);
       }
       if (patch.getSourceFileName() != null) {
         final String text;
@@ -409,16 +399,15 @@ public class PatchTable extends Composite {
 
       int C_UNIFIED = C_SIDEBYSIDE + 1;
       if (patch.getPatchType() == Patch.PatchType.UNIFIED) {
-        table.setWidget(row, C_SIDEBYSIDE, new PatchLink.SideBySide(Util.C
+        table.setWidget(row, C_SIDEBYSIDE, new PatchLink.SideBySide(
-            .patchTableDiffSideBySide(), patch.getKey(), row - 1, detail,
+            Util.C.patchTableDiffSideBySide(), base, patch.getKey(), row - 1,
-            PatchTable.this));
+            detail, PatchTable.this));
-
       } else if (patch.getPatchType() == Patch.PatchType.BINARY) {
         C_UNIFIED = C_SIDEBYSIDE + 2;
       }
-      table.setWidget(row, C_UNIFIED, new PatchLink.Unified(Util.C
+      table.setWidget(row, C_UNIFIED, new PatchLink.Unified(
-          .patchTableDiffUnified(), patch.getKey(), row - 1, detail,
+          Util.C.patchTableDiffUnified(), base, patch.getKey(), row - 1,
-          PatchTable.this));
+          detail, PatchTable.this));
     }
 
     void appendHeader(final SafeHtmlBuilder m) {

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

@@ -297,8 +297,8 @@ public class PublishCommentScreen extends AccountScreen implements
           draftsPanel.add(panel);
           // Parent table can be null here since we are not showing any
           // next/previous links
-          panel.add(new PatchLink.SideBySide(PatchTable
+          panel.add(new PatchLink.SideBySide(
-              .getDisplayFileName(patchKey), patchKey, 0, null, null));
+              PatchTable.getDisplayFileName(patchKey), null, patchKey, 0, null, null));
           priorFile = fn;
         }
 

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

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.client.patches;
 
+import com.google.gerrit.client.Dispatcher;
 import com.google.gerrit.client.Gerrit;
 import com.google.gerrit.client.changes.Util;
 import com.google.gerrit.client.ui.FancyFlexTable;
@@ -43,19 +44,28 @@ class HistoryTable extends FancyFlexTable<Patch> {
   }
 
   void onClick(final HistoryRadio b) {
+    PatchSet.Id sideA = screen.idSideA;
+    PatchSet.Id sideB = screen.idSideB;
     switch (b.file) {
       case 0:
-        screen.setSideA(b.patchSetId);
+        sideA = b.patchSetId;
         break;
       case 1:
-        screen.setSideB(b.patchSetId);
+        sideB = b.patchSetId;
         break;
       default:
         return;
     }
-
     enableAll(false);
-    screen.refresh(false);
+    Patch.Key k = new Patch.Key(sideB, screen.getPatchKey().get());
+    switch (screen.getPatchScreenType()) {
+      case SIDE_BY_SIDE:
+        Gerrit.display(Dispatcher.toPatchSideBySide(sideA, k));
+        break;
+      case UNIFIED:
+        Gerrit.display(Dispatcher.toPatchUnified(sideA, k));
+        break;
+    }
   }
 
   void enableAll(final boolean on) {

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

@@ -54,8 +54,8 @@ public abstract class PatchScreen extends Screen implements
   public static class SideBySide extends PatchScreen {
     public SideBySide(final Patch.Key id, final int patchIndex,
         final PatchSetDetail patchSetDetail, final PatchTable patchTable,
-        final TopView topView) {
+        final TopView topView, final PatchSet.Id baseId) {
-      super(id, patchIndex, patchSetDetail, patchTable, topView);
+       super(id, patchIndex, patchSetDetail, patchTable, topView, baseId);
     }
 
     @Override
@@ -72,8 +72,8 @@ public abstract class PatchScreen extends Screen implements
   public static class Unified extends PatchScreen {
     public Unified(final Patch.Key id, final int patchIndex,
         final PatchSetDetail patchSetDetail, final PatchTable patchTable,
-        final TopView topView) {
+        final TopView topView, final PatchSet.Id baseId) {
-      super(id, patchIndex, patchSetDetail, patchTable, topView);
+      super(id, patchIndex, patchSetDetail, patchTable, topView, baseId);
     }
 
     @Override
@@ -87,10 +87,6 @@ public abstract class PatchScreen extends Screen implements
     }
   }
 
-  // Which patch set id's are being diff'ed
-  private static PatchSet.Id diffSideA = null;
-  private static PatchSet.Id diffSideB = null;
-
   /**
    * What should be displayed in the top of the screen
    */
@@ -137,20 +133,14 @@ public abstract class PatchScreen extends Screen implements
 
   protected PatchScreen(final Patch.Key id, final int patchIndex,
       final PatchSetDetail detail, final PatchTable patchTable,
-      final TopView top) {
+      final TopView top, final PatchSet.Id baseId) {
     patchKey = id;
     patchSetDetail = detail;
     fileList = patchTable;
     topView = top;
 
-    if (patchTable != null) {
+    idSideA = baseId; // null here means we're diff'ing from the Base
-      diffSideA = patchTable.getPatchSetIdToCompareWith();
+    idSideB = id.getParentKey();
-    } else {
-      diffSideA = null;
-    }
-
-    idSideA = diffSideA; // null here means we're diff'ing from the Base
-    idSideB = diffSideB != null ? diffSideB : id.getParentKey();
     this.patchIndex = patchIndex;
 
     reviewed = new CheckBox(Util.C.reviewed());
@@ -315,7 +305,7 @@ public abstract class PatchScreen extends Screen implements
               patchSetDetail = result;
               if (fileList == null) {
                 fileList = new PatchTable(prefs);
-                fileList.display(result);
+                fileList.display(idSideA, result);
                 patchIndex = fileList.indexOf(patchKey);
               }
               refresh(true);
@@ -350,6 +340,10 @@ public abstract class PatchScreen extends Screen implements
 
   public abstract PatchScreen.Type getPatchScreenType();
 
+  public PatchSet.Id getSideA() {
+    return idSideA;
+  }
+
   public Patch.Key getPatchKey() {
     return patchKey;
   }
@@ -438,7 +432,7 @@ public abstract class PatchScreen extends Screen implements
       contentTable = new UnifiedDiffTable();
       contentTable.fileList = fileList;
       contentPanel.add(contentTable);
-      setToken(Dispatcher.toPatchUnified(patchKey));
+      setToken(Dispatcher.toPatchUnified(idSideA, patchKey));
     }
 
     if (hasDifferences) {
@@ -494,19 +488,6 @@ public abstract class PatchScreen extends Screen implements
     contentTable.setRegisterKeys(isCurrentView() && showPatch);
   }
 
-  public void setSideA(PatchSet.Id patchSetId) {
-    idSideA = patchSetId;
-    diffSideA = patchSetId;
-    if (fileList != null) {
-      fileList.setPatchSetIdToCompareWith(patchSetId);
-    }
-  }
-
-  public void setSideB(PatchSet.Id patchSetId) {
-    idSideB = patchSetId;
-    diffSideB = patchSetId;
-  }
-
   public void setTopView(TopView tv) {
     topView = tv;
     topPanel.clear();
@@ -536,7 +517,7 @@ public abstract class PatchScreen extends Screen implements
         Util.DETAIL_SVC.patchSetDetail(psid,
             new GerritCallback<PatchSetDetail>() {
               public void onSuccess(final PatchSetDetail result) {
-                fileList.display(result);
+                fileList.display(idSideA, result);
               }
             });
       }

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

@@ -19,8 +19,10 @@ import com.google.gerrit.client.changes.PatchTable;
 import com.google.gerrit.client.patches.PatchScreen;
 import com.google.gerrit.common.data.PatchSetDetail;
 import com.google.gerrit.reviewdb.Patch;
+import com.google.gerrit.reviewdb.PatchSet;
 
 public class PatchLink extends InlineHyperlink {
+  protected PatchSet.Id base;
   protected Patch.Key patchKey;
   protected int patchIndex;
   protected PatchSetDetail patchSetDetail;
@@ -29,17 +31,19 @@ public class PatchLink extends InlineHyperlink {
 
   /**
    * @param text The text of this link
+   * @param base optional base to compare against.
    * @param patchKey The key for this patch
    * @param patchIndex The index of the current patch in the patch set
    * @param historyToken The history token
    * @param patchSetDetail Detailed information about the patch set.
    * @param parentPatchTable The table used to display this link
    */
-  protected PatchLink(final String text, final Patch.Key patchKey,
+  protected PatchLink(String text, PatchSet.Id base, Patch.Key patchKey,
-      final int patchIndex, final String historyToken,
+      int patchIndex, String historyToken,
-      final PatchSetDetail patchSetDetail, final PatchTable parentPatchTable,
+      PatchSetDetail patchSetDetail, PatchTable parentPatchTable,
-      final PatchScreen.TopView topView) {
+      PatchScreen.TopView topView) {
     super(text, historyToken);
+    this.base = base;
     this.patchKey = patchKey;
     this.patchIndex = patchIndex;
     this.patchSetDetail = patchSetDetail;
@@ -53,9 +57,9 @@ public class PatchLink extends InlineHyperlink {
    * @param type The type of the link to create (unified/side-by-side)
    * @param patchScreen The patchScreen to grab contents to link to from
    */
-  public PatchLink(final String text, final PatchScreen.Type type,
+  public PatchLink(String text, PatchScreen.Type type, PatchScreen patchScreen) {
-      final PatchScreen patchScreen) {
     this(text, //
+        patchScreen.getSideA(), //
         patchScreen.getPatchKey(), //
         patchScreen.getPatchIndex(), //
         Dispatcher.toPatch(type, patchScreen.getPatchKey()), //
@@ -69,6 +73,7 @@ public class PatchLink extends InlineHyperlink {
   public void go() {
     Dispatcher.patch( //
         getTargetHistoryToken(), //
+        base, //
         patchKey, //
         patchIndex, //
         patchSetDetail, //
@@ -78,19 +83,21 @@ public class PatchLink extends InlineHyperlink {
   }
 
   public static class SideBySide extends PatchLink {
-    public SideBySide(final String text, final Patch.Key patchKey,
+    public SideBySide(String text, PatchSet.Id base, Patch.Key patchKey,
-        final int patchIndex, PatchSetDetail patchSetDetail,
+        int patchIndex, PatchSetDetail patchSetDetail,
         PatchTable parentPatchTable) {
-      super(text, patchKey, patchIndex, Dispatcher.toPatchSideBySide(patchKey),
+      super(text, base, patchKey, patchIndex,
+          Dispatcher.toPatchSideBySide(base, patchKey),
           patchSetDetail, parentPatchTable, null);
     }
   }
 
   public static class Unified extends PatchLink {
-    public Unified(final String text, final Patch.Key patchKey,
+    public Unified(String text, PatchSet.Id base, final Patch.Key patchKey,
-        final int patchIndex, PatchSetDetail patchSetDetail,
+        int patchIndex, PatchSetDetail patchSetDetail,
         PatchTable parentPatchTable) {
-      super(text, patchKey, patchIndex, Dispatcher.toPatchUnified(patchKey),
+      super(text, base, patchKey, patchIndex,
+          Dispatcher.toPatchUnified(base, patchKey),
           patchSetDetail, parentPatchTable, null);
     }
   }