From 909b312da97c99928f557757b2c6bd69c39d3318 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Fri, 17 Jul 2015 11:49:58 -0700 Subject: [PATCH 1/2] RebaseDialog: Use projectQuery to escape project name in operator A project name containing spaces or other special characters in the search language should be wrapped in "" to ensure the server can parse it accurately. Use PageLinks.toProjectQuery() to run the existing client side escaping rules. AND is the default operator between two terms in a query string, it is not necessary to specify it. Use - instead of NOT for the inverse of age:90d. Change-Id: I24fa20bd7320a916cdc1bab9fb5dd8a39e47e4a1 --- .../java/com/google/gerrit/client/ui/RebaseDialog.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/RebaseDialog.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/RebaseDialog.java index 2744877477..5aee9777b6 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/RebaseDialog.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/RebaseDialog.java @@ -20,8 +20,10 @@ import com.google.gerrit.client.changes.ChangeList; import com.google.gerrit.client.changes.Util; import com.google.gerrit.client.rpc.GerritCallback; import com.google.gerrit.client.rpc.Natives; +import com.google.gerrit.common.PageLinks; import com.google.gerrit.extensions.client.ListChangesOption; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Project; import com.google.gwt.event.dom.client.ClickEvent; import com.google.gwt.event.dom.client.ClickHandler; import com.google.gwt.user.client.ui.CheckBox; @@ -79,8 +81,9 @@ public abstract class RebaseDialog extends CommentedActionDialog { boolean checked = ((CheckBox) event.getSource()).getValue(); if (checked) { ChangeList.query( - "project:" + project + " AND branch:" + branch - + " AND is:open NOT age:90d", + PageLinks.projectQuery(new Project.NameKey(project)) + + " " + PageLinks.op("branch", branch) + + " is:open -age:90d", Collections. emptySet(), new GerritCallback() { @Override From 74606bb6ce359a2cb6a0e868823331970fa36fd4 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Fri, 17 Jul 2015 12:13:05 -0700 Subject: [PATCH 2/2] RebaseDialog: Clarify purpose of checkbox with comments Elaborate on the purpose of the SuggestBox and CheckBox widgets and how they interact together, to help future readers of this code try to understand this non-standard use of SuggestBox. Change-Id: I18a3eb89fda105262958efc6e028a499ef7623d4 --- .../google/gerrit/client/ui/RebaseDialog.java | 38 ++++++++++++------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/RebaseDialog.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/RebaseDialog.java index 5aee9777b6..18d3443498 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/RebaseDialog.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/RebaseDialog.java @@ -32,14 +32,14 @@ import com.google.gwt.user.client.ui.SuggestOracle.Suggestion; import com.google.gwtexpui.globalkey.client.GlobalKey; import com.google.gwtexpui.safehtml.client.HighlightSuggestOracle; +import java.util.ArrayList; import java.util.Collections; -import java.util.LinkedList; import java.util.List; public abstract class RebaseDialog extends CommentedActionDialog { private final SuggestBox base; - private final CheckBox cb; - private List changes; + private final CheckBox changeParent; + private List candidateChanges; private final boolean sendEnabled; public RebaseDialog(final String project, final String branch, @@ -48,13 +48,15 @@ public abstract class RebaseDialog extends CommentedActionDialog { this.sendEnabled = sendEnabled; sendButton.setText(Util.C.buttonRebaseChangeSend()); - // create the suggestion box + // Create the suggestion box to filter over a list of recent changes + // open on the same branch. The list of candidates is primed by the + // changeParent CheckBox (below) getting enabled by the user. base = new SuggestBox(new HighlightSuggestOracle() { @Override protected void onRequestSuggestions(Request request, Callback done) { String query = request.getQuery().toLowerCase(); - LinkedList suggestions = new LinkedList<>(); - for (final ChangeInfo ci : changes) { + List suggestions = new ArrayList<>(); + for (ChangeInfo ci : candidateChanges) { if (changeId.equals(ci.legacyId())) { continue; // do not suggest current change } @@ -73,13 +75,14 @@ public abstract class RebaseDialog extends CommentedActionDialog { Util.C.rebasePlaceholderMessage()); base.setStyleName(Gerrit.RESOURCES.css().rebaseSuggestBox()); - // the checkbox which must be clicked before the change list is populated - cb = new CheckBox(Util.C.rebaseConfirmMessage()); - cb.addClickHandler(new ClickHandler() { + // The changeParent checkbox must be clicked to load into browser memory + // a list of open changes from the same project and same branch that this + // change may rebase onto. + changeParent = new CheckBox(Util.C.rebaseConfirmMessage()); + changeParent.addClickHandler(new ClickHandler() { @Override public void onClick(ClickEvent event) { - boolean checked = ((CheckBox) event.getSource()).getValue(); - if (checked) { + if (changeParent.getValue()) { ChangeList.query( PageLinks.projectQuery(new Project.NameKey(project)) + " " + PageLinks.op("branch", branch) @@ -88,9 +91,16 @@ public abstract class RebaseDialog extends CommentedActionDialog { new GerritCallback() { @Override public void onSuccess(ChangeList result) { - changes = Natives.asList(result); + candidateChanges = Natives.asList(result); updateControls(true); } + + @Override + public void onFailure(Throwable err) { + updateControls(false); + changeParent.setValue(false); + super.onFailure(err); + } }); } else { updateControls(false); @@ -99,7 +109,7 @@ public abstract class RebaseDialog extends CommentedActionDialog { }); // add the checkbox and suggestbox widgets to the content panel - contentPanel.add(cb); + contentPanel.add(changeParent); contentPanel.add(base); contentPanel.setStyleName(Gerrit.RESOURCES.css().rebaseContentPanel()); } @@ -131,7 +141,7 @@ public abstract class RebaseDialog extends CommentedActionDialog { } public String getBase() { - return cb.getValue() ? base.getText() : null; + return changeParent.getValue() ? base.getText() : null; } private static class ChangeSuggestion implements Suggestion {