From 6b0ee9897378ba45988504a752068d73bf0a8b43 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Sat, 24 Jan 2015 23:06:19 +0100 Subject: [PATCH] InlineEdit: Fix rendering edits in unified diff screen Bug: Issue 3102 Change-Id: Iab524d03c263fd6a5ba691fb8a41b033b4b169cb --- .../google/gerrit/client/diff/SideBySide.java | 15 +++---- .../client/patches/PatchSetSelectBox.java | 3 +- .../gerrit/client/patches/ReviewedPanels.java | 44 ++++++++++--------- .../client/patches/UnifiedDiffTable.java | 2 +- .../changedetail/PatchSetDetailFactory.java | 43 ++++++++++++++---- .../server/patch/PatchScriptFactory.java | 11 ++++- 6 files changed, 74 insertions(+), 44 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide.java index fca7b1e409..576982c518 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide.java @@ -617,16 +617,11 @@ public class SideBySide extends Screen { } private List getLinks() { - // skip change edits - if (revision.get() > 0) { - InlineHyperlink toUnifiedDiffLink = new InlineHyperlink(); - toUnifiedDiffLink.setHTML(new ImageResourceRenderer().render(Gerrit.RESOURCES.unifiedDiff())); - toUnifiedDiffLink.setTargetHistoryToken(getUnifiedDiffUrl()); - toUnifiedDiffLink.setTitle(PatchUtil.C.unifiedDiff()); - return Collections.singletonList(toUnifiedDiffLink); - } else { - return Collections.emptyList(); - } + InlineHyperlink toUnifiedDiffLink = new InlineHyperlink(); + toUnifiedDiffLink.setHTML(new ImageResourceRenderer().render(Gerrit.RESOURCES.unifiedDiff())); + toUnifiedDiffLink.setTargetHistoryToken(getUnifiedDiffUrl()); + toUnifiedDiffLink.setTitle(PatchUtil.C.unifiedDiff()); + return Collections.singletonList(toUnifiedDiffLink); } private String getUnifiedDiffUrl() { diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchSetSelectBox.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchSetSelectBox.java index 7b3a20e3af..7c80397697 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchSetSelectBox.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchSetSelectBox.java @@ -115,7 +115,8 @@ public class PatchSetSelectBox extends Composite { for (Patch patch : script.getHistory()) { PatchSet.Id psId = patch.getKey().getParentKey(); - Anchor anchor = createLink(Integer.toString(psId.get()), psId); + String label = psId.get() == 0 ? "edit" : Integer.toString(psId.get()); + Anchor anchor = createLink(label, psId); links.put(psId.get(), anchor); linkPanel.add(anchor); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/ReviewedPanels.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/ReviewedPanels.java index ea36c18d9a..d889c7976a 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/ReviewedPanels.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/ReviewedPanels.java @@ -94,31 +94,33 @@ class ReviewedPanels { } void setReviewedByCurrentUser(boolean reviewed) { - if (fileList != null) { - fileList.updateReviewedStatus(patchKey, reviewed); - } - PatchSet.Id ps = patchKey.getParentKey(); - RestApi api = new RestApi("/changes/").id(ps.getParentKey().get()) - .view("revisions").id(ps.get()) - .view("files").id(patchKey.getFileName()) - .view("reviewed"); - - AsyncCallback cb = new AsyncCallback() { - @Override - public void onFailure(Throwable arg0) { - // nop + if (ps.get() != 0) { + if (fileList != null) { + fileList.updateReviewedStatus(patchKey, reviewed); } - @Override - public void onSuccess(VoidResult result) { - // nop + RestApi api = new RestApi("/changes/").id(ps.getParentKey().get()) + .view("revisions").id(ps.get()) + .view("files").id(patchKey.getFileName()) + .view("reviewed"); + + AsyncCallback cb = new AsyncCallback() { + @Override + public void onFailure(Throwable arg0) { + // nop + } + + @Override + public void onSuccess(VoidResult result) { + // nop + } + }; + if (reviewed) { + api.put(cb); + } else { + api.delete(cb); } - }; - if (reviewed) { - api.put(cb); - } else { - api.delete(cb); } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/UnifiedDiffTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/UnifiedDiffTable.java index ebcdf05ede..10c029f45f 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/UnifiedDiffTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/UnifiedDiffTable.java @@ -355,7 +355,7 @@ public class UnifiedDiffTable extends AbstractPatchContentTable { @Override public void display(final CommentDetail cd, boolean expandComments) { - if (cd.isEmpty()) { + if (cd == null || cd.isEmpty()) { return; } setAccountInfoCache(cd.getAccounts()); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java index 7b68f75f3b..9fc95b1ba4 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java @@ -15,18 +15,21 @@ package com.google.gerrit.httpd.rpc.changedetail; import com.google.common.base.Function; +import com.google.common.base.Optional; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.PatchSetDetail; import com.google.gerrit.common.data.UiCommandDetail; import com.google.gerrit.common.errors.NoSuchEntityException; +import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.webui.UiAction; import com.google.gerrit.httpd.rpc.Handler; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountDiffPreference; import com.google.gerrit.reviewdb.client.AccountDiffPreference.Whitespace; import com.google.gerrit.reviewdb.client.AccountPatchReview; +import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.PatchLineComment; import com.google.gerrit.reviewdb.client.PatchSet; @@ -38,6 +41,8 @@ import com.google.gerrit.server.PatchLineCommentsUtil; import com.google.gerrit.server.change.ChangesCollection; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.change.Revisions; +import com.google.gerrit.server.edit.ChangeEdit; +import com.google.gerrit.server.edit.ChangeEditUtil; import com.google.gerrit.server.extensions.webui.UiActions; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.patch.PatchList; @@ -58,6 +63,7 @@ import org.eclipse.jgit.lib.ObjectId; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.io.IOException; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -83,6 +89,7 @@ class PatchSetDetailFactory extends Handler { private final ChangesCollection changes; private final Revisions revisions; private final PatchLineCommentsUtil plcUtil; + private final ChangeEditUtil editUtil; private Project.NameKey projectKey; private final PatchSet.Id psIdBase; @@ -103,6 +110,7 @@ class PatchSetDetailFactory extends Handler { final ChangesCollection changes, final Revisions revisions, final PatchLineCommentsUtil plcUtil, + ChangeEditUtil editUtil, @Assisted("psIdBase") @Nullable final PatchSet.Id psIdBase, @Assisted("psIdNew") final PatchSet.Id psIdNew, @Assisted @Nullable final AccountDiffPreference diffPrefs) { @@ -114,6 +122,7 @@ class PatchSetDetailFactory extends Handler { this.changes = changes; this.revisions = revisions; this.plcUtil = plcUtil; + this.editUtil = editUtil; this.psIdBase = psIdBase; this.psIdNew = psIdNew; @@ -122,11 +131,21 @@ class PatchSetDetailFactory extends Handler { @Override public PatchSetDetail call() throws OrmException, NoSuchEntityException, - PatchSetInfoNotAvailableException, NoSuchChangeException { + PatchSetInfoNotAvailableException, NoSuchChangeException, AuthException, + IOException { + Optional edit = null; if (control == null || patchSet == null) { control = changeControlFactory.validateFor(psIdNew.getParentKey(), userProvider.get()); - patchSet = db.patchSets().get(psIdNew); + if (psIdNew.get() == 0) { + Change change = db.changes().get(psIdNew.getParentKey()); + edit = editUtil.byChange(change); + if (edit.isPresent()) { + patchSet = edit.get().getBasePatchSet(); + } + } else { + patchSet = db.patchSets().get(psIdNew); + } if (patchSet == null) { throw new NoSuchEntityException(); } @@ -137,7 +156,11 @@ class PatchSetDetailFactory extends Handler { try { if (psIdBase != null) { oldId = toObjectId(psIdBase); - newId = toObjectId(psIdNew); + if (edit != null && edit.isPresent()) { + newId = edit.get().getEditCommit().toObjectId(); + } else { + newId = toObjectId(psIdNew); + } list = listFor(keyFor(diffPrefs.getIgnoreWhitespace())); } else { // OK, means use base to compare @@ -154,10 +177,12 @@ class PatchSetDetailFactory extends Handler { } ChangeNotes notes = control.getNotes(); - for (PatchLineComment c : plcUtil.publishedByPatchSet(db, notes, psIdNew)) { - final Patch p = byKey.get(c.getKey().getParentKey()); - if (p != null) { - p.setCommentCount(p.getCommentCount() + 1); + if (edit == null) { + for (PatchLineComment c : plcUtil.publishedByPatchSet(db, notes, psIdNew)) { + final Patch p = byKey.get(c.getKey().getParentKey()); + if (p != null) { + p.setCommentCount(p.getCommentCount() + 1); + } } } @@ -165,11 +190,11 @@ class PatchSetDetailFactory extends Handler { detail.setPatchSet(patchSet); detail.setProject(projectKey); - detail.setInfo(infoFactory.get(db, psIdNew)); + detail.setInfo(infoFactory.get(db, patchSet.getId())); detail.setPatches(patches); final CurrentUser user = control.getCurrentUser(); - if (user.isIdentifiedUser()) { + if (user.isIdentifiedUser() && edit == null) { // If we are signed in, compute the number of draft comments by the // current user on each of these patch files. This way they can more // quickly locate where they have pending drafts, and review them. diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java index 4cb984fb3e..3bdb6b2d83 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java @@ -86,6 +86,7 @@ public class PatchScriptFactory implements Callable { private final PatchSet.Id psb; private final AccountDiffPreference diffPrefs; private final ChangeEditUtil editReader; + private Optional edit; private final Change.Id changeId; private boolean loadHistory = true; @@ -231,7 +232,7 @@ public class PatchScriptFactory implements Callable { private ObjectId getEditRev() throws AuthException, NoSuchChangeException, IOException { - Optional edit = editReader.byChange(change); + edit = editReader.byChange(change); if (edit.isPresent()) { return edit.get().getRef().getObjectId(); } @@ -284,9 +285,15 @@ public class PatchScriptFactory implements Callable { history.add(p); byKey.put(p.getKey(), p); } + if (edit != null && edit.isPresent()) { + final Patch p = new Patch(new Patch.Key( + new PatchSet.Id(psb.getParentKey(), 0), fileName)); + history.add(p); + byKey.put(p.getKey(), p); + } } - if (loadComments) { + if (loadComments && edit == null) { final AccountInfoCacheFactory aic = aicFactory.create(); comments = new CommentDetail(psa, psb); switch (changeType) {