From 051e7313baba931a9cb71ecc57b9d013edd73e5b Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 13 Feb 2013 11:58:18 -0800 Subject: [PATCH] Port ApprovalTable to new REST API The table now renders from a new-style ChangeInfo JSON object rather than a ChangeDetail object. The structure of the new JSON object is more suited to rendering the approval table, so the code should be simplified somewhat. Change-Id: I708068b6a5e359ebf5065bb52b23b78557634969 --- .../gerrit/common/data/ApprovalDetail.java | 23 + .../gerrit/client/changes/AccountInfo.java | 31 ++ .../gerrit/client/changes/ApprovalTable.java | 446 ++++++++---------- .../gerrit/client/changes/ChangeApi.java | 4 + .../gerrit/client/changes/ChangeInfo.java | 13 +- .../gerrit/client/changes/ChangeScreen.java | 20 +- .../client/changes/PublishCommentScreen.java | 22 +- 7 files changed, 286 insertions(+), 273 deletions(-) create mode 100644 gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountInfo.java 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 05e86be6f6..57e773da60 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 @@ -21,8 +21,10 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval; import java.sql.Timestamp; import java.util.ArrayList; import java.util.Comparator; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; public class ApprovalDetail { @@ -46,6 +48,7 @@ public class ApprovalDetail { private transient Set approved; private transient Set rejected; + private transient Map values; private transient int hasNonZero; private transient Timestamp sortOrder = EG_D; @@ -69,10 +72,12 @@ public class ApprovalDetail { canRemove = removeable; } + @Deprecated public List getPatchSetApprovals() { return approvals; } + @Deprecated public PatchSetApproval getPatchSetApproval(ApprovalCategory.Id category) { for (PatchSetApproval psa : approvals) { if (psa.getCategoryId().equals(category)) { @@ -87,12 +92,15 @@ public class ApprovalDetail { sortOrder = ApprovalDetail.EG_0; } + @Deprecated public void add(final PatchSetApproval ca) { approvals.add(ca); final Timestamp g = ca.getGranted(); if (g != null && g.compareTo(sortOrder) < 0) { sortOrder = g; + // Value is not set, but code calling this deprecated method does not + // call getValue. } if (ca.getValue() != 0) { hasNonZero = 1; @@ -120,6 +128,13 @@ public class ApprovalDetail { votable.add(label); } + public void value(String label, int value) { + if (values == null) { + values = new HashMap(); + } + values.put(label, value); + } + public boolean isApproved(String label) { return approved != null && approved.contains(label); } @@ -131,4 +146,12 @@ public class ApprovalDetail { public boolean canVote(String label) { return votable != null && votable.contains(label); } + + public int getValue(String label) { + if (values == null) { + return 0; + } + Integer v = values.get(label); + return v != null ? v : 0; + } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountInfo.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountInfo.java new file mode 100644 index 0000000000..b9ee62df79 --- /dev/null +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountInfo.java @@ -0,0 +1,31 @@ +// 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.client.changes; + +import com.google.gwt.core.client.JavaScriptObject; + +public class AccountInfo extends JavaScriptObject { + public final native int _account_id() /*-{ return this._account_id || 0; }-*/; + public final native String name() /*-{ return this.name; }-*/; + public final native String email() /*-{ return this.email; }-*/; + + public static native AccountInfo create(int id, String name, + String email) /*-{ + return {'_account_id': id, 'name': name, 'email': email}; + }-*/; + + protected AccountInfo() { + } +} 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 0d69c9afcf..1eb6b88a60 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 @@ -14,28 +14,29 @@ package com.google.gerrit.client.changes; +import static com.google.gerrit.reviewdb.client.ApprovalCategoryValue.formatValue; + import com.google.gerrit.client.ConfirmationCallback; import com.google.gerrit.client.ConfirmationDialog; import com.google.gerrit.client.ErrorDialog; -import com.google.gerrit.client.FormatUtil; import com.google.gerrit.client.Gerrit; -import com.google.gerrit.client.patches.PatchUtil; +import com.google.gerrit.client.changes.ChangeInfo.ApprovalInfo; +import com.google.gerrit.client.changes.ChangeInfo.LabelInfo; import com.google.gerrit.client.rpc.GerritCallback; -import com.google.gerrit.client.ui.AccountLink; +import com.google.gerrit.client.rpc.NativeMap; +import com.google.gerrit.client.rpc.NativeString; +import com.google.gerrit.client.rpc.Natives; import com.google.gerrit.client.ui.AddMemberBox; +import com.google.gerrit.client.ui.InlineHyperlink; import com.google.gerrit.client.ui.ReviewerSuggestOracle; -import com.google.gerrit.common.data.AccountInfoCache; +import com.google.gerrit.common.PageLinks; import com.google.gerrit.common.data.ApprovalDetail; import com.google.gerrit.common.data.ApprovalType; import com.google.gerrit.common.data.ApprovalTypes; -import com.google.gerrit.common.data.ChangeDetail; -import com.google.gerrit.common.data.PatchSetPublishDetail; -import com.google.gerrit.common.data.ReviewerResult; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gwt.core.client.JavaScriptObject; +import com.google.gwt.core.client.JsArray; import com.google.gwt.event.dom.client.ClickEvent; import com.google.gwt.event.dom.client.ClickHandler; import com.google.gwt.user.client.DOM; @@ -51,24 +52,34 @@ import com.google.gwt.user.client.ui.Widget; import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder; import java.util.ArrayList; -import java.util.Collections; -import java.util.Comparator; +import java.util.Collection; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; +import java.util.Set; /** Displays a table of {@link ApprovalDetail} objects for a change record. */ public class ApprovalTable extends Composite { + static short parseLabelValue(String value) { + if (value.charAt(0) == ' ' || value.charAt(0) == '+') { + value = value.substring(1); + } + return Short.parseShort(value); + } + private final ApprovalTypes types; private final Grid table; private final Widget missing; private final Panel addReviewer; private final ReviewerSuggestOracle reviewerSuggestOracle; private final AddMemberBox addMemberBox; - private Change.Id changeId; - private AccountInfoCache accountCache = AccountInfoCache.empty(); + private ChangeInfo lastChange; + private Map rows; public ApprovalTable() { + rows = new HashMap(); types = Gerrit.getConfig().getApprovalTypes(); table = new Grid(1, 3); table.addStyleName(Gerrit.RESOURCES.css().infoTable()); @@ -104,7 +115,7 @@ public class ApprovalTable extends Composite { setStyleName(Gerrit.RESOURCES.css().approvalTable()); } - private void displayHeader(List labels) { + private void displayHeader(Collection labels) { table.resizeColumns(2 + labels.size()); final CellFormatter fmt = table.getCellFormatter(); @@ -126,263 +137,212 @@ public class ApprovalTable extends Composite { fmt.addStyleName(0, col - 1, Gerrit.RESOURCES.css().rightmost()); } - public void setAccountInfoCache(final AccountInfoCache aic) { - assert aic != null; - accountCache = aic; - } + void display(ChangeInfo change) { + lastChange = change; + reviewerSuggestOracle.setChange(change.legacy_id()); + Map byUser = + new LinkedHashMap(); + Map accounts = + new LinkedHashMap(); + List missingLabels = initLabels(change, accounts, byUser); - private AccountLink link(final Account.Id id) { - return AccountLink.link(accountCache, id); - } - - void display(PatchSetPublishDetail detail) { - doDisplay(detail.getChange(), detail.getApprovals(), - detail.getSubmitRecords()); - } - - void display(ChangeDetail detail) { - doDisplay(detail.getChange(), detail.getApprovals(), - detail.getSubmitRecords()); - } - - private void doDisplay(Change change, List approvals, - List submitRecords) { - changeId = change.getId(); - reviewerSuggestOracle.setChange(changeId); - - List columns = new ArrayList(); - - final Element missingList = missing.getElement(); - while (DOM.getChildCount(missingList) > 0) { - DOM.removeChild(missingList, DOM.getChild(missingList, 0)); - } - missing.setVisible(false); - - if (submitRecords != null) { - HashSet reportedMissing = new HashSet(); - - HashMap byUser = - new HashMap(); - for (ApprovalDetail ad : approvals) { - byUser.put(ad.getAccount(), ad); - } - - for (SubmitRecord rec : submitRecords) { - if (rec.labels == null) { - continue; - } - - for (SubmitRecord.Label lbl : rec.labels) { - if (!columns.contains(lbl.label)) { - columns.add(lbl.label); - } - - switch (lbl.status) { - case OK: { - ApprovalDetail ad = byUser.get(lbl.appliedBy); - if (ad != null) { - ad.approved(lbl.label); - } - break; - } - - case REJECT: { - ApprovalDetail ad = byUser.get(lbl.appliedBy); - if (ad != null) { - ad.rejected(lbl.label); - } - break; - } - - case MAY: - break; - - case NEED: - case IMPOSSIBLE: - if (reportedMissing.add(lbl.label)) { - Element li = DOM.createElement("li"); - li.setClassName(Gerrit.RESOURCES.css().missingApproval()); - DOM.setInnerText(li, Util.M.needApproval(lbl.label)); - DOM.appendChild(missingList, li); - } - break; - } - } - } - missing.setVisible(!reportedMissing.isEmpty()); - - } else { - for (ApprovalDetail ad : approvals) { - for (PatchSetApproval psa : ad.getPatchSetApprovals()) { - ApprovalType legacyType = types.byId(psa.getCategoryId()); - if (legacyType == null) { - continue; - } - String labelName = legacyType.getCategory().getLabelName(); - if (psa.getValue() != 0 ) { - if (psa.getValue() == legacyType.getMax().getValue()) { - ad.approved(labelName); - } else if (psa.getValue() == legacyType.getMin().getValue()) { - ad.rejected(labelName); - } - } - if (!columns.contains(labelName)) { - columns.add(labelName); - } - } - Collections.sort(columns, new Comparator() { - @Override - public int compare(String o1, String o2) { - ApprovalType a = types.byLabel(o1); - ApprovalType b = types.byLabel(o2); - int cmp = 0; - if (a != null && b != null) { - cmp = a.getCategory().getPosition() - b.getCategory().getPosition(); - } - if (cmp == 0) { - cmp = o1.compareTo(o2); - } - return cmp; - } - }); - } + removeAllChildren(missing.getElement()); + for (String label : missingLabels) { + addMissingLabel(Util.M.needApproval(label)); } - if (approvals.isEmpty()) { + if (byUser.isEmpty()) { table.setVisible(false); } else { - displayHeader(columns); - table.resizeRows(1 + approvals.size()); - for (int i = 0; i < approvals.size(); i++) { - displayRow(i + 1, approvals.get(i), change, columns); + displayHeader(change.labels()); + table.resizeRows(1 + byUser.size()); + int i = 1; + for (ApprovalDetail ad : byUser.values()) { + displayRow(i++, ad, change, accounts.get(ad.getAccount().get())); } table.setVisible(true); } - addReviewer.setVisible(Gerrit.isSignedIn()); - if (Gerrit.getConfig().testChangeMerge() - && !change.isMergeable()) { - Element li = DOM.createElement("li"); - li.setClassName(Gerrit.RESOURCES.css().missingApproval()); - DOM.setInnerText(li, Util.C.messageNeedsRebaseOrHasDependency()); - DOM.appendChild(missingList, li); - missing.setVisible(true); + && !change.mergeable()) { + addMissingLabel(Util.C.messageNeedsRebaseOrHasDependency()); } + missing.setVisible(DOM.getChildCount(missing.getElement()) > 0); + addReviewer.setVisible(Gerrit.isSignedIn()); + } + + private void removeAllChildren(Element el) { + for (int i = DOM.getChildCount(el) - 1; i >= 0; i--) { + DOM.removeChild(el, DOM.getChild(el, i)); + } + } + + private void addMissingLabel(String text) { + Element li = DOM.createElement("li"); + li.setClassName(Gerrit.RESOURCES.css().missingApproval()); + DOM.setInnerText(li, text); + DOM.appendChild(missing.getElement(), li); + } + + private Set removableReviewers(ChangeInfo change) { + Set result = + new HashSet(change.removable_reviewers().length()); + for (int i = 0; i < change.removable_reviewers().length(); i++) { + result.add(change.removable_reviewers().get(i)._account_id()); + } + return result; + } + + private List initLabels(ChangeInfo change, + Map accounts, + Map byUser) { + Set removableReviewers = removableReviewers(change); + List missing = new ArrayList(); + for (String name : change.labels()) { + LabelInfo label = change.label(name); + + String min = null; + String max = null; + for (String v : label.values()) { + if (min == null) { + min = v; + } + if (v.startsWith("+")) { + max = v; + } + } + + if (label.status() == SubmitRecord.Label.Status.NEED) { + missing.add(name); + } + + if (label.all() != null) { + for (ApprovalInfo ai : Natives.asList(label.all())) { + if (!accounts.containsKey(ai._account_id())) { + accounts.put(ai._account_id(), ai); + } + int id = ai._account_id(); + ApprovalDetail ad = byUser.get(id); + if (ad == null) { + ad = new ApprovalDetail(new Account.Id(id)); + ad.setCanRemove(removableReviewers.contains(id)); + byUser.put(id, ad); + } + ad.votable(name); + ad.value(name, ai.value()); + if (formatValue(ai.value()).equals(max)) { + ad.approved(name); + } else if (formatValue(ai.value()).equals(min)) { + ad.rejected(name); + } + } + } + } + return missing; } private void doAddReviewer() { - final String reviewer = addMemberBox.getText(); - if (reviewer.length() == 0) { - return; + String reviewer = addMemberBox.getText(); + if (!reviewer.isEmpty()) { + addMemberBox.setEnabled(false); + addReviewer(reviewer, false); } - - addMemberBox.setEnabled(false); - final List reviewers = new ArrayList(); - reviewers.add(reviewer); - - addReviewers(reviewers, false); } - private void addReviewers(final List reviewers, - final boolean confirmed) { - PatchUtil.DETAIL_SVC.addReviewers(changeId, reviewers, confirmed, - new GerritCallback() { - public void onSuccess(final ReviewerResult result) { + private static class PostInput extends JavaScriptObject { + static PostInput create(String reviewer, boolean confirmed) { + PostInput input = createObject().cast(); + input.init(reviewer, confirmed); + return input; + } + + private native void init(String reviewer, boolean confirmed) /*-{ + this.reviewer = reviewer; + if (confirmed) { + this.confirmed = true; + } + }-*/; + + protected PostInput() { + } + } + + private static class ReviewerInfo extends AccountInfo { + final Set approvals() { + return Natives.keys(_approvals()); + } + final native String approval(String l) /*-{ return this.approvals[l]; }-*/; + private final native NativeMap _approvals() /*-{ return this.approvals; }-*/; + + protected ReviewerInfo() { + } + } + + private static class PostResult extends JavaScriptObject { + final native JsArray reviewers() /*-{ return this.reviewers; }-*/; + final native boolean confirm() /*-{ return this.confirm || false; }-*/; + final native String error() /*-{ return this.error; }-*/; + + protected PostResult() { + } + } + + private void addReviewer(final String reviewer, boolean confirmed) { + ChangeApi.reviewers(lastChange.legacy_id().get()).post( + PostInput.create(reviewer, confirmed), + new GerritCallback() { + public void onSuccess(PostResult result) { addMemberBox.setEnabled(true); addMemberBox.setText(""); - - final ChangeDetail changeDetail = result.getChange(); - if (changeDetail != null) { - setAccountInfoCache(changeDetail.getAccounts()); - display(changeDetail); - } - - if (!result.getErrors().isEmpty()) { - final SafeHtmlBuilder r = new SafeHtmlBuilder(); - for (final ReviewerResult.Error e : result.getErrors()) { - switch (e.getType()) { - case REVIEWER_NOT_FOUND: - r.append(Util.M.reviewerNotFound(e.getName())); - break; - - case ACCOUNT_INACTIVE: - r.append(Util.M.accountInactive(e.getName())); - break; - - case CHANGE_NOT_VISIBLE: - r.append(Util.M.changeNotVisibleTo(e.getName())); - break; - - case GROUP_EMPTY: - r.append(Util.M.groupIsEmpty(e.getName())); - break; - - case GROUP_HAS_TOO_MANY_MEMBERS: - if (result.askForConfirmation() && !confirmed) { - askForConfirmation(e.getName(), result.getMemberCount()); - return; - } else { - r.append(Util.M.groupHasTooManyMembers(e.getName())); - } - break; - - case GROUP_NOT_ALLOWED: - r.append(Util.M.groupIsNotAllowed(e.getName())); - break; - - default: - r.append(e.getName()); - r.append(" - "); - r.append(e.getType()); - r.br(); - break; - } - } - new ErrorDialog(r).center(); + if (result.error() == null) { + reload(); + } else if (result.confirm()) { + askForConfirmation(result.error()); + } else { + new ErrorDialog(new SafeHtmlBuilder().append(result.error())); } } - private void askForConfirmation(final String groupName, - final int memberCount) { - final SafeHtmlBuilder b = new SafeHtmlBuilder(); - b.openElement("b"); - b.append(Util.M - .groupManyMembersConfirmation(groupName, memberCount)); - b.closeElement("b"); - final ConfirmationDialog confirmationDialog = - new ConfirmationDialog(Util.C - .approvalTableAddManyReviewersConfirmationDialogTitle(), - b.toSafeHtml(), new ConfirmationCallback() { - @Override - public void onOk() { - addReviewers(reviewers, true); - } - }); + private void askForConfirmation(String text) { + String title = Util.C + .approvalTableAddManyReviewersConfirmationDialogTitle(); + ConfirmationDialog confirmationDialog = new ConfirmationDialog( + title, new SafeHtmlBuilder().append(text), + new ConfirmationCallback() { + @Override + public void onOk() { + addReviewer(reviewer, true); + } + }); confirmationDialog.center(); } @Override public void onFailure(final Throwable caught) { addMemberBox.setEnabled(true); - super.onFailure(caught); + if (isNoSuchEntity(caught)) { + new ErrorDialog(Util.M.reviewerNotFound(reviewer)).center(); + } else { + super.onFailure(caught); + } } }); } - private void displayRow(final int row, final ApprovalDetail ad, - final Change change, List columns) { + private void displayRow(int row, final ApprovalDetail ad, ChangeInfo change, + AccountInfo account) { final CellFormatter fmt = table.getCellFormatter(); int col = 0; - table.setWidget(row, col++, link(ad.getAccount())); + table.setWidget(row, col++, new InlineHyperlink(account.name(), + PageLinks.toAccountQuery(account.name()))); + rows.put(account._account_id(), row); if (ad.canRemove()) { final PushButton remove = new PushButton( // new Image(Util.R.removeReviewerNormal()), // new Image(Util.R.removeReviewerPressed())); - remove.setTitle(Util.M.removeReviewer( // - FormatUtil.name(accountCache.get(ad.getAccount())))); + remove.setTitle(Util.M.removeReviewer(account.name())); remove.setStyleName(Gerrit.RESOURCES.css().removeReviewer()); remove.addStyleName(Gerrit.RESOURCES.css().link()); remove.addClickHandler(new ClickHandler() { @@ -397,7 +357,7 @@ public class ApprovalTable extends Composite { } fmt.setStyleName(row, col++, Gerrit.RESOURCES.css().removeReviewerCell()); - for (String labelName : columns) { + for (String labelName : change.labels()) { fmt.setStyleName(row, col, Gerrit.RESOURCES.css().approvalscore()); if (!ad.canVote(labelName)) { fmt.addStyleName(row, col, Gerrit.RESOURCES.css().notVotable()); @@ -411,6 +371,7 @@ public class ApprovalTable extends Composite { table.setWidget(row, col, new Image(Gerrit.RESOURCES.greenCheck())); } else { + // TODO: support arbitrary labels. ApprovalType legacyType = types.byLabel(labelName); if (legacyType == null) { table.clearCell(row, col); @@ -418,15 +379,14 @@ public class ApprovalTable extends Composite { continue; } - PatchSetApproval ca = ad.getPatchSetApproval(legacyType.getCategory().getId()); - if (ca == null || ca.getValue() == 0) { + int v = ad.getValue(labelName); + if (v == 0) { table.clearCell(row, col); col++; continue; } - - String vstr = String.valueOf(ca.getValue()); - if (ca.getValue() > 0) { + String vstr = String.valueOf(ad.getValue(labelName)); + if (v > 0) { vstr = "+" + vstr; fmt.addStyleName(row, col, Gerrit.RESOURCES.css().posscore()); } else { @@ -442,19 +402,19 @@ public class ApprovalTable extends Composite { } private void reload() { - Util.DETAIL_SVC.changeDetail(changeId, - new GerritCallback() { + ChangeApi.detail(lastChange.legacy_id().get(), + new GerritCallback() { @Override - public void onSuccess(ChangeDetail result) { + public void onSuccess(ChangeInfo result) { display(result); } - }); + }); } private void doRemove(ApprovalDetail ad, final PushButton remove) { remove.setEnabled(false); - ChangeApi.reviewer(changeId.get(), ad.getAccount().get()).delete( - new GerritCallback() { + ChangeApi.reviewer(lastChange.legacy_id().get(), ad.getAccount().get()) + .delete(new GerritCallback() { @Override public void onSuccess(JavaScriptObject result) { reload(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java index a1424e2e34..abd94c9b88 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java @@ -69,6 +69,10 @@ public class ChangeApi { return change(id.getParentKey().get()).view("revisions").id(id.get()); } + public static RestApi reviewers(int id) { + return change(id).view("reviewers"); + } + public static RestApi reviewer(int id, int reviewer) { return change(id).view("reviewers").id(reviewer); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java index 21ecb6c15c..91b261ab1b 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java @@ -106,13 +106,6 @@ public class ChangeInfo extends JavaScriptObject { protected ChangeInfo() { } - public static class AccountInfo extends JavaScriptObject { - public final native String name() /*-{ return this.name; }-*/; - - protected AccountInfo() { - } - } - public static class LabelInfo extends JavaScriptObject { public final SubmitRecord.Label.Status status() { if (approved() != null) { @@ -136,6 +129,7 @@ public class ChangeInfo extends JavaScriptObject { public final native JsArray all() /*-{ return this.all; }-*/; private final native NativeMap _values() /*-{ return this.values; }-*/; + public final Set values() { return Natives.keys(_values()); } @@ -154,10 +148,7 @@ public class ChangeInfo extends JavaScriptObject { } } - public static class ApprovalInfo extends JavaScriptObject { - public final native int _account_id() /*-{ return this._account_id || 0; }-*/; - public final native String name() /*-{ return this.name; }-*/; - public final native String email() /*-{ return this.email; }-*/; + public static class ApprovalInfo extends AccountInfo { public final native short value() /*-{ return this.value; }-*/; protected ApprovalInfo() { diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java index 509f34c477..79cf2fe0ab 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java @@ -16,6 +16,7 @@ package com.google.gerrit.client.changes; import com.google.gerrit.client.Dispatcher; import com.google.gerrit.client.Gerrit; +import com.google.gerrit.client.rpc.GerritCallback; import com.google.gerrit.client.ui.CommentPanel; import com.google.gerrit.client.ui.ComplexDisclosurePanel; import com.google.gerrit.client.ui.ExpandAllCommand; @@ -62,6 +63,7 @@ public class ChangeScreen extends Screen private final Change.Id changeId; private final PatchSet.Id openPatchSetId; private ChangeDetailCache detailCache; + private com.google.gerrit.client.changes.ChangeInfo changeInfo; private ChangeDescriptionBlock descriptionBlock; private ApprovalTable approvals; @@ -255,9 +257,20 @@ public class ChangeScreen extends Screen } @Override - public void onValueChange(ValueChangeEvent event) { + public void onValueChange(final ValueChangeEvent event) { if (isAttached()) { - display(event.getValue()); + // Until this screen is fully migrated to the new API, this call must be + // sequential, because we can't start an async get at the source of every + // call that might trigger a value change. + ChangeApi.detail(event.getValue().getChange().getId().get(), + new GerritCallback() { + @Override + public void onSuccess( + com.google.gerrit.client.changes.ChangeInfo result) { + changeInfo = result; + display(event.getValue()); + } + }); } } @@ -273,7 +286,6 @@ public class ChangeScreen extends Screen } dependencies.setAccountInfoCache(detail.getAccounts()); - approvals.setAccountInfoCache(detail.getAccounts()); descriptionBlock.display(detail.getChange(), detail.isStarred(), @@ -282,7 +294,7 @@ public class ChangeScreen extends Screen detail.getAccounts(), detail.getSubmitTypeRecord()); dependsOn.display(detail.getDependsOn()); neededBy.display(detail.getNeededBy()); - approvals.display(detail); + approvals.display(changeInfo); patchesList.clear(); if (detail.getCurrentPatchSetDetail().getInfo().getParents().size() > 1) { diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java index a4a3fdaffa..3e19c7af92 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java @@ -14,6 +14,8 @@ package com.google.gerrit.client.changes; +import static com.google.gerrit.client.changes.ApprovalTable.parseLabelValue; + import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.changes.ChangeInfo.ApprovalInfo; import com.google.gerrit.client.changes.ChangeInfo.LabelInfo; @@ -242,14 +244,13 @@ public class PublishCommentScreen extends AccountScreen implements mwrap.add(message); } - private void initApprovals(final PatchSetPublishDetail r, final Panel body) { + private void initApprovals(Panel body) { for (String labelName : change.labels()) { - initLabel(r, labelName, body); + initLabel(labelName, body); } } - private void initLabel(PatchSetPublishDetail r, String labelName, - Panel body) { + private void initLabel(String labelName, Panel body) { JsArrayString nativeValues = change.permitted_values(labelName); if (nativeValues == null || nativeValues.length() == 0) { return; @@ -302,10 +303,8 @@ public class PublishCommentScreen extends AccountScreen implements r.getSubmitTypeRecord()); if (r.getChange().getStatus().isOpen()) { - initApprovals(r, approvalPanel); - - approvals.setAccountInfoCache(r.getAccounts()); - approvals.display(r); + initApprovals(approvalPanel); + approvals.display(change); } else { approvals.setVisible(false); } @@ -451,13 +450,6 @@ public class PublishCommentScreen extends AccountScreen implements Gerrit.display(PageLinks.toChange(ck), new ChangeScreen(ck)); } - private static short parseLabelValue(String value) { - if (value.charAt(0) == ' ' || value.charAt(0) == '+') { - value = value.substring(1); - } - return Short.parseShort(value); - } - private static class ValueRadioButton extends RadioButton { final LabelInfo label; final String value;