From 547e8e8ebfc38e039f87c3e3bb11dfe708e2b92d Mon Sep 17 00:00:00 2001 From: Raviteja Sunkara Date: Wed, 2 Mar 2011 19:34:17 +0530 Subject: [PATCH] Enhance UpToChange link to activate the last browsed patchset. While returning to the Change using "UpToChange" link in any patchset, always the latest patchset is activated. This commit modifies the functionality of the "UpToChange" link by storing the last browsed patchset and activating it on return. This makes it easy to review older patchsets. Bug: issue 822 Change-Id: Iaf01c53797d92434a0bc150f73ad232aec3619d6 --- .../com/google/gerrit/common/PageLinks.java | 5 +++++ .../com/google/gerrit/client/Dispatcher.java | 11 ++++++++-- .../gerrit/client/changes/ChangeScreen.java | 10 +++++++++ .../gerrit/client/changes/PatchSetsBlock.java | 21 ++++++++++++------- .../gerrit/client/patches/NavLinks.java | 12 +++++------ .../gerrit/client/patches/PatchScreen.java | 8 +++---- .../client/patches/UpToChangeCommand.java | 10 ++++----- .../google/gerrit/client/ui/ChangeLink.java | 17 ++++++++++++++- 8 files changed, 67 insertions(+), 27 deletions(-) diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/PageLinks.java b/gerrit-common/src/main/java/com/google/gerrit/common/PageLinks.java index 0f29214269..efe311fa90 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/PageLinks.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/PageLinks.java @@ -18,6 +18,7 @@ import com.google.gerrit.common.data.AccountInfo; import com.google.gerrit.common.data.ChangeInfo; import com.google.gerrit.reviewdb.Account; import com.google.gerrit.reviewdb.Change; +import com.google.gerrit.reviewdb.PatchSet; import com.google.gerrit.reviewdb.Project; import com.google.gerrit.reviewdb.Change.Status; import com.google.gwtorm.client.KeyUtil; @@ -49,6 +50,10 @@ public class PageLinks { return "change," + c.toString(); } + public static String toChange(final PatchSet.Id ps) { + return "change," + ps.getParentKey().toString() + ",patchset=" + ps.get(); + } + public static String toAccountDashboard(final AccountInfo acct) { return toAccountDashboard(acct.getId()); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/Dispatcher.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/Dispatcher.java index 383a226ba8..2b3ace4651 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/Dispatcher.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/Dispatcher.java @@ -232,8 +232,15 @@ public class Dispatcher { String p; p = "change,"; - if (token.startsWith(p)) - return new ChangeScreen(Change.Id.parse(skip(p, token))); + if (token.startsWith(p)) { + final String s = skip(p, token); + final String q = "patchset="; + final String t[] = s.split(",", 2); + if (t.length > 1 && t[1].startsWith(q)) { + return new ChangeScreen(PatchSet.Id.parse(t[0] + "," + skip(q, t[1]))); + } + return new ChangeScreen(Change.Id.parse(t[0])); + } p = "dashboard,"; if (token.startsWith(p)) 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 4db387f9ce..7213d8808d 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 @@ -54,6 +54,7 @@ import java.util.List; public class ChangeScreen extends Screen { private final Change.Id changeId; + private final PatchSet.Id openPatchSetId; private Image starChange; private boolean starred; @@ -78,6 +79,12 @@ public class ChangeScreen extends Screen { public ChangeScreen(final Change.Id toShow) { changeId = toShow; + openPatchSetId = null; + } + + public ChangeScreen(final PatchSet.Id toShow) { + changeId = toShow.getParentKey(); + openPatchSetId = toShow; } public ChangeScreen(final ChangeInfo c) { @@ -251,6 +258,9 @@ public class ChangeScreen extends Screen { .getApprovals()); patchSetsBlock.display(detail); + if (openPatchSetId != null) { + patchSetsBlock.activate(openPatchSetId); + } addComments(detail); // If any dependency change is still open, show our dependency list. diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetsBlock.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetsBlock.java index edfeb235e6..f453b4f90a 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetsBlock.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetsBlock.java @@ -15,6 +15,7 @@ package com.google.gerrit.client.changes; import com.google.gerrit.client.Gerrit; +import com.google.gerrit.common.PageLinks; import com.google.gerrit.common.data.ChangeDetail; import com.google.gerrit.reviewdb.AccountGeneralPreferences; import com.google.gerrit.reviewdb.PatchSet; @@ -151,14 +152,18 @@ public class PatchSetsBlock extends Composite { * This method also ensures that the current row is only highlighted in the * table of the active patch set panel. */ - private void activate(final PatchSet.Id patchSetId) { - if (!patchSetId.equals(activePatchSetId)) { - deactivate(); - PatchSetComplexDisclosurePanel patchSetPanel = - patchSetPanels.get(patchSetId); - patchSetPanel.setOpen(true); - patchSetPanel.setActive(true); - activePatchSetId = patchSetId; + public void activate(final PatchSet.Id patchSetId) { + if (indexOf(patchSetId) != -1) { + if (!patchSetId.equals(activePatchSetId)) { + deactivate(); + PatchSetComplexDisclosurePanel patchSetPanel = + patchSetPanels.get(patchSetId); + patchSetPanel.setOpen(true); + patchSetPanel.setActive(true); + activePatchSetId = patchSetId; + } + } else { + Gerrit.display(PageLinks.toChange(patchSetId.getParentKey())); } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/NavLinks.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/NavLinks.java index b8c52d8cb6..6d42cc9c68 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/NavLinks.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/NavLinks.java @@ -19,7 +19,7 @@ import com.google.gerrit.client.changes.PatchTable; import com.google.gerrit.client.changes.Util; import com.google.gerrit.client.ui.ChangeLink; import com.google.gerrit.client.ui.InlineHyperlink; -import com.google.gerrit.reviewdb.Change; +import com.google.gerrit.reviewdb.PatchSet; import com.google.gwt.event.dom.client.KeyPressEvent; import com.google.gwt.user.client.ui.Composite; import com.google.gwt.user.client.ui.Grid; @@ -47,14 +47,14 @@ class NavLinks extends Composite { } } - private final Change.Id changeId; + private final PatchSet.Id patchSetId; private final KeyCommandSet keys; private final Grid table; private KeyCommand cmds[] = new KeyCommand[2]; - NavLinks(KeyCommandSet kcs, Change.Id forChange) { - changeId = forChange; + NavLinks(KeyCommandSet kcs, PatchSet.Id forPatch) { + patchSetId = forPatch; keys = kcs; table = new Grid(1, 3); initWidget(table); @@ -65,7 +65,7 @@ class NavLinks extends Composite { fmt.setHorizontalAlignment(0, 1, HasHorizontalAlignment.ALIGN_CENTER); fmt.setHorizontalAlignment(0, 2, HasHorizontalAlignment.ALIGN_RIGHT); - final ChangeLink up = new ChangeLink("", changeId); + final ChangeLink up = new ChangeLink("", patchSetId); SafeHtml.set(up, SafeHtml.asis(Util.C.upToChangeIconLink())); table.setWidget(0, 1, up); } @@ -104,7 +104,7 @@ class NavLinks extends Composite { } }; } else { - cmds[nav.cmd] = new UpToChangeCommand(changeId, 0, nav.key); + cmds[nav.cmd] = new UpToChangeCommand(patchSetId, 0, nav.key); } keys.add(cmds[nav.cmd]); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java index cbe037b7ee..cad371e39a 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java @@ -250,9 +250,8 @@ public abstract class PatchScreen extends Screen implements protected void onInitUI() { super.onInitUI(); - final Change.Id ck = patchKey.getParentKey().getParentKey(); keysNavigation = new KeyCommandSet(Gerrit.C.sectionNavigation()); - keysNavigation.add(new UpToChangeCommand(ck, 0, 'u')); + keysNavigation.add(new UpToChangeCommand(patchKey.getParentKey(), 0, 'u')); keysNavigation.add(new FileListCmd(0, 'f', PatchUtil.C.fileList())); historyTable = new HistoryTable(this); @@ -284,9 +283,8 @@ public abstract class PatchScreen extends Screen implements contentTable = createContentTable(); contentTable.fileList = fileList; - topNav = - new NavLinks(keysNavigation, patchKey.getParentKey().getParentKey()); - bottomNav = new NavLinks(null, patchKey.getParentKey().getParentKey()); + topNav = new NavLinks(keysNavigation, patchKey.getParentKey()); + bottomNav = new NavLinks(null, patchKey.getParentKey()); add(topNav); contentPanel = new FlowPanel(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/UpToChangeCommand.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/UpToChangeCommand.java index 709065d3d9..3faa99826d 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/UpToChangeCommand.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/UpToChangeCommand.java @@ -17,20 +17,20 @@ package com.google.gerrit.client.patches; import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.changes.ChangeScreen; import com.google.gerrit.common.PageLinks; -import com.google.gerrit.reviewdb.Change; +import com.google.gerrit.reviewdb.PatchSet; import com.google.gwt.event.dom.client.KeyPressEvent; import com.google.gwtexpui.globalkey.client.KeyCommand; class UpToChangeCommand extends KeyCommand { - private final Change.Id changeId; + private final PatchSet.Id patchSetId; - UpToChangeCommand(Change.Id changeId, int mask, int key) { + UpToChangeCommand(PatchSet.Id patchSetId, int mask, int key) { super(mask, key, PatchUtil.C.upToChange()); - this.changeId = changeId; + this.patchSetId = patchSetId; } @Override public void onKeyPress(final KeyPressEvent event) { - Gerrit.display(PageLinks.toChange(changeId), new ChangeScreen(changeId)); + Gerrit.display(PageLinks.toChange(patchSetId), new ChangeScreen(patchSetId)); } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ChangeLink.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ChangeLink.java index 589b776f48..0d0dfdb362 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ChangeLink.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ChangeLink.java @@ -19,6 +19,7 @@ import com.google.gerrit.client.changes.ChangeScreen; import com.google.gerrit.common.PageLinks; import com.google.gerrit.common.data.ChangeInfo; import com.google.gerrit.reviewdb.Change; +import com.google.gerrit.reviewdb.PatchSet; import com.google.gwt.core.client.GWT; import com.google.gwt.user.client.DOM; @@ -28,12 +29,20 @@ public class ChangeLink extends InlineHyperlink { } protected Change.Id id; + protected PatchSet.Id ps; private ChangeInfo info; public ChangeLink(final String text, final Change.Id c) { super(text, PageLinks.toChange(c)); DOM.setElementProperty(getElement(), "href", permalink(c)); id = c; + ps = null; + } + + public ChangeLink(final String text, final PatchSet.Id ps) { + super(text, PageLinks.toChange(ps)); + id = ps.getParentKey(); + this.ps = ps; } public ChangeLink(final String text, final ChangeInfo c) { @@ -47,6 +56,12 @@ public class ChangeLink extends InlineHyperlink { } private Screen createScreen() { - return info != null ? new ChangeScreen(info) : new ChangeScreen(id); + if (info != null) { + return new ChangeScreen(info); + } else if (ps != null) { + return new ChangeScreen(ps); + } else { + return new ChangeScreen(id); + } } }