Rewrite UI plugin action description API

Using methods on an interface that plugins/extensions implement
makes for a very brittle API. We cannot safely add a new method
without breaking all existing code or introducing another interface
for plugins to implement.

Change the UI action description to be a single method interface
that returns an instance of a concrete description object. Plugins
can populate the fields they know about and need for their use case,
and the server will do its best to honor what the plugin has given.
New methods can be added to the description without affecting the
API versioning, enabling Gerrit to add more features in the future.

The confirmation message feature has been removed. In the near
future plugins will have the ability to register JavaScript code
to be executed client-side when the button is activated and before
the RPC is started. This will allow plugins to perform more custom
logic, such as obtaining additional input.

Looking even further ahead plugins may register their own JS handler
to render a button, allowing the plugin to use a more complex widget.
The UiAction.Description type may need to be extended to allow
server side view code to pass custom data to the UI rendering logic.

Change-Id: If5e0e6b42d6d1e8a111a2d304ce54b9d8834688a
This commit is contained in:
Shawn Pearce
2013-07-15 12:20:40 -07:00
parent 3600951d0b
commit 3d57549a1d
17 changed files with 252 additions and 364 deletions

View File

@@ -21,7 +21,7 @@ import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.DefaultInput;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.webui.UiCommand;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -42,11 +42,9 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.Collections;
import java.util.EnumSet;
import java.util.Set;
public class Abandon implements RestModifyView<ChangeResource, Input>,
UiCommand<ChangeResource> {
UiAction<ChangeResource> {
private static final Logger log = LoggerFactory.getLogger(Abandon.class);
private final ChangeHooks hooks;
@@ -133,34 +131,11 @@ public class Abandon implements RestModifyView<ChangeResource, Input>,
}
@Override
public Set<Place> getPlaces() {
return EnumSet.of(Place.PATCHSET_ACTION_PANEL);
}
@Override
public String getLabel(ChangeResource resource) {
return "Abandon";
}
@Override
public String getTitle(ChangeResource resource) {
return null;
}
@Override
public boolean isVisible(ChangeResource resource) {
return isEnabled(resource);
}
@Override
public boolean isEnabled(ChangeResource resource) {
return resource.getChange().getStatus().isOpen()
&& resource.getControl().canAbandon();
}
@Override
public String getConfirmationMessage(ChangeResource resource) {
return null;
public UiAction.Description getDescription(ChangeResource resource) {
return new UiAction.Description()
.setLabel("Abandon")
.setVisible(resource.getChange().getStatus().isOpen()
&& resource.getControl().canAbandon());
}
private ChangeMessage newMessage(Input input, IdentifiedUser caller,

View File

@@ -46,11 +46,10 @@ import com.google.gerrit.common.data.LabelValue;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.common.data.UiCommandDetail;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.extensions.webui.UiCommand;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Change.Status;
@@ -68,7 +67,7 @@ import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountInfo;
import com.google.gerrit.server.config.CanonicalWebUrl;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.extensions.webui.UiCommands;
import com.google.gerrit.server.extensions.webui.UiActions;
import com.google.gerrit.server.git.LabelNormalizer;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
@@ -293,11 +292,10 @@ public class ChangeJson {
if (has(CURRENT_ACTIONS) && user instanceof IdentifiedUser) {
out.actions = Maps.newTreeMap();
for (UiCommandDetail c : UiCommands.from(
for (UiAction.Description d : UiActions.from(
changes,
new ChangeResource(control(cd)),
EnumSet.of(UiCommand.Place.PATCHSET_ACTION_PANEL))) {
out.actions.put(c.id, new ActionInfo(c));
new ChangeResource(control(cd)))) {
out.actions.put(d.getId(), new ActionInfo(d));
}
}
lastControl = null;
@@ -794,11 +792,10 @@ public class ChangeJson {
if (out.isCurrent && has(CURRENT_ACTIONS) && user instanceof IdentifiedUser) {
out.actions = Maps.newTreeMap();
for (UiCommandDetail c : UiCommands.from(
for (UiAction.Description d : UiActions.from(
revisions,
new RevisionResource(new ChangeResource(control(cd)), in),
EnumSet.of(UiCommand.Place.PATCHSET_ACTION_PANEL))) {
out.actions.put(c.id, new ActionInfo(c));
new RevisionResource(new ChangeResource(control(cd)), in))) {
out.actions.put(d.getId(), new ActionInfo(d));
}
}
return out;
@@ -983,14 +980,12 @@ public class ChangeJson {
String label;
String title;
Boolean enabled;
String confirmationMessage;
ActionInfo(UiCommandDetail c) {
method = c.method;
label = c.label;
title = c.title;
enabled = c.enabled ? true : null;
confirmationMessage = c.confirmationMessage;
ActionInfo(UiAction.Description d) {
method = d.getMethod();
label = d.getLabel();
title = d.getTitle();
enabled = d.isEnabled() ? true : null;
}
}
}

View File

@@ -18,7 +18,7 @@ import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.webui.UiCommand;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -30,11 +30,8 @@ import com.google.gerrit.server.project.RefControl;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.util.EnumSet;
import java.util.Set;
class CherryPick implements RestModifyView<RevisionResource, Input>,
UiCommand<RevisionResource> {
UiAction<RevisionResource> {
private final Provider<ReviewDb> dbProvider;
private final Provider<CherryPickChange> cherryPickChange;
private final ChangeJson json;
@@ -96,32 +93,10 @@ class CherryPick implements RestModifyView<RevisionResource, Input>,
}
@Override
public Set<Place> getPlaces() {
return EnumSet.of(Place.PATCHSET_ACTION_PANEL);
}
@Override
public String getLabel(RevisionResource resource) {
return "Cherry Pick To";
}
@Override
public String getTitle(RevisionResource resource) {
return "Cherry pick change to a different branch";
}
@Override
public boolean isVisible(RevisionResource resource) {
return isEnabled(resource);
}
@Override
public boolean isEnabled(RevisionResource resource) {
return resource.getControl().getProjectControl().canUpload();
}
@Override
public String getConfirmationMessage(RevisionResource resource) {
return null;
public UiAction.Description getDescription(RevisionResource resource) {
return new UiAction.Description()
.setLabel("Cherry Pick")
.setTitle("Cherry pick change to a different branch")
.setVisible(resource.getControl().getProjectControl().canUpload());
}
}

View File

@@ -22,7 +22,7 @@ import com.google.gerrit.extensions.restapi.DefaultInput;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.webui.UiCommand;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -36,11 +36,9 @@ import com.google.inject.Inject;
import com.google.inject.Provider;
import java.util.Collections;
import java.util.EnumSet;
import java.util.Set;
class PutTopic implements RestModifyView<ChangeResource, Input>,
UiCommand<ChangeResource> {
UiAction<ChangeResource> {
private final Provider<ReviewDb> dbProvider;
private final ChangeIndexer indexer;
private final ChangeHooks hooks;
@@ -119,32 +117,9 @@ class PutTopic implements RestModifyView<ChangeResource, Input>,
}
@Override
public Set<Place> getPlaces() {
return EnumSet.of(Place.PATCHSET_ACTION_PANEL);
}
@Override
public String getLabel(ChangeResource resource) {
return "Edit Topic";
}
@Override
public String getTitle(ChangeResource resource) {
return null;
}
@Override
public boolean isVisible(ChangeResource resource) {
return isEnabled(resource);
}
@Override
public boolean isEnabled(ChangeResource resource) {
return resource.getControl().canEditTopicName();
}
@Override
public String getConfirmationMessage(ChangeResource resource) {
return null;
public UiAction.Description getDescription(ChangeResource resource) {
return new UiAction.Description()
.setLabel("Edit Topic")
.setVisible(resource.getControl().canEditTopicName());
}
}

View File

@@ -20,7 +20,7 @@ import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.webui.UiCommand;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -35,11 +35,9 @@ import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.IOException;
import java.util.EnumSet;
import java.util.Set;
public class Rebase implements RestModifyView<RevisionResource, Input>,
UiCommand<RevisionResource> {
UiAction<RevisionResource> {
public static class Input {
}
@@ -81,35 +79,13 @@ public class Rebase implements RestModifyView<RevisionResource, Input>,
}
@Override
public Set<Place> getPlaces() {
return EnumSet.of(Place.PATCHSET_ACTION_PANEL);
}
@Override
public String getLabel(RevisionResource resource) {
return "Rebase";
}
@Override
public String getTitle(RevisionResource resource) {
return "Rebase onto tip of branch or parent change";
}
@Override
public boolean isVisible(RevisionResource resource) {
return isEnabled(resource);
}
@Override
public boolean isEnabled(RevisionResource resource) {
return resource.getChange().getStatus().isOpen()
&& resource.getControl().canRebase()
&& rebaseChange.get().canRebase(resource);
}
@Override
public String getConfirmationMessage(RevisionResource resource) {
return null;
public UiAction.Description getDescription(RevisionResource resource) {
return new UiAction.Description()
.setLabel("Rebase")
.setTitle("Rebase onto tip of branch or parent change")
.setVisible(resource.getChange().getStatus().isOpen()
&& resource.getControl().canRebase()
&& rebaseChange.get().canRebase(resource));
}
public static class CurrentRevision implements

View File

@@ -20,7 +20,7 @@ import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.DefaultInput;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.webui.UiCommand;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Change.Status;
import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -42,11 +42,9 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.Collections;
import java.util.EnumSet;
import java.util.Set;
public class Restore implements RestModifyView<ChangeResource, Input>,
UiCommand<ChangeResource> {
UiAction<ChangeResource> {
private static final Logger log = LoggerFactory.getLogger(Restore.class);
private final ChangeHooks hooks;
@@ -94,8 +92,8 @@ public class Restore implements RestModifyView<ChangeResource, Input>,
new AtomicUpdate<Change>() {
@Override
public Change update(Change change) {
if (change.getStatus() == Change.Status.ABANDONED) {
change.setStatus(Change.Status.NEW);
if (change.getStatus() == Status.ABANDONED) {
change.setStatus(Status.NEW);
ChangeUtil.updated(change);
return change;
}
@@ -132,34 +130,11 @@ public class Restore implements RestModifyView<ChangeResource, Input>,
}
@Override
public Set<Place> getPlaces() {
return EnumSet.of(Place.PATCHSET_ACTION_PANEL);
}
@Override
public String getLabel(ChangeResource resource) {
return "Restore";
}
@Override
public String getTitle(ChangeResource resource) {
return null;
}
@Override
public boolean isVisible(ChangeResource resource) {
return isEnabled(resource);
}
@Override
public boolean isEnabled(ChangeResource resource) {
return resource.getChange().getStatus() == Change.Status.ABANDONED
&& resource.getControl().canRestore();
}
@Override
public String getConfirmationMessage(ChangeResource resource) {
return null;
public UiAction.Description getDescription(ChangeResource resource) {
return new UiAction.Description()
.setLabel("Restore")
.setVisible(resource.getChange().getStatus() == Status.ABANDONED
&& resource.getControl().canRestore());
}
private ChangeMessage newMessage(Input input, IdentifiedUser caller,

View File

@@ -20,7 +20,7 @@ import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.webui.UiCommand;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Change.Status;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -41,11 +41,8 @@ import com.google.inject.Provider;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
import java.util.EnumSet;
import java.util.Set;
public class Revert implements RestModifyView<ChangeResource, Input>,
UiCommand<ChangeResource> {
UiAction<ChangeResource> {
private final ChangeHooks hooks;
private final RevertedSender.Factory revertedSenderFactory;
private final CommitValidators.Factory commitValidatorsFactory;
@@ -113,34 +110,11 @@ public class Revert implements RestModifyView<ChangeResource, Input>,
}
@Override
public Set<Place> getPlaces() {
return EnumSet.of(Place.PATCHSET_ACTION_PANEL);
}
@Override
public String getLabel(ChangeResource resource) {
return "Revert";
}
@Override
public String getTitle(ChangeResource resource) {
return null;
}
@Override
public boolean isVisible(ChangeResource resource) {
return isEnabled(resource);
}
@Override
public boolean isEnabled(ChangeResource resource) {
return resource.getChange().getStatus() == Change.Status.MERGED
&& resource.getControl().getRefControl().canUpload();
}
@Override
public String getConfirmationMessage(ChangeResource resource) {
return null;
public UiAction.Description getDescription(ChangeResource resource) {
return new UiAction.Description()
.setLabel("Revert")
.setVisible(resource.getChange().getStatus() == Status.MERGED
&& resource.getControl().getRefControl().canUpload());
}
private static String status(Change change) {

View File

@@ -24,7 +24,7 @@ import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.webui.UiCommand;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -50,12 +50,10 @@ import java.io.IOException;
import java.sql.Timestamp;
import java.util.Collection;
import java.util.Collections;
import java.util.EnumSet;
import java.util.List;
import java.util.Set;
public class Submit implements RestModifyView<RevisionResource, Input>,
UiCommand<RevisionResource> {
UiAction<RevisionResource> {
public static class Input {
public boolean waitForMerge;
}
@@ -145,41 +143,15 @@ public class Submit implements RestModifyView<RevisionResource, Input>,
}
@Override
public Set<Place> getPlaces() {
return EnumSet.of(Place.PATCHSET_ACTION_PANEL);
}
@Override
public String getLabel(RevisionResource resource) {
return String.format(
"Submit Patch Set %d",
resource.getPatchSet().getPatchSetId());
}
@Override
public String getTitle(RevisionResource resource) {
return null;
}
@Override
public boolean isVisible(RevisionResource resource) {
public UiAction.Description getDescription(RevisionResource resource) {
PatchSet.Id current = resource.getChange().currentPatchSetId();
return resource.getChange().getStatus().isOpen()
&& resource.getPatchSet().getId().equals(current)
&& isEnabled(resource);
}
@Override
public boolean isEnabled(RevisionResource resource) {
// Enable based on approximation. If the user has permission enable,
// even if the change has not reached as submittable state according
// to the project rules.
return resource.getControl().canSubmit();
}
@Override
public String getConfirmationMessage(RevisionResource resource) {
return null;
return new UiAction.Description()
.setLabel(String.format(
"Submit Revision %d",
resource.getPatchSet().getPatchSetId()))
.setVisible(resource.getChange().getStatus().isOpen()
&& resource.getPatchSet().getId().equals(current)
&& resource.getControl().canSubmit());
}
/**

View File

@@ -19,82 +19,76 @@ import com.google.common.base.Predicate;
import com.google.common.base.Predicates;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import com.google.gerrit.common.data.UiCommandDetail;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.RestCollection;
import com.google.gerrit.extensions.restapi.RestResource;
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.extensions.webui.UiCommand;
import com.google.gerrit.extensions.webui.PrivateInternals_UiActionDescription;
import com.google.gerrit.extensions.webui.UiAction;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.Collections;
import java.util.Comparator;
import java.util.EnumSet;
import java.util.List;
import javax.annotation.Nullable;
public class UiCommands {
private static final Logger log = LoggerFactory.getLogger(UiCommands.class);
public class UiActions {
private static final Logger log = LoggerFactory.getLogger(UiActions.class);
public static Predicate<UiCommandDetail> enabled() {
return new Predicate<UiCommandDetail>() {
public static Predicate<UiAction.Description> enabled() {
return new Predicate<UiAction.Description>() {
@Override
public boolean apply(UiCommandDetail input) {
return input.enabled;
public boolean apply(UiAction.Description input) {
return input.isEnabled();
}
};
}
public static List<UiCommandDetail> sorted(Iterable<UiCommandDetail> in) {
List<UiCommandDetail> s = Lists.newArrayList(in);
Collections.sort(s, new Comparator<UiCommandDetail>() {
public static List<UiAction.Description> sorted(Iterable<UiAction.Description> in) {
List<UiAction.Description> s = Lists.newArrayList(in);
Collections.sort(s, new Comparator<UiAction.Description>() {
@Override
public int compare(UiCommandDetail a, UiCommandDetail b) {
return a.id.compareTo(b.id);
public int compare(UiAction.Description a, UiAction.Description b) {
return a.getId().compareTo(b.getId());
}
});
return s;
}
public static Iterable<UiCommandDetail> plugins(Iterable<UiCommandDetail> in) {
public static Iterable<UiAction.Description> plugins(Iterable<UiAction.Description> in) {
return Iterables.filter(in,
new Predicate<UiCommandDetail>() {
new Predicate<UiAction.Description>() {
@Override
public boolean apply(UiCommandDetail input) {
return input.id.indexOf('~') > 0;
public boolean apply(UiAction.Description input) {
return input.getId().indexOf('~') > 0;
}
});
}
public static <R extends RestResource> Iterable<UiCommandDetail> from(
public static <R extends RestResource> Iterable<UiAction.Description> from(
RestCollection<?, R> collection,
R resource,
EnumSet<UiCommand.Place> places) {
return from(collection.views(), resource, places);
R resource) {
return from(collection.views(), resource);
}
public static <R extends RestResource> Iterable<UiCommandDetail> from(
public static <R extends RestResource> Iterable<UiAction.Description> from(
DynamicMap<RestView<R>> views,
final R resource,
final EnumSet<UiCommand.Place> places) {
final R resource) {
return Iterables.filter(
Iterables.transform(
views,
new Function<DynamicMap.Entry<RestView<R>>, UiCommandDetail> () {
new Function<DynamicMap.Entry<RestView<R>>, UiAction.Description> () {
@Override
@Nullable
public UiCommandDetail apply(DynamicMap.Entry<RestView<R>> e) {
public UiAction.Description apply(DynamicMap.Entry<RestView<R>> e) {
int d = e.getExportName().indexOf('.');
if (d < 0) {
return null;
}
String method = e.getExportName().substring(0, d);
String name = e.getExportName().substring(d + 1);
RestView<R> view;
try {
view = e.getProvider().get();
@@ -105,31 +99,31 @@ public class UiCommands {
return null;
}
if (!(view instanceof UiCommand)) {
if (!(view instanceof UiAction)) {
return null;
}
UiCommand<R> cmd = (UiCommand<R>) view;
if (Sets.intersection(cmd.getPlaces(), places).isEmpty()
|| !cmd.isVisible(resource)) {
UiAction.Description dsc =
((UiAction<R>) view).getDescription(resource);
if (dsc == null || !dsc.isVisible()) {
return null;
}
UiCommandDetail dsc = new UiCommandDetail();
dsc.id = "gerrit".equals(e.getPluginName())
? name
: e.getPluginName() + '~' + name;
dsc.method = method;
dsc.label = cmd.getLabel(resource);
dsc.title = cmd.getTitle(resource);
dsc.enabled = cmd.isEnabled(resource);
dsc.confirmationMessage = cmd.getConfirmationMessage(resource);
String name = e.getExportName().substring(d + 1);
PrivateInternals_UiActionDescription.setMethod(
dsc,
e.getExportName().substring(0, d));
PrivateInternals_UiActionDescription.setId(
dsc,
"gerrit".equals(e.getPluginName())
? name
: e.getPluginName() + '~' + name);
return dsc;
}
}),
Predicates.notNull());
}
private UiCommands() {
private UiActions() {
}
}