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
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/Dispatcher.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/Dispatcher.java index 2e8cd5e..39effed 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/Dispatcher.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/Dispatcher.java
@@ -81,20 +81,34 @@ 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()) { - type = "," + type; + StringBuilder p = new StringBuilder(); + 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 @@ 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 @@ }.onSuccess(); } - public static void patch(String token, final Patch.Key id, - final int patchIndex, final PatchSetDetail patchSetDetail, - final PatchTable patchTable, final PatchScreen.TopView topView) { - patch(token, id, patchIndex, patchSetDetail, patchTable, topView, null); + public static void patch(String token, PatchSet.Id base, Patch.Key id, + int patchIndex, PatchSetDetail patchSetDetail, + PatchTable patchTable, PatchScreen.TopView topView) { + patch(token, base, id, patchIndex, patchSetDetail, patchTable, topView, null); } - public static void patch(String token, final Patch.Key id, - final int patchIndex, final PatchSetDetail patchSetDetail, - final PatchTable patchTable, final String panelType) { - patch(token, id, patchIndex, patchSetDetail, patchTable, + public static void patch(String token, PatchSet.Id base, Patch.Key id, + int patchIndex, PatchSetDetail patchSetDetail, + PatchTable patchTable, String panelType) { + 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 @@ patchIndex, // patchSetDetail, // patchTable, // - top // + top, // + baseId // ); } else if ("unified".equals(panel)) { return new PatchScreen.Unified( // @@ -463,7 +487,8 @@ patchIndex, // patchSetDetail, // patchTable, // - top // + top, // + baseId // ); } }
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java index d3c3f76..4f8eca8 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java
@@ -165,8 +165,7 @@ if (!patchSet.getId().equals(diffBaseId)) { patchTable = new PatchTable(); patchTable.setSavePointerId("PatchTable " + patchSet.getId()); - patchTable.setPatchSetIdToCompareWith(diffBaseId); - patchTable.display(detail); + patchTable.display(diffBaseId, detail); actionsPanel = new FlowPanel(); actionsPanel.setStyleName(Gerrit.RESOURCES.css().patchSetActions()); @@ -548,12 +547,10 @@ 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() + "#" - + Dispatcher.toPatchSideBySide(p.getKey()), "_blank", null); + openWindow(Dispatcher.toPatchSideBySide(diffBaseId, p.getKey())); } } }); @@ -561,18 +558,21 @@ 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() + "#" - + Dispatcher.toPatchUnified(p.getKey()), "_blank", null); + openWindow(Dispatcher.toPatchUnified(diffBaseId, p.getKey())); } } }); 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 @@ 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(result); + patchTable.display(diffBaseId, result); body.add(patchTable); for (ClickHandler clickHandler : registeredClickHandler) {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchTable.java index a028619..54bb697 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchTable.java
@@ -55,6 +55,7 @@ private Command onLoadCommand; private MyTable myTable; private String savePointerId; + private PatchSet.Id base; private List<Patch> patchList; private ListenableAccountDiffPreference listenablePrefs; @@ -62,8 +63,6 @@ private boolean active; private boolean registerKeys; - private PatchSet.Id patchSetIdToCompareWith; - public PatchTable(ListenableAccountDiffPreference prefs) { listenablePrefs = prefs; myBody = new FlowPanel(); @@ -83,12 +82,13 @@ 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 @@ 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 @@ 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 @@ Widget nameCol; if (patch.getPatchType() == Patch.PatchType.UNIFIED) { - nameCol = - new PatchLink.SideBySide(getDisplayFileName(patch), patch.getKey(), - row - 1, detail, PatchTable.this); + nameCol = new PatchLink.SideBySide(getDisplayFileName(patch), base, + patch.getKey(), row - 1, detail, PatchTable.this); } else { - nameCol = - new PatchLink.Unified(getDisplayFileName(patch), patch.getKey(), - row - 1, detail, PatchTable.this); + nameCol = new PatchLink.Unified(getDisplayFileName(patch), base, + patch.getKey(), row - 1, detail, PatchTable.this); } if (patch.getSourceFileName() != null) { final String text; @@ -409,16 +399,15 @@ int C_UNIFIED = C_SIDEBYSIDE + 1; if (patch.getPatchType() == Patch.PatchType.UNIFIED) { - table.setWidget(row, C_SIDEBYSIDE, new PatchLink.SideBySide(Util.C - .patchTableDiffSideBySide(), patch.getKey(), row - 1, detail, - PatchTable.this)); - + table.setWidget(row, C_SIDEBYSIDE, new PatchLink.SideBySide( + Util.C.patchTableDiffSideBySide(), base, patch.getKey(), row - 1, + 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 - .patchTableDiffUnified(), patch.getKey(), row - 1, detail, - PatchTable.this)); + table.setWidget(row, C_UNIFIED, new PatchLink.Unified( + Util.C.patchTableDiffUnified(), base, patch.getKey(), row - 1, + detail, PatchTable.this)); } void appendHeader(final SafeHtmlBuilder m) {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java index 03ddfd0..59e171b 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java
@@ -297,8 +297,8 @@ draftsPanel.add(panel); // Parent table can be null here since we are not showing any // next/previous links - panel.add(new PatchLink.SideBySide(PatchTable - .getDisplayFileName(patchKey), patchKey, 0, null, null)); + panel.add(new PatchLink.SideBySide( + PatchTable.getDisplayFileName(patchKey), null, patchKey, 0, null, null)); priorFile = fn; }
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/HistoryTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/HistoryTable.java index 3e90050..e79ca15 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/HistoryTable.java +++ b/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 @@ } 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) {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java index 83f24b3..59bf71d 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java
@@ -54,8 +54,8 @@ public static class SideBySide extends PatchScreen { public SideBySide(final Patch.Key id, final int patchIndex, final PatchSetDetail patchSetDetail, final PatchTable patchTable, - final TopView topView) { - super(id, patchIndex, patchSetDetail, patchTable, topView); + final TopView topView, final PatchSet.Id baseId) { + super(id, patchIndex, patchSetDetail, patchTable, topView, baseId); } @Override @@ -72,8 +72,8 @@ public static class Unified extends PatchScreen { public Unified(final Patch.Key id, final int patchIndex, final PatchSetDetail patchSetDetail, final PatchTable patchTable, - final TopView topView) { - super(id, patchIndex, patchSetDetail, patchTable, topView); + final TopView topView, final PatchSet.Id baseId) { + super(id, patchIndex, patchSetDetail, patchTable, topView, baseId); } @Override @@ -87,10 +87,6 @@ } } - // 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 @@ 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) { - diffSideA = patchTable.getPatchSetIdToCompareWith(); - } else { - diffSideA = null; - } - - idSideA = diffSideA; // null here means we're diff'ing from the Base - idSideB = diffSideB != null ? diffSideB : id.getParentKey(); + idSideA = baseId; // null here means we're diff'ing from the Base + idSideB = id.getParentKey(); this.patchIndex = patchIndex; reviewed = new CheckBox(Util.C.reviewed()); @@ -315,7 +305,7 @@ 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 PatchScreen.Type getPatchScreenType(); + public PatchSet.Id getSideA() { + return idSideA; + } + public Patch.Key getPatchKey() { return patchKey; } @@ -438,7 +432,7 @@ contentTable = new UnifiedDiffTable(); contentTable.fileList = fileList; contentPanel.add(contentTable); - setToken(Dispatcher.toPatchUnified(patchKey)); + setToken(Dispatcher.toPatchUnified(idSideA, patchKey)); } if (hasDifferences) { @@ -494,19 +488,6 @@ 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 @@ Util.DETAIL_SVC.patchSetDetail(psid, new GerritCallback<PatchSetDetail>() { public void onSuccess(final PatchSetDetail result) { - fileList.display(result); + fileList.display(idSideA, result); } }); }
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/PatchLink.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/PatchLink.java index 5028540..bd3d596 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/PatchLink.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/PatchLink.java
@@ -19,8 +19,10 @@ 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 @@ /** * @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, - final int patchIndex, final String historyToken, - final PatchSetDetail patchSetDetail, final PatchTable parentPatchTable, - final PatchScreen.TopView topView) { + protected PatchLink(String text, PatchSet.Id base, Patch.Key patchKey, + int patchIndex, String historyToken, + PatchSetDetail patchSetDetail, PatchTable parentPatchTable, + PatchScreen.TopView topView) { super(text, historyToken); + this.base = base; this.patchKey = patchKey; this.patchIndex = patchIndex; this.patchSetDetail = patchSetDetail; @@ -53,9 +57,9 @@ * @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, - final PatchScreen patchScreen) { + public PatchLink(String text, PatchScreen.Type type, PatchScreen patchScreen) { this(text, // + patchScreen.getSideA(), // patchScreen.getPatchKey(), // patchScreen.getPatchIndex(), // Dispatcher.toPatch(type, patchScreen.getPatchKey()), // @@ -69,6 +73,7 @@ public void go() { Dispatcher.patch( // getTargetHistoryToken(), // + base, // patchKey, // patchIndex, // patchSetDetail, // @@ -78,19 +83,21 @@ } public static class SideBySide extends PatchLink { - public SideBySide(final String text, final Patch.Key patchKey, - final int patchIndex, PatchSetDetail patchSetDetail, + public SideBySide(String text, PatchSet.Id base, Patch.Key patchKey, + 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, - final int patchIndex, PatchSetDetail patchSetDetail, + public Unified(String text, PatchSet.Id base, final Patch.Key patchKey, + 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); } }