diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/ApprovalDetail.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/ApprovalDetail.java index 86d70d4cad..d931742507 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/ApprovalDetail.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/ApprovalDetail.java @@ -18,6 +18,7 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.PatchSetApproval; import java.util.ArrayList; +import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -25,6 +26,13 @@ import java.util.Map; import java.util.Set; public class ApprovalDetail { + public static final Comparator SORT = + new Comparator() { + public int compare(ApprovalDetail o1, ApprovalDetail o2) { + return o1.hasNonZero - o2.hasNonZero; + } + }; + protected Account.Id account; protected List approvals; protected boolean canRemove; @@ -33,6 +41,7 @@ public class ApprovalDetail { private transient Set approved; private transient Set rejected; private transient Map values; + private transient int hasNonZero; protected ApprovalDetail() { } @@ -59,6 +68,7 @@ public class ApprovalDetail { approved = new HashSet(); } approved.add(label); + hasNonZero = 1; } public void rejected(String label) { @@ -66,6 +76,7 @@ public class ApprovalDetail { rejected = new HashSet(); } rejected.add(label); + hasNonZero = 1; } public void votable(String label) { @@ -80,6 +91,9 @@ public class ApprovalDetail { values = new HashMap(); } values.put(label, value); + if (value != 0) { + hasNonZero = 1; + } } public boolean isApproved(String label) { diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java index 1eb6b88a60..2c9b48bb7c 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java @@ -53,6 +53,7 @@ import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; @@ -156,8 +157,11 @@ public class ApprovalTable extends Composite { } else { displayHeader(change.labels()); table.resizeRows(1 + byUser.size()); + List sorted = + new ArrayList(byUser.values()); + Collections.sort(sorted, ApprovalDetail.SORT); int i = 1; - for (ApprovalDetail ad : byUser.values()) { + for (ApprovalDetail ad : sorted) { displayRow(i++, ad, change, accounts.get(ad.getAccount().get())); } table.setVisible(true); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index b57b060a0e..4e9e330f6c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -322,7 +322,8 @@ public class ChangeJson { return Collections.emptyMap(); } - Map labels = Maps.newLinkedHashMap(); + Map labels = + Maps.newTreeMap(LabelOrdering.create(approvalTypes)); initLabels(cd, labels, standard); Collection approvals = null; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/LabelOrdering.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/LabelOrdering.java new file mode 100644 index 0000000000..d7304ae6f7 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/LabelOrdering.java @@ -0,0 +1,36 @@ +// Copyright (C) 2013 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.server.change; + +import com.google.common.base.Function; +import com.google.common.collect.Ordering; +import com.google.gerrit.common.data.ApprovalType; +import com.google.gerrit.common.data.ApprovalTypes; + +class LabelOrdering { + public static Ordering create(final ApprovalTypes approvalTypes) { + return Ordering.natural().nullsLast().onResultOf( + new Function() { + @Override + public Short apply(String n) { + ApprovalType at = approvalTypes.byLabel(n); + return at != null ? at.getCategory().getPosition() : null; + } + }).compound(Ordering.natural()); + } + + private LabelOrdering() { + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java index 72ec5c8805..d174ef563c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java @@ -79,8 +79,8 @@ public class ReviewerJson { PatchSet.Id psId = ctl.getChange().currentPatchSetId(); if (approvals == null) { - approvals = db.get().patchSetApprovals() - .byPatchSetUser(psId, out._id).toList(); + approvals = ChangeData.sortApprovals(db.get().patchSetApprovals() + .byPatchSetUser(psId, out._id)); } FunctionState fs = functionState.create(ctl, psId, approvals); @@ -88,7 +88,7 @@ public class ReviewerJson { CategoryFunction.forCategory(at.getCategory()).run(at, fs); } - out.approvals = Maps.newHashMapWithExpectedSize(approvals.size()); + out.approvals = Maps.newTreeMap(LabelOrdering.create(approvalTypes)); for (PatchSetApproval ca : approvals) { for (PermissionRange pr : ctl.getLabelRanges()) { if (!pr.isEmpty()) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index 07222fa921..ba5faad390 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -14,10 +14,12 @@ package com.google.gerrit.server.query.change; +import com.google.common.base.Function; import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.ListMultimap; import com.google.common.collect.Lists; import com.google.common.collect.Maps; -import com.google.common.collect.Multimap; +import com.google.common.collect.Ordering; import com.google.common.collect.Sets; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.reviewdb.client.Change; @@ -46,6 +48,7 @@ import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; import java.io.IOException; +import java.sql.Timestamp; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -55,6 +58,19 @@ import java.util.Map; import java.util.Set; public class ChangeData { + private static Ordering SORT_APPROVALS = Ordering.natural() + .onResultOf(new Function() { + @Override + public Timestamp apply(PatchSetApproval a) { + return a.getGranted(); + } + }); + + public static List sortApprovals( + Iterable approvals) { + return SORT_APPROVALS.sortedCopy(approvals); + } + public static void ensureChangeLoaded( Provider db, List changes) throws OrmException { Map missing = Maps.newHashMap(); @@ -102,7 +118,7 @@ public class ChangeData { int idx = 0; for (ChangeData cd : changes) { if (cd.currentApprovals == null && cd.approvals == null) { - cd.currentApprovals = pending.get(idx++).toList(); + cd.currentApprovals = sortApprovals(pending.get(idx++)); } } } @@ -114,8 +130,8 @@ public class ChangeData { private PatchSet currentPatchSet; private Set limitedIds; private Collection patches; - private Multimap approvals; - private Collection currentApprovals; + private ListMultimap approvals; + private List currentApprovals; private String[] currentFiles; private Collection comments; private Collection trackingIds; @@ -254,7 +270,7 @@ public class ChangeData { return currentPatchSet; } - public Collection currentApprovals(Provider db) + public List currentApprovals(Provider db) throws OrmException { if (currentApprovals == null) { Change c = change(db); @@ -264,8 +280,8 @@ public class ChangeData { (limitedIds == null || limitedIds.contains(c.currentPatchSetId()))) { return approvals.get(c.currentPatchSetId()); } else { - currentApprovals = db.get().patchSetApprovals() - .byPatchSet(c.currentPatchSetId()).toList(); + currentApprovals = sortApprovals(db.get().patchSetApprovals() + .byPatchSet(c.currentPatchSetId())); } } return currentApprovals; @@ -318,29 +334,30 @@ public class ChangeData { /** * @param db review database. - * @return patch set approvals for the change. If + * @return patch set approvals for the change in timestamp order. If * {@link #limitToPatchSets(Collection)} was previously called, only contains * approvals for the patches with the specified IDs. * @throws OrmException an error occurred reading the database. */ - public Collection approvals(Provider db) + public List approvals(Provider db) throws OrmException { - return approvalsMap(db).values(); + return sortApprovals(approvalsMap(db).values()); } /** * @param db review database. - * @return patch set approvals for the change, keyed by ID. If + * @return patch set approvals for the change, keyed by ID, ordered by + * timestamp within each patch set. If * {@link #limitToPatchSets(Collection)} was previously called, only * contains approvals for the patches with the specified IDs. * @throws OrmException an error occurred reading the database. */ - public Multimap approvalsMap( + public ListMultimap approvalsMap( Provider db) throws OrmException { if (approvals == null) { approvals = ArrayListMultimap.create(); - for (PatchSetApproval psa : - db.get().patchSetApprovals().byChange(legacyId)) { + for (PatchSetApproval psa : sortApprovals( + db.get().patchSetApprovals().byChange(legacyId))) { if (limitedIds == null || limitedIds.contains(legacyId)) { approvals.put(psa.getPatchSetId(), psa); }