From 1a865ce7fd77603a67f1dab3a66917011ef23c57 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 14 Sep 2016 12:55:36 +0200 Subject: [PATCH] Refactor: Add a new class to represent an object that can be diffed In the change screen a user can select various objects as base for the diff view: a regular patch set, the base of a patch set, the parent of a merge, the auto-merge of a merge or an edit patch set. At the moment all of these are represented by a PatchSet.Id: * regular patch set: PatchSet.Id with id > 0 * base of a patch set: null * the parent of a merge: PatchSet.Id with id < 0 * auto-merge of a merge: null * edit patch set: PatchSet.Id with id == 0 This means if we only look at the patch set ID and it is null, we don't know if it's the base of a patch set or the auto-merge of a merge. To understand what null means, we must know if we are looking at a merge commit, but this knowledge is not available in all classes. This change refactors the UI classes to take a DiffObject as base instead of a PatchSet.Id so that in future we can differentiate between the base of a patch set and the auto-merge of a merge (see follow-up change). Change-Id: Ie3097ae4ff6435eef909f62eced8e6e82ff65de6 Signed-off-by: Edwin Kempin --- .../google/gerrit/client/info/ChangeInfo.java | 4 + .../com/google/gerrit/client/DiffObject.java | 164 ++++++++++++++++++ .../com/google/gerrit/client/Dispatcher.java | 59 +++---- .../gerrit/client/change/ChangeScreen.java | 49 +++--- .../gerrit/client/change/FileTable.java | 11 +- .../gerrit/client/diff/CommentManager.java | 18 +- .../client/diff/CommentsCollections.java | 19 +- .../google/gerrit/client/diff/DiffScreen.java | 22 +-- .../google/gerrit/client/diff/DiffTable.java | 12 +- .../com/google/gerrit/client/diff/Header.java | 34 ++-- .../gerrit/client/diff/PatchSetSelectBox.java | 48 ++--- .../google/gerrit/client/diff/SideBySide.java | 11 +- .../client/diff/SideBySideCommentManager.java | 3 +- .../gerrit/client/diff/SideBySideTable.java | 4 +- .../google/gerrit/client/diff/Unified.java | 6 +- .../client/diff/UnifiedCommentManager.java | 3 +- .../gerrit/client/diff/UnifiedTable.java | 4 +- 17 files changed, 330 insertions(+), 141 deletions(-) create mode 100644 gerrit-gwtui/src/main/java/com/google/gerrit/client/DiffObject.java diff --git a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java index 2644304faa..ff060b8051 100644 --- a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java +++ b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java @@ -415,6 +415,10 @@ public class ChangeInfo extends JavaScriptObject { return PatchSet.Id.toId(_number()); } + public final boolean isMerge() { + return commit().parents().length() > 1; + } + protected RevisionInfo () { } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/DiffObject.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/DiffObject.java new file mode 100644 index 0000000000..77003f9c6a --- /dev/null +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/DiffObject.java @@ -0,0 +1,164 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.client; + +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchSet; + +/** + * Represent an object that can be diffed. This can be either a regular patch + * set, the base of a patch set, the parent of a merge, the auto-merge of a + * merge or an edit patch set. + */ +public class DiffObject { + + /** + * Parses a string that represents a diff object. + *

+ * The following string representations are supported: + *

+ * + * @param changeId the ID of the change to which the diff object belongs + * @param str the string representation of the diff object + * @return the parsed diff object, {@code null} if str cannot be parsed as + * diff object + */ + public static DiffObject parse(Change.Id changeId, String str) { + if (str == null || str.isEmpty()) { + return new DiffObject(null); + } + + try { + return new DiffObject(new PatchSet.Id(changeId, Integer.parseInt(str))); + } catch (NumberFormatException e) { + return null; + } + } + + /** + * Create a DiffObject that represents the parent of a 1-parent patch set. + */ + public static DiffObject base() { + return new DiffObject(null); + } + + /** + * Create a DiffObject that represents the auto-merge for a merge patch set. + */ + public static DiffObject autoMerge() { + return new DiffObject(null); + } + + /** + * Create a DiffObject that represents a patch set. + */ + public static DiffObject patchSet(PatchSet.Id psId) { + return new DiffObject(psId); + } + + private final PatchSet.Id psId; + + private DiffObject(PatchSet.Id psId) { + this.psId = psId; + } + + public boolean isBaseOrAutoMerge() { + return psId == null; + } + + public boolean isPatchSet() { + return psId != null && psId.get() > 0; + } + + public boolean isParent() { + return psId != null && psId.get() < 0; + } + + public boolean isEdit() { + return psId != null && psId.get() == 0; + } + + /** + * Returns the DiffObject as PatchSet.Id. + * + * @return PatchSet.Id with an id > 0 for a regular patch set; PatchSet.Id + * with an id < 0 for a parent of a merge; PatchSet.Id with id == 0 + * for an edit patch set; {@code null} for the base of a 1-parent + * patch set and for the auto-merge of a merge patch set + */ + public PatchSet.Id asPatchSetId() { + return psId; + } + + /** + * Returns the parent number for a parent of a merge. + * + * @return 1-based parent number, 0 if this DiffObject is not a parent of a + * merge + */ + public int getParentNum() { + if (!isParent()) { + return 0; + } + + return -psId.get(); + } + + /** + * Returns a string representation of this DiffObject that can be used in + * URLs. + *

+ * The following string representations are returned: + *

+ * + * @return string representation of this DiffObject + */ + public String asString() { + if (psId != null) { + return psId.getId(); + } + + return null; + } + + @Override + public String toString() { + if (isPatchSet()) { + return "Patch Set " + psId.getId(); + } + + if (isParent()) { + return "Parent " + psId.getId(); + } + + if (isEdit()) { + return "Edit Patch Set"; + } + + return "Base or Auto-Merge"; + } +} 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 cb2ac07dbc..0674d9aab1 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 @@ -108,35 +108,35 @@ import com.google.gwtexpui.user.client.UserAgent; import com.google.gwtorm.client.KeyUtil; public class Dispatcher { - public static String toPatch(PatchSet.Id diffBase, + public static String toPatch(DiffObject diffBase, PatchSet.Id revision, String fileName) { return toPatch("", diffBase, revision, fileName, null, 0); } - public static String toPatch(PatchSet.Id diffBase, + public static String toPatch(DiffObject diffBase, PatchSet.Id revision, String fileName, DisplaySide side, int line) { return toPatch("", diffBase, revision, fileName, side, line); } - public static String toSideBySide(PatchSet.Id diffBase, Patch.Key id) { + public static String toSideBySide(DiffObject diffBase, Patch.Key id) { return toPatch("sidebyside", diffBase, id); } - public static String toSideBySide(PatchSet.Id diffBase, - PatchSet.Id revision, String fileName) { + public static String toSideBySide(DiffObject diffBase, PatchSet.Id revision, + String fileName) { return toPatch("sidebyside", diffBase, revision, fileName, null, 0); } - public static String toUnified(PatchSet.Id diffBase, + public static String toUnified(DiffObject diffBase, PatchSet.Id revision, String fileName) { return toPatch("unified", diffBase, revision, fileName, null, 0); } - public static String toUnified(PatchSet.Id diffBase, Patch.Key id) { + public static String toUnified(DiffObject diffBase, Patch.Key id) { return toPatch("unified", diffBase, id); } - public static String toPatch(String type, PatchSet.Id diffBase, Patch.Key id) { + public static String toPatch(String type, DiffObject diffBase, Patch.Key id) { return toPatch(type, diffBase, id.getParentKey(), id.get(), null, 0); } @@ -145,16 +145,16 @@ public class Dispatcher { } public static String toEditScreen(PatchSet.Id revision, String fileName, int line) { - return toPatch("edit", null, revision, fileName, null, line); + return toPatch("edit", DiffObject.base(), revision, fileName, null, line); } - private static String toPatch(String type, PatchSet.Id diffBase, + private static String toPatch(String type, DiffObject diffBase, PatchSet.Id revision, String fileName, DisplaySide side, int line) { Change.Id c = revision.getParentKey(); StringBuilder p = new StringBuilder(); p.append("/c/").append(c).append("/"); - if (diffBase != null) { - p.append(diffBase.get()).append(".."); + if (diffBase != null && !diffBase.isBaseOrAutoMerge()) { + p.append(diffBase.asString()).append(".."); } p.append(revision.getId()).append("/").append(KeyUtil.encode(fileName)); if (type != null && !type.isEmpty() @@ -395,7 +395,7 @@ public class Dispatcher { panel = null; } Gerrit.display(token, panel == null - ? new ChangeScreen(id, null, null, false, mode) + ? new ChangeScreen(id, DiffObject.base(), null, false, mode) : new NotFoundScreen()); return; } @@ -410,11 +410,14 @@ public class Dispatcher { rest = ""; } - PatchSet.Id base = null; + DiffObject base = DiffObject.base(); PatchSet.Id ps; int dotdot = psIdStr.indexOf(".."); if (1 <= dotdot) { - base = new PatchSet.Id(id, Integer.parseInt(psIdStr.substring(0, dotdot))); + base = DiffObject.parse(id, psIdStr.substring(0, dotdot)); + if (base == null) { + Gerrit.display(token, new NotFoundScreen()); + } psIdStr = psIdStr.substring(dotdot + 2); } ps = toPsId(id, psIdStr); @@ -438,9 +441,7 @@ public class Dispatcher { if (panel == null) { Gerrit.display(token, new ChangeScreen(id, - base != null - ? String.valueOf(base.get()) - : null, + base, String.valueOf(ps.get()), false, FileTable.Mode.REVIEW)); } else { Gerrit.display(token, new NotFoundScreen()); @@ -464,7 +465,7 @@ public class Dispatcher { } private static void patch(String token, - PatchSet.Id baseId, + DiffObject base, Patch.Key id, DisplaySide side, int line, @@ -477,14 +478,14 @@ public class Dispatcher { if ("".equals(panel) || /* DEPRECATED URL */"cm".equals(panel)) { if (preferUnified()) { - unified(token, baseId, id, side, line); + unified(token, base, id, side, line); } else { - codemirror(token, baseId, id, side, line); + codemirror(token, base, id, side, line); } } else if ("sidebyside".equals(panel)) { - codemirror(token, baseId, id, side, line); + codemirror(token, base, id, side, line); } else if ("unified".equals(panel)) { - unified(token, baseId, id, side, line); + unified(token, base, id, side, line); } else if ("edit".equals(panel)) { if (!Patch.isMagic(id.get()) || Patch.COMMIT_MSG.equals(id.get())) { codemirrorForEdit(token, id, line); @@ -501,24 +502,24 @@ public class Dispatcher { || (UserAgent.isPortrait() && UserAgent.isMobile()); } - private static void unified(final String token, final PatchSet.Id baseId, + private static void unified(final String token, final DiffObject base, final Patch.Key id, final DisplaySide side, final int line) { GWT.runAsync(new AsyncSplit(token) { @Override public void onSuccess() { - Gerrit.display(token, - new Unified(baseId, id.getParentKey(), id.get(), side, line)); + Gerrit.display(token, new Unified(base, + DiffObject.patchSet(id.getParentKey()), id.get(), side, line)); } }); } - private static void codemirror(final String token, final PatchSet.Id baseId, + private static void codemirror(final String token, final DiffObject base, final Patch.Key id, final DisplaySide side, final int line) { GWT.runAsync(new AsyncSplit(token) { @Override public void onSuccess() { - Gerrit.display(token, - new SideBySide(baseId, id.getParentKey(), id.get(), side, line)); + Gerrit.display(token, new SideBySide(base, + DiffObject.patchSet(id.getParentKey()), id.get(), side, line)); } }); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java index 60911e1476..127148081d 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java @@ -15,6 +15,7 @@ package com.google.gerrit.client.change; import com.google.gerrit.client.AvatarImage; +import com.google.gerrit.client.DiffObject; import com.google.gerrit.client.ErrorDialog; import com.google.gerrit.client.FormatUtil; import com.google.gerrit.client.Gerrit; @@ -146,7 +147,7 @@ public class ChangeScreen extends Screen { } private final Change.Id changeId; - private String base; + private DiffObject base; private String revision; private ChangeInfo changeInfo; private boolean hasDraftComments; @@ -239,10 +240,10 @@ public class ChangeScreen extends Screen { private DeleteFileAction deleteFileAction; private RenameFileAction renameFileAction; - public ChangeScreen(Change.Id changeId, String base, String revision, + public ChangeScreen(Change.Id changeId, DiffObject base, String revision, boolean openReplyBox, FileTable.Mode mode) { this.changeId = changeId; - this.base = normalize(base); + this.base = base; this.revision = normalize(revision); this.openReplyBox = openReplyBox; this.fileTableMode = mode; @@ -299,9 +300,10 @@ public class ChangeScreen extends Screen { group.addListener(new GerritCallback() { @Override public void onSuccess(Void result) { - if (base == null && rev.commit().parents().length() > 1) { - base = Gerrit.getUserPreferences() - .defaultBaseForMerges().getBase(); + if (base.isBaseOrAutoMerge() && rev.isMerge()) { + base = DiffObject.parse(info.legacyId(), + Gerrit.getUserPreferences() + .defaultBaseForMerges().getBase()); } loadConfigInfo(info, base); JsArray mAr = info.messages(); @@ -915,7 +917,7 @@ public class ChangeScreen extends Screen { int idx = diffBase.getSelectedIndex(); if (0 <= idx) { String n = diffBase.getValue(idx); - loadConfigInfo(changeInfo, !n.isEmpty() ? n : null); + loadConfigInfo(changeInfo, DiffObject.parse(changeInfo.legacyId(), n)); } } @@ -968,13 +970,14 @@ public class ChangeScreen extends Screen { int idx = diffBase.getSelectedIndex(); if (0 <= idx) { String n = diffBase.getValue(idx); - loadConfigInfo(changeInfo, !n.isEmpty() ? n : null); + loadConfigInfo(changeInfo, DiffObject.parse(changeInfo.legacyId(), n)); } } - private void loadConfigInfo(final ChangeInfo info, String base) { + private void loadConfigInfo(final ChangeInfo info, DiffObject base) { final RevisionInfo rev = info.revision(revision); - RevisionInfo b = resolveRevisionOrPatchSetId(info, base, null); + RevisionInfo baseRev = + resolveRevisionOrPatchSetId(info, base.asString(), null); CallbackGroup group = new CallbackGroup(); Timestamp lastReply = myLastReply(info); @@ -984,9 +987,9 @@ public class ChangeScreen extends Screen { RevisionInfo p = RevisionInfo.findEditParentRevision( info.revisions().values()); List>> comments = loadComments(p, group); - loadFileList(b, rev, lastReply, group, comments, null); + loadFileList(base, baseRev, rev, lastReply, group, comments, null); } else { - loadDiff(b, rev, lastReply, group); + loadDiff(base, baseRev, rev, lastReply, group); } group.addListener(new AsyncCallback() { @Override @@ -1038,11 +1041,11 @@ public class ChangeScreen extends Screen { return null; } - private void loadDiff(RevisionInfo base, RevisionInfo rev, + private void loadDiff(DiffObject base, RevisionInfo baseRev, RevisionInfo rev, Timestamp myLastReply, CallbackGroup group) { List>> comments = loadComments(rev, group); List>> drafts = loadDrafts(rev, group); - loadFileList(base, rev, myLastReply, group, comments, drafts); + loadFileList(base, baseRev, rev, myLastReply, group, comments, drafts); if (Gerrit.isSignedIn() && fileTableMode == FileTable.Mode.REVIEW) { ChangeApi.revision(changeId.get(), rev.name()) @@ -1061,19 +1064,19 @@ public class ChangeScreen extends Screen { } } - private void loadFileList(final RevisionInfo base, final RevisionInfo rev, - final Timestamp myLastReply, CallbackGroup group, + private void loadFileList(final DiffObject base, final RevisionInfo baseRev, + final RevisionInfo rev, final Timestamp myLastReply, CallbackGroup group, final List>> comments, final List>> drafts) { DiffApi.list(changeId.get(), rev.name(), - base, + baseRev, group.add( new AsyncCallback>() { @Override public void onSuccess(NativeMap m) { files.set( - base != null ? new PatchSet.Id(changeId, base._number()) : null, + base, new PatchSet.Id(changeId, rev._number()), style, reply, fileTableMode, edit != null); files.setValue(m, myLastReply, @@ -1475,12 +1478,12 @@ public class ChangeScreen extends Screen { RevisionInfo r = list.get(i); diffBase.addItem( r.id() + ": " + r.name().substring(0, 6), - r.name()); + r.id()); if (r.name().equals(revision)) { SelectElement.as(diffBase.getElement()).getOptions() .getItem(diffBase.getItemCount() - 1).setDisabled(true); } - if (base != null && base.equals(String.valueOf(r._number()))) { + if (base.isPatchSet() && base.asPatchSetId().get() == r._number()) { selectedIdx = diffBase.getItemCount() - 1; } } @@ -1494,9 +1497,9 @@ public class ChangeScreen extends Screen { diffBase.addItem(Util.M.diffBaseParent(parentNum), String.valueOf(-parentNum)); } - int parentNum = toParentNum(base); - if (parentNum > 0) { - selectedIdx = list.length() + parentNum; + + if (base.isParent()) { + selectedIdx = list.length() + base.getParentNum(); } } else { diffBase.addItem(Util.C.baseDiffItem(), ""); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java index 3a85b2655d..56691dc78b 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileTable.java @@ -19,6 +19,7 @@ import static com.google.gerrit.client.FormatUtil.formatAbsPercentage; import static com.google.gerrit.client.FormatUtil.formatBytes; import static com.google.gerrit.client.FormatUtil.formatPercentage; +import com.google.gerrit.client.DiffObject; import com.google.gerrit.client.Dispatcher; import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.VoidResult; @@ -182,7 +183,7 @@ public class FileTable extends FlowPanel { return null; } - private PatchSet.Id base; + private DiffObject base; private PatchSet.Id curr; private MyTable table; private boolean register; @@ -199,7 +200,7 @@ public class FileTable extends FlowPanel { R.css().ensureInjected(); } - public void set(PatchSet.Id base, PatchSet.Id curr, ChangeScreen.Style style, + public void set(DiffObject base, PatchSet.Id curr, ChangeScreen.Style style, Widget replyButton, Mode mode, boolean editExists) { this.base = base; this.curr = curr; @@ -340,7 +341,7 @@ public class FileTable extends FlowPanel { }); setSavePointerId( - (base != null ? base.toString() + ".." : "") + (!base.isBaseOrAutoMerge() ? base.toString() + ".." : "") + curr.toString()); } @@ -789,9 +790,9 @@ public class FileTable extends FlowPanel { for (CommentInfo c : Natives.asList(list)) { if (c.side() == Side.REVISION) { result.push(c); - } else if (base == null && !c.hasParent()) { + } else if (base.isBaseOrAutoMerge() && !c.hasParent()) { result.push(c); - } else if (base != null && c.parent() == -base.get()) { + } else if (base.isParent() && c.parent() == base.getParentNum()) { result.push(c); } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentManager.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentManager.java index 2f3ead3f17..4c01941ddc 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentManager.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentManager.java @@ -14,6 +14,7 @@ package com.google.gerrit.client.diff; +import com.google.gerrit.client.DiffObject; import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.changes.CommentInfo; import com.google.gerrit.client.patches.SkippedLine; @@ -40,7 +41,7 @@ import java.util.TreeMap; /** Tracks comment widgets for {@link DiffScreen}. */ abstract class CommentManager { - private final PatchSet.Id base; + private final DiffObject base; private final PatchSet.Id revision; private final String path; private final CommentLinkProcessor commentLinkProcessor; @@ -55,7 +56,7 @@ abstract class CommentManager { CommentManager( DiffScreen host, - PatchSet.Id base, + DiffObject base, PatchSet.Id revision, String path, CommentLinkProcessor clp, @@ -129,29 +130,30 @@ abstract class CommentManager { } Side getStoredSideFromDisplaySide(DisplaySide side) { - if (side == DisplaySide.A && (base == null || base.get() < 0)) { + if (side == DisplaySide.A && base.isBaseOrAutoMerge() || base.isParent()) { return Side.PARENT; } return Side.REVISION; } int getParentNumFromDisplaySide(DisplaySide side) { - if (side == DisplaySide.A && base != null && base.get() < 0) { - return -base.get(); + if (side == DisplaySide.A) { + return base.getParentNum(); } return 0; } PatchSet.Id getPatchSetIdFromSide(DisplaySide side) { - if (side == DisplaySide.A && base != null && base.get() >= 0) { - return base; + if (side == DisplaySide.A && (base.isPatchSet() || base.isEdit())) { + return base.asPatchSetId(); } return revision; } DisplaySide displaySide(CommentInfo info, DisplaySide forSide) { if (info.side() == Side.PARENT) { - return (base == null || base.get() < 0) ? DisplaySide.A : null; + return (base.isBaseOrAutoMerge() || base.isParent()) + ? DisplaySide.A : null; } return forSide; } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentsCollections.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentsCollections.java index ce1d294fa8..0b8e1416da 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentsCollections.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentsCollections.java @@ -14,6 +14,7 @@ package com.google.gerrit.client.diff; +import com.google.gerrit.client.DiffObject; import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.changes.CommentApi; import com.google.gerrit.client.changes.CommentInfo; @@ -31,7 +32,7 @@ import java.util.Comparator; /** Collection of published and draft comments loaded from the server. */ class CommentsCollections { private final String path; - private final PatchSet.Id base; + private final DiffObject base; private final PatchSet.Id revision; private NativeMap> publishedBaseAll; private NativeMap> publishedRevisionAll; @@ -40,28 +41,28 @@ class CommentsCollections { JsArray draftsBase; JsArray draftsRevision; - CommentsCollections(PatchSet.Id base, PatchSet.Id revision, String path) { + CommentsCollections(DiffObject base, PatchSet.Id revision, String path) { this.path = path; this.base = base; this.revision = revision; } void load(CallbackGroup group) { - if (base != null && base.get() > 0) { - CommentApi.comments(base, group.add(publishedBase())); + if (base.isPatchSet()) { + CommentApi.comments(base.asPatchSetId(), group.add(publishedBase())); } CommentApi.comments(revision, group.add(publishedRevision())); if (Gerrit.isSignedIn()) { - if (base != null && base.get() > 0) { - CommentApi.drafts(base, group.add(draftsBase())); + if (base.isPatchSet()) { + CommentApi.drafts(base.asPatchSetId(), group.add(draftsBase())); } CommentApi.drafts(revision, group.add(draftsRevision())); } } boolean hasCommentForPath(String filePath) { - if (base != null && base.get() > 0) { + if (base.isPatchSet()) { JsArray forBase = publishedBaseAll.get(filePath); if (forBase != null && forBase.length() > 0) { return true; @@ -110,9 +111,9 @@ class CommentsCollections { for (CommentInfo c : Natives.asList(list)) { if (c.side() == Side.REVISION) { result.push(c); - } else if (base == null && !c.hasParent()) { + } else if (base.isBaseOrAutoMerge() && !c.hasParent()) { result.push(c); - } else if (base != null && c.parent() == -base.get()) { + } else if (base.isParent() && c.parent() == base.getParentNum()) { result.push(c); } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffScreen.java index de19b35147..a22d4bda0d 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffScreen.java @@ -17,6 +17,7 @@ package com.google.gerrit.client.diff; import static com.google.gerrit.extensions.client.DiffPreferencesInfo.WHOLE_FILE_CONTEXT; import static java.lang.Double.POSITIVE_INFINITY; +import com.google.gerrit.client.DiffObject; import com.google.gerrit.client.Dispatcher; import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.account.DiffPreferences; @@ -96,7 +97,7 @@ abstract class DiffScreen extends Screen { } private final Change.Id changeId; - final PatchSet.Id base; + final DiffObject base; final PatchSet.Id revision; final String path; final DiffPreferences prefs; @@ -123,15 +124,15 @@ abstract class DiffScreen extends Screen { Header header; DiffScreen( - PatchSet.Id base, - PatchSet.Id revision, + DiffObject base, + DiffObject revision, String path, DisplaySide startSide, int startLine, DiffView diffScreenType) { this.base = base; - this.revision = revision; - this.changeId = revision.getParentKey(); + this.revision = revision.asPatchSetId(); + this.changeId = revision.asPatchSetId().getParentKey(); this.path = path; this.startSide = startSide; this.startLine = startLine; @@ -173,7 +174,7 @@ abstract class DiffScreen extends Screen { })); DiffApi.diff(revision, path) - .base(base) + .base(base.asPatchSetId()) .wholeFile() .intraline(prefs.intralineDifference()) .ignoreWhitespace(prefs.ignoreWhitespace()) @@ -789,11 +790,10 @@ abstract class DiffScreen extends Screen { group.addListener(new GerritCallback() { @Override public void onSuccess(Void result) { - String b = base != null ? String.valueOf(base.get()) : null; String rev = String.valueOf(revision.get()); Gerrit.display( - PageLinks.toChange(changeId, b, rev), - new ChangeScreen(changeId, b, rev, openReplyBox, + PageLinks.toChange(changeId, base.asString(), rev), + new ChangeScreen(changeId, base, rev, openReplyBox, FileTable.Mode.REVIEW)); } }); @@ -901,7 +901,7 @@ abstract class DiffScreen extends Screen { String nextPath = header.getNextPath(); if (nextPath != null) { DiffApi.diff(revision, nextPath) - .base(base) + .base(base.asPatchSetId()) .wholeFile() .intraline(prefs.intralineDifference()) .ignoreWhitespace(prefs.ignoreWhitespace()) @@ -924,7 +924,7 @@ abstract class DiffScreen extends Screen { void reloadDiffInfo() { final int id = ++reloadVersionId; DiffApi.diff(revision, path) - .base(base) + .base(base.asPatchSetId()) .wholeFile() .intraline(prefs.intralineDifference()) .ignoreWhitespace(prefs.ignoreWhitespace()) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java index 392ad2fb8e..54b55f04e5 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java @@ -14,6 +14,7 @@ package com.google.gerrit.client.diff; +import com.google.gerrit.client.DiffObject; import com.google.gerrit.client.account.DiffPreferences; import com.google.gerrit.client.info.ChangeInfo.RevisionInfo; import com.google.gerrit.reviewdb.client.Patch.ChangeType; @@ -66,11 +67,12 @@ abstract class DiffTable extends Composite { private ChangeType changeType; Scrollbar scrollbar; - DiffTable(DiffScreen parent, PatchSet.Id base, PatchSet.Id revision, String path) { - patchSetSelectBoxA = new PatchSetSelectBox( - parent, DisplaySide.A, revision.getParentKey(), base, path); - patchSetSelectBoxB = new PatchSetSelectBox( - parent, DisplaySide.B, revision.getParentKey(), revision, path); + DiffTable(DiffScreen parent, DiffObject base, DiffObject revision, + String path) { + patchSetSelectBoxA = new PatchSetSelectBox(parent, DisplaySide.A, + revision.asPatchSetId().getParentKey(), base, path); + patchSetSelectBoxB = new PatchSetSelectBox(parent, DisplaySide.B, + revision.asPatchSetId().getParentKey(), revision, path); PatchSetSelectBox.link(patchSetSelectBoxA, patchSetSelectBoxB); this.scrollbar = new Scrollbar(this); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Header.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Header.java index 3033cbb1f8..f3b9886bee 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Header.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Header.java @@ -14,6 +14,7 @@ package com.google.gerrit.client.diff; +import com.google.gerrit.client.DiffObject; import com.google.gerrit.client.Dispatcher; import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.account.DiffPreferences; @@ -87,7 +88,7 @@ public class Header extends Composite { @UiField Image preferences; private final KeyCommandSet keys; - private final PatchSet.Id base; + private final DiffObject base; private final PatchSet.Id patchSetId; private final String path; private final DiffView diffScreenType; @@ -99,12 +100,12 @@ public class Header extends Composite { private PreferencesAction prefsAction; private ReviewedState reviewedState; - Header(KeyCommandSet keys, PatchSet.Id base, PatchSet.Id patchSetId, + Header(KeyCommandSet keys, DiffObject base, DiffObject patchSetId, String path, DiffView diffSreenType, DiffPreferences prefs) { initWidget(uiBinder.createAndBindUi(this)); this.keys = keys; this.base = base; - this.patchSetId = patchSetId; + this.patchSetId = patchSetId.asPatchSetId(); this.path = path; this.diffScreenType = diffSreenType; this.prefs = prefs; @@ -113,9 +114,9 @@ public class Header extends Composite { reviewed.getElement().getStyle().setVisibility(Visibility.HIDDEN); } SafeHtml.setInnerHTML(filePath, formatPath(path)); - up.setTargetHistoryToken(PageLinks.toChange( - patchSetId.getParentKey(), - base != null ? base.getId() : null, patchSetId.getId())); + up.setTargetHistoryToken( + PageLinks.toChange(patchSetId.asPatchSetId().getParentKey(), + base.asString(), patchSetId.asPatchSetId().getId())); } public static SafeHtml formatPath(String path) { @@ -147,16 +148,17 @@ public class Header extends Composite { @Override protected void onLoad() { - DiffApi.list(patchSetId, base, new GerritCallback>() { - @Override - public void onSuccess(NativeMap result) { - files = result.values(); - FileInfo.sortFileInfoByPath(files); - fileNumber.setInnerText( - Integer.toString(Natives.asList(files).indexOf(result.get(path)) + 1)); - fileCount.setInnerText(Integer.toString(files.length())); - } - }); + DiffApi.list(patchSetId, base.asPatchSetId(), + new GerritCallback>() { + @Override + public void onSuccess(NativeMap result) { + files = result.values(); + FileInfo.sortFileInfoByPath(files); + fileNumber.setInnerText(Integer + .toString(Natives.asList(files).indexOf(result.get(path)) + 1)); + fileCount.setInnerText(Integer.toString(files.length())); + } + }); if (Gerrit.isSignedIn()) { ChangeApi.revision(patchSetId).view("files") diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PatchSetSelectBox.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PatchSetSelectBox.java index d45b8d8ab8..b07a199ea3 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PatchSetSelectBox.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PatchSetSelectBox.java @@ -14,6 +14,7 @@ package com.google.gerrit.client.diff; +import com.google.gerrit.client.DiffObject; import com.google.gerrit.client.Dispatcher; import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.blame.BlameInfo; @@ -67,13 +68,13 @@ class PatchSetSelectBox extends Composite { private String path; private Change.Id changeId; private PatchSet.Id revision; - private PatchSet.Id idActive; + private DiffObject idActive; private PatchSetSelectBox other; PatchSetSelectBox(DiffScreen parent, DisplaySide side, Change.Id changeId, - PatchSet.Id revision, + DiffObject diffObject, String path) { initWidget(uiBinder.createAndBindUi(this)); icon.setTitle(PatchUtil.C.addFileCommentToolTip()); @@ -83,8 +84,8 @@ class PatchSetSelectBox extends Composite { this.side = side; this.sideA = side == DisplaySide.A; this.changeId = changeId; - this.revision = revision; - this.idActive = (sideA && revision == null) ? null : revision; + this.revision = diffObject.asPatchSetId(); + this.idActive = diffObject; this.path = path; } @@ -93,19 +94,22 @@ class PatchSetSelectBox extends Composite { InlineHyperlink selectedLink = null; if (sideA) { if (parents <= 1) { - InlineHyperlink link = createLink(PatchUtil.C.patchBase(), null); + InlineHyperlink link = + createLink(PatchUtil.C.patchBase(), DiffObject.base()); linkPanel.add(link); selectedLink = link; } else { for (int i = parents; i > 0; i--) { PatchSet.Id id = new PatchSet.Id(changeId, -i); - InlineHyperlink link = createLink(Util.M.diffBaseParent(i), id); + InlineHyperlink link = + createLink(Util.M.diffBaseParent(i), DiffObject.patchSet(id)); linkPanel.add(link); if (revision != null && id.equals(revision)) { selectedLink = link; } } - InlineHyperlink link = createLink(Util.C.autoMerge(), null); + InlineHyperlink link = + createLink(Util.C.autoMerge(), DiffObject.autoMerge()); linkPanel.add(link); if (selectedLink == null) { selectedLink = link; @@ -115,7 +119,7 @@ class PatchSetSelectBox extends Composite { for (int i = 0; i < list.length(); i++) { RevisionInfo r = list.get(i); InlineHyperlink link = createLink(r.id(), - new PatchSet.Id(changeId, r._number())); + DiffObject.patchSet(new PatchSet.Id(changeId, r._number()))); linkPanel.add(link); if (revision != null && r.id().equals(revision.getId())) { selectedLink = link; @@ -131,8 +135,8 @@ class PatchSetSelectBox extends Composite { if (!Patch.isMagic(path)) { linkPanel.add(createDownloadLink()); } - if (!binary && open && idActive != null && Gerrit.isSignedIn()) { - if ((editExists && idActive.get() == 0) + if (!binary && open && !idActive.isBaseOrAutoMerge() && Gerrit.isSignedIn()) { + if ((editExists && idActive.isEdit()) || (!editExists && current)) { linkPanel.add(createEditIcon()); } @@ -172,7 +176,9 @@ class PatchSetSelectBox extends Composite { } private Widget createEditIcon() { - PatchSet.Id id = (idActive == null) ? other.idActive : idActive; + PatchSet.Id id = idActive.isBaseOrAutoMerge() + ? other.idActive.asPatchSetId() + : idActive.asPatchSetId(); Anchor anchor = new Anchor( new ImageResourceRenderer().render(Gerrit.RESOURCES.edit()), "#" + Dispatcher.toEditScreen(id, path)); @@ -192,27 +198,29 @@ class PatchSetSelectBox extends Composite { b.other = a; } - private InlineHyperlink createLink(String label, PatchSet.Id id) { + private InlineHyperlink createLink(String label, DiffObject id) { assert other != null; if (sideA) { - assert other.idActive != null; + assert !other.idActive.isBaseOrAutoMerge(); } - PatchSet.Id diffBase = sideA ? id : other.idActive; - PatchSet.Id revision = sideA ? other.idActive : id; + DiffObject diffBase = sideA ? id : other.idActive; + DiffObject revision = sideA ? other.idActive : id; return new InlineHyperlink(label, parent.isSideBySide() - ? Dispatcher.toSideBySide(diffBase, revision, path) - : Dispatcher.toUnified(diffBase, revision, path)); + ? Dispatcher.toSideBySide(diffBase, revision.asPatchSetId(), path) + : Dispatcher.toUnified(diffBase, revision.asPatchSetId(), path)); } private Anchor createDownloadLink() { - PatchSet.Id id = (idActive == null) ? other.idActive : idActive; - String sideURL = (idActive == null) ? "1" : "0"; + DiffObject diffObject = idActive.isBaseOrAutoMerge() + ? other.idActive : idActive; + String sideURL = idActive.isBaseOrAutoMerge() ? "1" : "0"; String base = GWT.getHostPageBaseURL() + "cat/"; Anchor anchor = new Anchor( new ImageResourceRenderer().render(Gerrit.RESOURCES.downloadIcon()), - base + KeyUtil.encode(id + "," + path) + "^" + sideURL); + base + KeyUtil.encode(diffObject.asPatchSetId() + "," + path) + "^" + + sideURL); anchor.setTitle(PatchUtil.C.download()); return anchor; } 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 dbe7e5d092..6e2120a4c3 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 @@ -16,6 +16,7 @@ package com.google.gerrit.client.diff; import static java.lang.Double.POSITIVE_INFINITY; +import com.google.gerrit.client.DiffObject; import com.google.gerrit.client.Dispatcher; import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.diff.LineMapper.LineOnOtherInfo; @@ -25,7 +26,6 @@ import com.google.gerrit.client.rpc.ScreenLoadCallback; import com.google.gerrit.client.ui.InlineHyperlink; import com.google.gerrit.extensions.client.GeneralPreferencesInfo.DiffView; import com.google.gerrit.reviewdb.client.Patch; -import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gwt.core.client.GWT; import com.google.gwt.core.client.Scheduler; import com.google.gwt.core.client.Scheduler.ScheduledCommand; @@ -69,8 +69,8 @@ public class SideBySide extends DiffScreen { private SideBySideCommentManager commentManager; public SideBySide( - PatchSet.Id base, - PatchSet.Id revision, + DiffObject base, + DiffObject revision, String path, DisplaySide startSide, int startLine) { @@ -192,9 +192,8 @@ public class SideBySide extends DiffScreen { cmA = newCm(diff.metaA(), diff.textA(), diffTable.cmA); cmB = newCm(diff.metaB(), diff.textB(), diffTable.cmB); - boolean reviewingBase = base == null; - getDiffTable().setUpBlameIconA(cmA, reviewingBase, - reviewingBase ? revision : base, path); + getDiffTable().setUpBlameIconA(cmA, base.isBaseOrAutoMerge(), + base.isBaseOrAutoMerge() ? revision : base.asPatchSetId(), path); getDiffTable().setUpBlameIconB(cmB, revision, path); cmA.extras().side(DisplaySide.A); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentManager.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentManager.java index bcb7daceec..fab6e6bea4 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentManager.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentManager.java @@ -14,6 +14,7 @@ package com.google.gerrit.client.diff; +import com.google.gerrit.client.DiffObject; import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.changes.CommentInfo; import com.google.gerrit.client.ui.CommentLinkProcessor; @@ -29,7 +30,7 @@ import java.util.SortedMap; /** Tracks comment widgets for {@link SideBySide}. */ class SideBySideCommentManager extends CommentManager { SideBySideCommentManager(SideBySide host, - PatchSet.Id base, PatchSet.Id revision, + DiffObject base, PatchSet.Id revision, String path, CommentLinkProcessor clp, boolean open) { diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideTable.java index 2296796080..5e8d7cc3fb 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideTable.java @@ -14,8 +14,8 @@ package com.google.gerrit.client.diff; +import com.google.gerrit.client.DiffObject; import com.google.gerrit.reviewdb.client.Patch.ChangeType; -import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gwt.core.client.GWT; import com.google.gwt.dom.client.Element; import com.google.gwt.resources.client.CssResource; @@ -46,7 +46,7 @@ class SideBySideTable extends DiffTable { private boolean visibleA; - SideBySideTable(SideBySide parent, PatchSet.Id base, PatchSet.Id revision, + SideBySideTable(SideBySide parent, DiffObject base, DiffObject revision, String path) { super(parent, base, revision, path); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Unified.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Unified.java index a231580065..566d87cb0e 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Unified.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Unified.java @@ -16,6 +16,7 @@ package com.google.gerrit.client.diff; import static java.lang.Double.POSITIVE_INFINITY; +import com.google.gerrit.client.DiffObject; import com.google.gerrit.client.Dispatcher; import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.diff.UnifiedChunkManager.LineRegionInfo; @@ -25,7 +26,6 @@ import com.google.gerrit.client.rpc.ScreenLoadCallback; import com.google.gerrit.client.ui.InlineHyperlink; import com.google.gerrit.extensions.client.GeneralPreferencesInfo.DiffView; import com.google.gerrit.reviewdb.client.Patch; -import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gwt.core.client.GWT; import com.google.gwt.core.client.JavaScriptObject; import com.google.gwt.core.client.JsArrayString; @@ -69,8 +69,8 @@ public class Unified extends DiffScreen { private boolean autoHideDiffTableHeader; public Unified( - PatchSet.Id base, - PatchSet.Id revision, + DiffObject base, + DiffObject revision, String path, DisplaySide startSide, int startLine) { diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedCommentManager.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedCommentManager.java index 8968bc79bb..21356fce6e 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedCommentManager.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedCommentManager.java @@ -14,6 +14,7 @@ package com.google.gerrit.client.diff; +import com.google.gerrit.client.DiffObject; import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.changes.CommentInfo; import com.google.gerrit.client.diff.LineMapper.LineOnOtherInfo; @@ -43,7 +44,7 @@ class UnifiedCommentManager extends CommentManager { private final Map duplicates; UnifiedCommentManager(Unified host, - PatchSet.Id base, PatchSet.Id revision, + DiffObject base, PatchSet.Id revision, String path, CommentLinkProcessor clp, boolean open) { diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedTable.java index 72b3e4981d..e3317c472d 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/UnifiedTable.java @@ -14,7 +14,7 @@ package com.google.gerrit.client.diff; -import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.client.DiffObject; import com.google.gwt.core.client.GWT; import com.google.gwt.dom.client.Element; import com.google.gwt.resources.client.CssResource; @@ -45,7 +45,7 @@ class UnifiedTable extends DiffTable { @UiField Element cm; @UiField static DiffTableStyle style; - UnifiedTable(Unified parent, PatchSet.Id base, PatchSet.Id revision, + UnifiedTable(Unified parent, DiffObject base, DiffObject revision, String path) { super(parent, base, revision, path);