Implement web interface for patches diff

Support diff between patches in the change screen. Allow changing
patches base comparison.

All displayed patches can now have their files listed and compared
to target branch or to a specific patch of the change.

Bug issue: 194
Change-Id: I617cbe647338b8a4fb71788f04ab6e2c952780fc
This commit is contained in:
monica.dionisio
2011-05-04 11:18:03 -03:00
committed by Shawn O. Pearce
parent 283a13df29
commit 237174c276
9 changed files with 239 additions and 42 deletions

View File

@@ -188,4 +188,6 @@ public interface GerritCss extends CssResource {
String usernameField();
String version();
String watchedProjectFilter();
String selectPatchSetOldVersion();
String patchCellReverseDiff();
}

View File

@@ -107,6 +107,8 @@ public interface ChangeConstants extends Constants {
String buttonAbandonChangeCancel();
String headingAbandonMessage();
String abandonChangeTitle();
String oldVersionHistory();
String baseDiffItem();
String buttonReview();
String buttonPublishCommentsSend();

View File

@@ -84,6 +84,8 @@ buttonAbandonChangeSend = Abandon Change
buttonAbandonChangeCancel = Cancel
headingAbandonMessage = Abandon Message:
abandonChangeTitle = Code Review - Abandon Change
oldVersionHistory = Old Version History:
baseDiffItem = Base
buttonRestoreChangeBegin = Restore Change
restoreChangeTitle = Code Review - Restore Change

View File

@@ -29,9 +29,11 @@ import com.google.gerrit.common.data.ChangeInfo;
import com.google.gerrit.common.data.ToggleStarRequest;
import com.google.gerrit.reviewdb.Account;
import com.google.gerrit.reviewdb.Change;
import com.google.gerrit.reviewdb.Change.Status;
import com.google.gerrit.reviewdb.ChangeMessage;
import com.google.gerrit.reviewdb.PatchSet;
import com.google.gerrit.reviewdb.Change.Status;
import com.google.gwt.event.dom.client.ChangeEvent;
import com.google.gwt.event.dom.client.ChangeHandler;
import com.google.gwt.event.dom.client.ClickEvent;
import com.google.gwt.event.dom.client.ClickHandler;
import com.google.gwt.event.dom.client.KeyPressEvent;
@@ -39,9 +41,11 @@ import com.google.gwt.event.shared.HandlerRegistration;
import com.google.gwt.i18n.client.LocaleInfo;
import com.google.gwt.user.client.ui.DisclosurePanel;
import com.google.gwt.user.client.ui.FlowPanel;
import com.google.gwt.user.client.ui.Grid;
import com.google.gwt.user.client.ui.HorizontalPanel;
import com.google.gwt.user.client.ui.Image;
import com.google.gwt.user.client.ui.Label;
import com.google.gwt.user.client.ui.ListBox;
import com.google.gwt.user.client.ui.Panel;
import com.google.gwtexpui.globalkey.client.GlobalKey;
import com.google.gwtexpui.globalkey.client.KeyCommand;
@@ -77,9 +81,30 @@ public class ChangeScreen extends Screen {
private HandlerRegistration regNavigation;
private HandlerRegistration regAction;
private Grid patchesGrid;
private ListBox patchesList;
/**
* The change id for which the old version history is valid.
*/
private static Change.Id currentChangeId;
/**
* Which patch set id is the diff base.
*/
private static PatchSet.Id diffBaseId;
public ChangeScreen(final Change.Id toShow) {
changeId = toShow;
openPatchSetId = null;
// If we have any diff stored, make sure they are applicable to the
// current change, discard them otherwise.
//
if (currentChangeId != null && !currentChangeId.equals(toShow)) {
diffBaseId = null;
}
currentChangeId = toShow;
}
public ChangeScreen(final PatchSet.Id toShow) {
@@ -202,6 +227,30 @@ public class ChangeScreen extends Screen {
dependenciesPanel.setContent(dependencies);
add(dependenciesPanel);
patchesList = new ListBox();
patchesList.addChangeHandler(new ChangeHandler() {
@Override
public void onChange(ChangeEvent event) {
final int index = patchesList.getSelectedIndex();
final String selectedPatchSet = patchesList.getValue(index);
if (index == 0) {
diffBaseId = null;
} else {
diffBaseId = PatchSet.Id.parse(selectedPatchSet);
}
if (patchSetsBlock != null) {
patchSetsBlock.refresh(diffBaseId);
}
}
});
patchesList.addItem(Util.C.baseDiffItem());
patchesGrid = new Grid(1, 2);
patchesGrid.setStyleName(Gerrit.RESOURCES.css().selectPatchSetOldVersion());
patchesGrid.setText(0, 0, Util.C.oldVersionHistory());
patchesGrid.setWidget(0, 1, patchesList);
add(patchesGrid);
patchSetsBlock = new PatchSetsBlock(this);
add(patchSetsBlock);
@@ -257,7 +306,18 @@ public class ChangeScreen extends Screen {
approvals.display(detail.getChange(), detail.getMissingApprovals(), detail
.getApprovals());
patchSetsBlock.display(detail);
for (PatchSet pId : detail.getPatchSets()) {
if (patchesList != null) {
patchesList.addItem(Util.M.patchSetHeader(pId.getPatchSetId()), pId
.getId().toString());
}
}
if (diffBaseId != null && patchesList != null) {
patchesList.setSelectedIndex(diffBaseId.get());
}
patchSetsBlock.display(detail, diffBaseId);
if (openPatchSetId != null) {
patchSetsBlock.activate(openPatchSetId);
}

View File

@@ -20,13 +20,13 @@ import com.google.gerrit.client.Gerrit;
import com.google.gerrit.client.rpc.GerritCallback;
import com.google.gerrit.client.ui.AccountDashboardLink;
import com.google.gerrit.client.ui.ComplexDisclosurePanel;
import com.google.gerrit.client.ui.ListenableAccountDiffPreference;
import com.google.gerrit.common.data.ChangeDetail;
import com.google.gerrit.common.data.GitwebLink;
import com.google.gerrit.common.data.PatchSetDetail;
import com.google.gerrit.reviewdb.Account;
import com.google.gerrit.reviewdb.AccountDiffPreference;
import com.google.gerrit.reviewdb.AccountGeneralPreferences;
import com.google.gerrit.reviewdb.AccountGeneralPreferences.DownloadCommand;
import com.google.gerrit.reviewdb.AccountGeneralPreferences.DownloadScheme;
import com.google.gerrit.reviewdb.ApprovalCategory;
import com.google.gerrit.reviewdb.Change;
import com.google.gerrit.reviewdb.ChangeMessage;
@@ -35,6 +35,8 @@ import com.google.gerrit.reviewdb.PatchSet;
import com.google.gerrit.reviewdb.PatchSetInfo;
import com.google.gerrit.reviewdb.Project;
import com.google.gerrit.reviewdb.UserIdentity;
import com.google.gerrit.reviewdb.AccountGeneralPreferences.DownloadCommand;
import com.google.gerrit.reviewdb.AccountGeneralPreferences.DownloadScheme;
import com.google.gwt.core.client.GWT;
import com.google.gwt.event.dom.client.ClickEvent;
import com.google.gwt.event.dom.client.ClickHandler;
@@ -47,9 +49,9 @@ import com.google.gwt.user.client.ui.Button;
import com.google.gwt.user.client.ui.DisclosurePanel;
import com.google.gwt.user.client.ui.FlowPanel;
import com.google.gwt.user.client.ui.Grid;
import com.google.gwt.user.client.ui.HTMLTable.CellFormatter;
import com.google.gwt.user.client.ui.InlineLabel;
import com.google.gwt.user.client.ui.Panel;
import com.google.gwt.user.client.ui.HTMLTable.CellFormatter;
import com.google.gwtexpui.clippy.client.CopyableLabel;
import java.util.Collections;
@@ -74,6 +76,8 @@ class PatchSetComplexDisclosurePanel extends ComplexDisclosurePanel implements O
private PatchTable patchTable;
private final Set<ClickHandler> registeredClickHandler = new HashSet<ClickHandler>();
private PatchSet.Id diffBaseId;
/**
* Creates a closed complex disclosure panel for a patch set.
* The patch set details are loaded when the complex disclosure panel is opened.
@@ -116,6 +120,10 @@ class PatchSetComplexDisclosurePanel extends ComplexDisclosurePanel implements O
}
}
public void setDiffBaseId(PatchSet.Id diffBaseId) {
this.diffBaseId = diffBaseId;
}
/**
* Display the table showing the Author, Committer and Download links,
* followed by the action buttons.
@@ -145,13 +153,14 @@ class PatchSetComplexDisclosurePanel extends ComplexDisclosurePanel implements O
displayParents(info.getParents());
displayDownload();
body.add(infoTable);
if (!patchSet.getId().equals(diffBaseId)) {
patchTable = new PatchTable();
patchTable.setSavePointerId("PatchTable " + patchSet.getId());
patchTable.setPatchSetIdToCompareWith(diffBaseId);
patchTable.display(detail);
body.add(infoTable);
actionsPanel = new FlowPanel();
actionsPanel.setStyleName(Gerrit.RESOURCES.css().patchSetActions());
body.add(actionsPanel);
@@ -168,6 +177,7 @@ class PatchSetComplexDisclosurePanel extends ComplexDisclosurePanel implements O
patchTable.addClickHandler(clickHandler);
}
}
}
private void displayDownload() {
final Project.NameKey projectKey = changeDetail.getChange().getProject();
@@ -500,10 +510,51 @@ class PatchSetComplexDisclosurePanel extends ComplexDisclosurePanel implements O
actionsPanel.add(b);
}
public void refresh() {
AccountDiffPreference diffPrefs;
if (patchTable == null) {
diffPrefs = new ListenableAccountDiffPreference().get();
} else {
diffPrefs = patchTable.getPreferences().get();
}
Util.DETAIL_SVC.patchSetDetail(patchSet.getId(), diffBaseId, diffPrefs,
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);
body.add(patchTable);
for (ClickHandler clickHandler : registeredClickHandler) {
patchTable.addClickHandler(clickHandler);
}
}
}
});
}
@Override
public void onOpen(final OpenEvent<DisclosurePanel> event) {
if (infoTable == null) {
Util.DETAIL_SVC.patchSetDetail(patchSet.getId(), null, null,
AccountDiffPreference diffPrefs;
if (diffBaseId == null) {
diffPrefs = null;
} else {
diffPrefs = new ListenableAccountDiffPreference().get();
}
Util.DETAIL_SVC.patchSetDetail(patchSet.getId(), diffBaseId, diffPrefs,
new GerritCallback<PatchSetDetail>() {
public void onSuccess(final PatchSetDetail result) {
ensureLoaded(result);

View File

@@ -32,6 +32,7 @@ import com.google.gwtexpui.globalkey.client.GlobalKey;
import com.google.gwtexpui.globalkey.client.KeyCommand;
import com.google.gwtexpui.globalkey.client.KeyCommandSet;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
@@ -50,6 +51,8 @@ public class PatchSetsBlock extends Composite {
private final FlowPanel body;
private HandlerRegistration regNavigation;
private List<PatchSetComplexDisclosurePanel> patchSetPanelsList;
/**
* the patch set id of the patch set for which is the keyboard navigation is
* currently enabled
@@ -69,13 +72,19 @@ public class PatchSetsBlock extends Composite {
}
/** Adds UI elements for each patch set of the given change to this composite. */
public void display(final ChangeDetail detail) {
public void display(final ChangeDetail detail, final PatchSet.Id diffBaseId) {
clear();
final PatchSet currps = detail.getCurrentPatchSet();
currentPatchSetId = currps.getId();
patchSets = detail.getPatchSets();
final List<PatchSet.Id> changePatchSets = new ArrayList<PatchSet.Id>();
for (final PatchSet ps : patchSets) {
changePatchSets.add(ps.getId());
}
if (Gerrit.isSignedIn()) {
final AccountGeneralPreferences p =
Gerrit.getUserAccount().getGeneralPreferences();
@@ -84,13 +93,25 @@ public class PatchSetsBlock extends Composite {
}
}
patchSetPanelsList = new ArrayList<PatchSetComplexDisclosurePanel>();
for (final PatchSet ps : patchSets) {
final PatchSetComplexDisclosurePanel p;
if (ps == currps) {
add(new PatchSetComplexDisclosurePanel(parent, detail, detail
.getCurrentPatchSetDetail()));
} else {
add(new PatchSetComplexDisclosurePanel(parent, detail, ps));
p = new PatchSetComplexDisclosurePanel(parent, detail, detail
.getCurrentPatchSetDetail());
if (diffBaseId != null) {
p.setDiffBaseId(diffBaseId);
p.refresh();
}
} else {
p = new PatchSetComplexDisclosurePanel(parent, detail, ps);
if (diffBaseId != null) {
p.setDiffBaseId(diffBaseId);
}
}
add(p);
patchSetPanelsList.add(p);
}
}
@@ -100,6 +121,17 @@ public class PatchSetsBlock extends Composite {
patchSetPanels.clear();
}
public void refresh(final PatchSet.Id diffBaseId) {
if (patchSetPanelsList != null) {
for (final PatchSetComplexDisclosurePanel p : patchSetPanelsList) {
p.setDiffBaseId(diffBaseId);
if (p.isOpen()) {
p.refresh();
}
}
}
}
/**
* Adds the given patch set panel to this composite and ensures that handler
* to activate / deactivate keyboard navigation for the patch set panel are

View File

@@ -22,6 +22,7 @@ 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.PatchSet;
import com.google.gerrit.reviewdb.Patch.ChangeType;
import com.google.gerrit.reviewdb.Patch.Key;
import com.google.gerrit.reviewdb.Patch.PatchType;
@@ -61,6 +62,8 @@ public class PatchTable extends Composite {
private boolean active;
private boolean registerKeys;
private PatchSet.Id patchSetIdToCompareWith;
public PatchTable(ListenableAccountDiffPreference prefs) {
listenablePrefs = prefs;
myBody = new FlowPanel();
@@ -85,7 +88,7 @@ public class PatchTable extends Composite {
this.patchList = detail.getPatches();
myTable = null;
final DisplayCommand cmd = new DisplayCommand(patchList);
final DisplayCommand cmd = new DisplayCommand(patchList, patchSetIdToCompareWith);
if (cmd.execute()) {
cmd.initMeter();
Scheduler.get().scheduleIncremental(cmd);
@@ -272,6 +275,14 @@ 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;
@@ -279,6 +290,8 @@ public class PatchTable extends Composite {
private static final int C_SIDEBYSIDE = 5;
private int activeRow = -1;
private PatchSet.Id patchSetIdToCompareWith;
MyTable() {
keysNavigation.add(new PrevKeyCommand(0, 'k', Util.C.patchTablePrev()));
keysNavigation.add(new NextKeyCommand(0, 'j', Util.C.patchTableNext()));
@@ -463,7 +476,8 @@ public class PatchTable extends Composite {
m.closeTr();
}
void appendRow(final SafeHtmlBuilder m, final Patch p) {
void appendRow(final SafeHtmlBuilder m, final Patch p,
final boolean isReverseDiff) {
m.openTr();
m.openTd();
@@ -474,6 +488,10 @@ public class PatchTable extends Composite {
m.openTd();
m.setStyleName(Gerrit.RESOURCES.css().changeTypeCell());
if (isReverseDiff) {
m.addStyleName(Gerrit.RESOURCES.css().patchCellReverseDiff());
}
if (Patch.COMMIT_MSG.equals(p.getFileName())) {
m.nbsp();
} else {
@@ -494,7 +512,12 @@ public class PatchTable extends Composite {
m.openTd();
m.addStyleName(Gerrit.RESOURCES.css().dataCell());
m.addStyleName(Gerrit.RESOURCES.css().patchSizeCell());
if (isReverseDiff) {
m.addStyleName(Gerrit.RESOURCES.css().patchCellReverseDiff());
}
appendSize(m, p);
m.closeTd();
@@ -559,7 +582,8 @@ public class PatchTable extends Composite {
m.closeTr();
}
void appendTotals(final SafeHtmlBuilder m, int ins, int dels) {
void appendTotals(final SafeHtmlBuilder m, int ins, int dels,
final boolean isReverseDiff) {
m.openTr();
m.openTd();
@@ -576,6 +600,11 @@ public class PatchTable extends Composite {
m.addStyleName(Gerrit.RESOURCES.css().dataCell());
m.addStyleName(Gerrit.RESOURCES.css().patchSizeCell());
m.addStyleName(Gerrit.RESOURCES.css().leftMostCell());
if (isReverseDiff) {
m.addStyleName(Gerrit.RESOURCES.css().patchCellReverseDiff());
}
m.append(Util.M.patchTableSize_Modify(ins, dels));
m.closeTd();
@@ -699,9 +728,12 @@ public class PatchTable extends Composite {
private int insertions;
private int deletions;
private DisplayCommand(final List<Patch> list) {
private final PatchSet.Id psIdToCompareWith;
private DisplayCommand(final List<Patch> list, final PatchSet.Id psIdToCompareWith) {
this.table = new MyTable();
this.list = list;
this.psIdToCompareWith = psIdToCompareWith;
}
/**
@@ -722,24 +754,31 @@ public class PatchTable extends Composite {
return false;
}
boolean isReverseDiff = false;
if (psIdToCompareWith != null
&& list.get(0).getKey().getParentKey().get() < psIdToCompareWith.get()) {
isReverseDiff = true;
}
start = System.currentTimeMillis();
switch (stage) {
case 0:
if (row == 0) {
table.appendHeader(nc);
table.appendRow(nc, list.get(row++));
table.appendRow(nc, list.get(row++), isReverseDiff);
}
while (row < list.size()) {
Patch p = list.get(row);
insertions += p.getInsertions();
deletions += p.getDeletions();
table.appendRow(nc, p);
table.appendRow(nc, p, isReverseDiff);
if ((++row % 10) == 0 && longRunning()) {
updateMeter();
return true;
}
}
table.appendTotals(nc, insertions, deletions);
table.appendTotals(nc, insertions, deletions, isReverseDiff);
table.resetHtml(nc);
nc = null;
stage = 1;

View File

@@ -421,6 +421,10 @@
color: #ff5555;
}
.changeTable .patchCellReverseDiff {
color: red;
}
.changeTable .patchSizeCell {
text-align: right;
white-space: nowrap;
@@ -905,6 +909,12 @@
font-size: 8pt;
}
.selectPatchSetOldVersion {
font-weight: bold;
margin-right: 30px;
margin-top: 5px;
}
.changeScreen .approvalTable {
margin-top: 1em;
margin-bottom: 1em;

View File

@@ -156,16 +156,15 @@ public abstract class PatchScreen extends Screen implements
patchSetDetail = detail;
fileList = patchTable;
// If we have any diff side stored, make sure they are applicable to the
// current change, discard them otherwise.
//
Change.Id thisChangeId = id.getParentKey().getParentKey();
if (currentChangeId != null && !currentChangeId.equals(thisChangeId)) {
if (patchTable != null) {
diffSideA = patchTable.getPatchSetIdToCompareWith();
} else {
diffSideA = null;
diffSideB = null;
}
if (diffSideA == null) {
historyOpen = null;
}
currentChangeId = thisChangeId;
idSideA = diffSideA; // null here means we're diff'ing from the Base
idSideB = diffSideB != null ? diffSideB : id.getParentKey();
this.patchIndex = patchIndex;