opendev/gerrit
Code Issues Proposed changes

Improve rebase usability with the RebaseDialog

Browse Source 
Since [1] the Rebase button is always visible for the current patch
set. This leads to somewhat degraded usability, since its no longer
obvious whether a change can be rebased or the change is up-to-date
and consequently any rebase attempts will fail.

This patch solves the problem by:
1. Displaying an orange circle next to the 'Parent(s)' text when the
   parent commit is not current, meaning the parent branch or change
   has moved on.
2. The Rebase (send) button in the RebaseDialog will be disabled, if
   the change is up-to-date and the "Change parent revision"
   checkbox is disabled (default).

[1] https://gerrit-review.googlesource.com/#/c/63196/

Change-Id: Iaa1b0ce4cf3309330a55d1ab48d5422536089667
This commit is contained in:
Zalan Blenessy 2015-03-20 17:31:54 +01:00 committed by David Pursehouse
parent b0393592e2
commit 0d99067b8d
14 changed files with 130 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Actions.java
View File

@ -128,6 +128,10 @@ class Actions extends Composite {
}
a2b(actions, "cherrypick", cherrypick);
a2b(actions, "rebase", rebase);
if (rebase.isVisible()) {
// it is the rebase button in RebaseDialog that the server wants to disable
rebase.setEnabled(true);
}
for (String id : filterNonCore(actions)) {
add(new ActionButton(info, revInfo, actions.get(id)));
}
@ -177,7 +181,16 @@ class Actions extends Composite {
@UiHandler("rebase")
void onRebase(@SuppressWarnings("unused") ClickEvent e) {
RebaseAction.call(rebase, project, changeInfo.branch(), changeId, revision);
boolean enabled = true;
RevisionInfo revInfo = changeInfo.revision(revision);
if (revInfo.has_actions()) {
NativeMap<ActionInfo> actions = revInfo.actions();
if (actions.containsKey("rebase")) {
enabled = actions.get("rebase").enabled();
}
}
RebaseAction.call(rebase, project, changeInfo.branch(), changeId, revision,
enabled);
}
@UiHandler("submit")

16
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/CommitBox.java
View File

@ -19,11 +19,13 @@ import com.google.gerrit.client.FormatUtil;
import com.google.gerrit.client.Gerrit;
import com.google.gerrit.client.GitwebLink;
import com.google.gerrit.client.WebLinkInfo;
import com.google.gerrit.client.actions.ActionInfo;
import com.google.gerrit.client.account.AccountInfo;
import com.google.gerrit.client.changes.ChangeInfo;
import com.google.gerrit.client.changes.ChangeInfo.CommitInfo;
import com.google.gerrit.client.changes.ChangeInfo.GitPerson;
import com.google.gerrit.client.changes.ChangeInfo.RevisionInfo;
import com.google.gerrit.client.rpc.NativeMap;
import com.google.gerrit.client.rpc.Natives;
import com.google.gerrit.client.ui.CommentLinkProcessor;
import com.google.gerrit.client.ui.InlineHyperlink;
@ -46,6 +48,7 @@ import com.google.gwt.user.client.ui.HTML;
import com.google.gwt.user.client.ui.HTMLPanel;
import com.google.gwt.user.client.ui.Image;
import com.google.gwt.user.client.ui.ScrollPanel;
import com.google.gwt.user.client.ui.UIObject;
import com.google.gwtexpui.clippy.client.CopyableLabel;
import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder;
@ -77,6 +80,7 @@ class CommitBox extends Composite {
@UiField HTML text;
@UiField ScrollPanel scroll;
@UiField Button more;
@UiField Element parentNotCurrentText;
private boolean expanded;
CommitBox() {
@ -121,7 +125,19 @@ class CommitBox extends Composite {
if (revInfo.commit().parents().length() > 1) {
mergeCommit.setVisible(true);
}
setParents(change.project(), revInfo.commit().parents());
// display the orange ball if parent has moved on (not current)
boolean parentNotCurrent = false;
if (revInfo.has_actions()) {
NativeMap<ActionInfo> actions = revInfo.actions();
if (actions.containsKey("rebase")) {
parentNotCurrent = actions.get("rebase").enabled();
}
}
UIObject.setVisible(parentNotCurrentText, parentNotCurrent);
parentNotCurrentText.setInnerText(parentNotCurrent ? "\u25CF" : "");
}
private void setWebLinks(ChangeInfo change, String revision,

22
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/CommitBox.ui.xml
View File

@ -68,7 +68,7 @@ limitations under the License.
padding: 0;
width: 560px;
}
.header th { width: 70px; }
.header th { width: 72px; }
.header td { white-space: nowrap; }
.date { width: 132px; }
@ -106,6 +106,16 @@ limitations under the License.
height: 16px !important;
vertical-align: bottom;
}
.parent {
margin-right: 3px;
float: left;
}
.parentNotCurrent {
color: #FFA62F; <!-- orange -->
font-weight: bold;
}
</ui:style>
<g:HTMLPanel>
<g:ScrollPanel styleName='{style.scroll}' ui:field='scroll'>
@ -163,7 +173,15 @@ limitations under the License.
</td>
</tr>
<tr ui:field='firstParent' style='display: none'>
<th><ui:msg>Parent(s)</ui:msg></th>
<th>
<div class='{style.parent}'>
<ui:msg>Parent(s)</ui:msg>
</div>
<div ui:field='parentNotCurrentText'
title='Not current - rebase possible'
class='{style.parentNotCurrent}'
style='display: none' aria-hidden='true'/>
</th>
<td>
<g:FlowPanel ui:field='parentCommits'/>
</td>

4
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/RebaseAction.java
View File

@ -27,10 +27,10 @@ import com.google.gwt.user.client.ui.PopupPanel;
class RebaseAction {
static void call(final Button b, final String project, final String branch,
final Change.Id id, final String revision) {
final Change.Id id, final String revision, final boolean enabled) {
b.setEnabled(false);
new RebaseDialog(project, branch, id) {
new RebaseDialog(project, branch, id, enabled) {
@Override
public void onSend() {
ChangeApi.rebase(id.get(), revision, getBase(), new GerritCallback<ChangeInfo>() {

1
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java
View File

@ -166,6 +166,7 @@ public interface ChangeConstants extends Constants {
String buttonRebaseChangeSend();
String rebaseConfirmMessage();
String rebaseNotPossibleMessage();
String rebasePlaceholderMessage();
String rebaseTitle();

1
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties
View File

@ -152,6 +152,7 @@ cherryPickTitle = Code Review - Cherry Pick Change to Another Branch
buttonRebaseChangeSend = Rebase
rebaseConfirmMessage = Change parent revision
rebaseNotPossibleMessage = Change is already up to date
rebasePlaceholderMessage = (subject, change number, or leave empty)
rebaseTitle = Code Review - Rebase Change

32
gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/RebaseDialog.java
View File

@ -36,10 +36,12 @@ public abstract class RebaseDialog extends CommentedActionDialog {
private final SuggestBox base;
private final CheckBox cb;
private List<ChangeInfo> changes;
private final boolean sendEnabled;
public RebaseDialog(final String project, final String branch,
final Change.Id changeId) {
final Change.Id changeId, final boolean sendEnabled) {
super(Util.C.rebaseTitle(), null);
this.sendEnabled = sendEnabled;
sendButton.setText(Util.C.buttonRebaseChangeSend());
// create the suggestion box
@ -63,7 +65,6 @@ public abstract class RebaseDialog extends CommentedActionDialog {
done.onSuggestionsReady(request, new Response(suggestions));
}
});
base.setEnabled(false);
base.getElement().setAttribute("placeholder",
Util.C.rebasePlaceholderMessage());
base.setStyleName(Gerrit.RESOURCES.css().rebaseSuggestBox());
@ -81,13 +82,11 @@ public abstract class RebaseDialog extends CommentedActionDialog {
@Override
public void onSuccess(ChangeList result) {
changes = Natives.asList(result);
base.setEnabled(true);
base.setFocus(true);
updateControls(true);
}
});
} else {
base.setEnabled(false);
sendButton.setFocus(true);
updateControls(false);
}
}
});
@ -102,7 +101,26 @@ public abstract class RebaseDialog extends CommentedActionDialog {
public void center() {
super.center();
GlobalKey.dialog(this);
sendButton.setFocus(true);
updateControls(false);
}
private void updateControls(boolean changeParentEnabled) {
if (changeParentEnabled) {
sendButton.setTitle(null);
sendButton.setEnabled(true);
base.setEnabled(true);
base.setFocus(true);
} else {
base.setEnabled(false);
sendButton.setEnabled(sendEnabled);
if (sendEnabled) {
sendButton.setTitle(null);
sendButton.setFocus(true);
} else {
sendButton.setTitle(Util.C.rebaseNotPossibleMessage());
cancelButton.setFocus(true);
}
}
}
public String getBase() {

8
gerrit-server/src/main/java/com/google/gerrit/server/change/ActionJson.java
View File

@ -22,6 +22,7 @@ import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.extensions.webui.PrivateInternals_UiActionDescription;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.changedetail.RebaseChange;
import com.google.gerrit.server.extensions.webui.UiActions;
import com.google.gerrit.server.project.ChangeControl;
import com.google.inject.Inject;
@ -36,13 +37,16 @@ import java.util.Map;
public class ActionJson {
private final Revisions revisions;
private final DynamicMap<RestView<ChangeResource>> changeViews;
private final RebaseChange rebaseChange;
@Inject
ActionJson(
Revisions revisions,
DynamicMap<RestView<ChangeResource>> changeViews) {
DynamicMap<RestView<ChangeResource>> changeViews,
RebaseChange rebaseChange) {
this.revisions = revisions;
this.changeViews = changeViews;
this.rebaseChange = rebaseChange;
}
public Map<String, ActionInfo> format(RevisionResource rsrc) {
@ -69,7 +73,7 @@ public class ActionJson {
Provider<CurrentUser> userProvider = Providers.of(ctl.getCurrentUser());
for (UiAction.Description d : UiActions.from(
changeViews,
new ChangeResource(ctl),
new ChangeResource(ctl, rebaseChange),
userProvider)) {
out.put(d.getId(), new ActionInfo(d));
}

8
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
View File

@ -92,6 +92,7 @@ import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchLineCommentsUtil;
import com.google.gerrit.server.WebLinks;
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.changedetail.RebaseChange;
import com.google.gerrit.server.git.LabelNormalizer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
@ -142,6 +143,7 @@ public class ChangeJson {
private final PatchLineCommentsUtil plcUtil;
private final Provider<ConsistencyChecker> checkerProvider;
private final ActionJson actionJson;
private final RebaseChange rebaseChange;
private AccountLoader accountLoader;
private FixInput fix;
@ -163,7 +165,8 @@ public class ChangeJson {
ChangeMessagesUtil cmUtil,
PatchLineCommentsUtil plcUtil,
Provider<ConsistencyChecker> checkerProvider,
ActionJson actionJson) {
ActionJson actionJson,
RebaseChange rebaseChange) {
this.db = db;
this.labelNormalizer = ln;
this.userProvider = user;
@ -180,6 +183,7 @@ public class ChangeJson {
this.plcUtil = plcUtil;
this.checkerProvider = checkerProvider;
this.actionJson = actionJson;
this.rebaseChange = rebaseChange;
options = EnumSet.noneOf(ListChangesOption.class);
}
@ -890,7 +894,7 @@ public class ChangeJson {
&& userProvider.get().isIdentifiedUser()) {
actionJson.addRevisionActions(out,
new RevisionResource(new ChangeResource(ctl), in));
new RevisionResource(new ChangeResource(ctl, rebaseChange), in));
}
if (has(DRAFT_COMMENTS)

9
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeResource.java
View File

@ -23,6 +23,7 @@ import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.changedetail.RebaseChange;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectState;
@ -36,13 +37,16 @@ public class ChangeResource implements RestResource, HasETag {
new TypeLiteral<RestView<ChangeResource>>() {};
private final ChangeControl control;
private final RebaseChange rebaseChange;
public ChangeResource(ChangeControl control) {
public ChangeResource(ChangeControl control, RebaseChange rebaseChange) {
this.control = control;
this.rebaseChange = rebaseChange;
}
protected ChangeResource(ChangeResource copy) {
this.control = copy.control;
this.rebaseChange = copy.rebaseChange;
}
public ChangeControl getControl() {
@ -66,7 +70,8 @@ public class ChangeResource implements RestResource, HasETag {
.putBoolean(user.getStarredChanges().contains(getChange().getId()))
.putInt(user.isIdentifiedUser()
? ((IdentifiedUser) user).getAccountId().get()
: 0);
: 0)
.putBoolean(rebaseChange != null && rebaseChange.canRebase(this));
byte[] buf = new byte[20];
ObjectId noteId;

10
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java
View File

@ -26,6 +26,7 @@ import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.changedetail.RebaseChange;
import com.google.gerrit.server.index.ChangeIndexer;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
@ -49,6 +50,7 @@ public class ChangesCollection implements
private final ChangeUtil changeUtil;
private final CreateChange createChange;
private final ChangeIndexer changeIndexer;
private final RebaseChange rebaseChange;
@Inject
ChangesCollection(
@ -58,7 +60,8 @@ public class ChangesCollection implements
DynamicMap<RestView<ChangeResource>> views,
ChangeUtil changeUtil,
CreateChange createChange,
ChangeIndexer changeIndexer) {
ChangeIndexer changeIndexer,
RebaseChange rebaseChange) {
this.user = user;
this.changeControlFactory = changeControlFactory;
this.queryFactory = queryFactory;
@ -66,6 +69,7 @@ public class ChangesCollection implements
this.changeUtil = changeUtil;
this.createChange = createChange;
this.changeIndexer = changeIndexer;
this.rebaseChange = rebaseChange;
}
@Override
@ -102,7 +106,7 @@ public class ChangesCollection implements
} catch (NoSuchChangeException e) {
throw new ResourceNotFoundException(id);
}
return new ChangeResource(control);
return new ChangeResource(control, rebaseChange);
}
public ChangeResource parse(Change.Id id)
@ -112,7 +116,7 @@ public class ChangesCollection implements
}
public ChangeResource parse(ChangeControl control) {
return new ChangeResource(control);
return new ChangeResource(control, rebaseChange);
}
@SuppressWarnings("unchecked")

8
gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java
View File

@ -214,12 +214,18 @@ public class Rebase implements RestModifyView<RevisionResource, RebaseInput>,
@Override
public UiAction.Description getDescription(RevisionResource resource) {
return new UiAction.Description()
UiAction.Description descr = new UiAction.Description()
.setLabel("Rebase")
.setTitle("Rebase onto tip of branch or parent change")
.setVisible(resource.getChange().getStatus().isOpen()
&& resource.getControl().canRebase()
&& hasOneParent(resource.getPatchSet().getId()));
if (descr.isVisible()) {
// Disable the rebase button in the RebaseDialog if
// the change cannot be rebased.
descr.setEnabled(rebaseChange.get().canRebase(resource));
}
return descr;
}
public static class CurrentRevision implements

19
gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java
View File

@ -22,11 +22,13 @@ import com.google.gerrit.reviewdb.client.Change.Status;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetAncestor;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.PatchSetInserter;
import com.google.gerrit.server.change.PatchSetInserter.ValidatePolicy;
import com.google.gerrit.server.change.RevisionResource;
@ -356,10 +358,21 @@ public class RebaseChange {
return objectId;
}
public boolean canRebase(ChangeResource r) {
Change c = r.getChange();
return canRebase(c.getProject(), c.currentPatchSetId(), c.getDest());
}
public boolean canRebase(RevisionResource r) {
return canRebase(r.getChange().getProject(),
r.getPatchSet().getId(), r.getChange().getDest());
}
public boolean canRebase(Project.NameKey project,
PatchSet.Id patchSetId, Branch.NameKey branch) {
Repository git;
try {
git = gitManager.openRepository(r.getChange().getProject());
git = gitManager.openRepository(project);
} catch (RepositoryNotFoundException err) {
return false;
} catch (IOException err) {
@ -367,9 +380,9 @@ public class RebaseChange {
}
try {
findBaseRevision(
r.getPatchSet().getId(),
patchSetId,
db.get(),
r.getChange().getDest(),
branch,
git,
null,
null,

4
gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java
View File

@ -298,8 +298,8 @@ public class CommentsTest {
update.commit();
ChangeControl ctl = stubChangeControl(change);
revRes1 = new RevisionResource(new ChangeResource(ctl), ps1);
revRes2 = new RevisionResource(new ChangeResource(ctl), ps2);
revRes1 = new RevisionResource(new ChangeResource(ctl, null), ps1);
revRes2 = new RevisionResource(new ChangeResource(ctl, null), ps2);
}
private ChangeControl stubChangeControl(Change c) throws OrmException {