Skip to content

Commit

Permalink
Fix browser NPE when ChangeCache is incomplete
Browse files Browse the repository at this point in the history
The ChangeCache is buggy and does not always populate its
contents. Unfortunately it is also designed to be unable to
load missing contents on demand, causing consumers to NPE.
ConfigInfoCache is a better design for this sort of lazy
loading and reuse of data.

ChangeCache can be missing information if:

  1) opens side-by-side view in a new tab;
  2) user presses 'r' to open publish comment screen

Instead of using the unreliable ChangeCache, stub out a
ChangeDetail to feed to the info block on the publish
comments screen.

Change-Id: Id542528d93af1cab49b001ca5e90addfc5d05b78
  • Loading branch information
spearce committed Aug 13, 2013
1 parent da5153f commit f646aa7
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 13 deletions.
Expand Up @@ -16,8 +16,8 @@

import com.google.gerrit.client.ui.CommentLinkProcessor;
import com.google.gerrit.common.data.AccountInfoCache;
import com.google.gerrit.common.data.ChangeDetail;
import com.google.gerrit.common.data.SubmitTypeRecord;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gwt.user.client.ui.Composite;
import com.google.gwt.user.client.ui.HorizontalPanel;
Expand All @@ -37,12 +37,12 @@ public ChangeDescriptionBlock(KeyCommandSet keysAction) {
initWidget(hp);
}

public void display(Change chg, Boolean starred, Boolean canEditCommitMessage,
public void display(ChangeDetail changeDetail, Boolean starred, Boolean canEditCommitMessage,
PatchSetInfo info, AccountInfoCache acc,
SubmitTypeRecord submitTypeRecord,
CommentLinkProcessor commentLinkProcessor) {
infoBlock.display(chg, acc, submitTypeRecord);
messageBlock.display(chg.currentPatchSetId(), starred,
infoBlock.display(changeDetail, acc, submitTypeRecord);
messageBlock.display(changeDetail.getChange().currentPatchSetId(), starred,
canEditCommitMessage, info.getMessage(), commentLinkProcessor);
}
}
Expand Up @@ -95,8 +95,9 @@ private void initRow(final int row, final String name) {
table.getCellFormatter().addStyleName(row, 0, Gerrit.RESOURCES.css().header());
}

public void display(final Change chg, final AccountInfoCache acc,
SubmitTypeRecord submitTypeRecord) {
public void display(final ChangeDetail changeDetail,
final AccountInfoCache acc, SubmitTypeRecord submitTypeRecord) {
final Change chg = changeDetail.getChange();
final Branch.NameKey dst = chg.getDest();

CopyableLabel changeIdLabel =
Expand All @@ -114,7 +115,7 @@ public void display(final Change chg, final AccountInfoCache acc,

table.setWidget(R_BRANCH, 1, new BranchLink(dst.getShortName(), chg
.getProject(), chg.getStatus(), dst.get(), null));
table.setWidget(R_TOPIC, 1, topic(chg));
table.setWidget(R_TOPIC, 1, topic(changeDetail));
table.setText(R_UPLOADED, 1, mediumFormat(chg.getCreatedOn()));
table.setText(R_UPDATED, 1, mediumFormat(chg.getLastUpdatedOn()));
table.setText(R_STATUS, 1, Util.toLongString(chg.getStatus()));
Expand Down Expand Up @@ -146,17 +147,15 @@ public void display(final Change chg, final AccountInfoCache acc,
}
}

public Widget topic(final Change chg) {
public Widget topic(final ChangeDetail changeDetail) {
final Change chg = changeDetail.getChange();
final Branch.NameKey dst = chg.getDest();

FlowPanel fp = new FlowPanel();
fp.addStyleName(Gerrit.RESOURCES.css().changeInfoTopicPanel());
fp.add(new BranchLink(chg.getTopic(), chg.getProject(), chg.getStatus(),
dst.get(), chg.getTopic()));

ChangeDetailCache detailCache = ChangeCache.get(chg.getId()).getChangeDetailCache();
ChangeDetail changeDetail = detailCache.get();

if (changeDetail.canEditTopicName()) {
final Image edit = new Image(Gerrit.RESOURCES.edit());
edit.addStyleName(Gerrit.RESOURCES.css().link());
Expand Down
Expand Up @@ -322,7 +322,7 @@ private void display(final ChangeDetail detail) {

dependencies.setAccountInfoCache(detail.getAccounts());

descriptionBlock.display(detail.getChange(),
descriptionBlock.display(detail,
detail.isStarred(),
detail.canEditCommitMessage(),
detail.getCurrentPatchSetDetail().getInfo(),
Expand Down
Expand Up @@ -31,6 +31,7 @@
import com.google.gerrit.client.ui.PatchLink;
import com.google.gerrit.client.ui.SmallHeading;
import com.google.gerrit.common.PageLinks;
import com.google.gerrit.common.data.ChangeDetail;
import com.google.gerrit.common.data.PatchSetPublishDetail;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Patch;
Expand Down Expand Up @@ -317,9 +318,12 @@ private void initLabel(String labelName, Panel body) {
}

private void display(final PatchSetPublishDetail r) {
ChangeDetail changeDetail = new ChangeDetail();
changeDetail.setChange(r.getChange());

setPageTitle(Util.M.publishComments(r.getChange().getKey().abbreviate(),
patchSetId.get()));
descBlock.display(r.getChange(), null, false, r.getPatchSetInfo(), r.getAccounts(),
descBlock.display(changeDetail, null, false, r.getPatchSetInfo(), r.getAccounts(),
r.getSubmitTypeRecord(), commentLinkProcessor);

if (r.getChange().getStatus().isOpen()) {
Expand Down

0 comments on commit f646aa7

Please sign in to comment.