Issue 59: No links from side-by-side view back to the associated change
Status:  Released
Owner:
Closed:  Oct 2012
Reported by code-rev...@gtempaccount.com, Sep 24, 2009
Reported by Dave Bort <dbort@android.com> on Fri Jan 30 16:03:56 PST 2009
Source: JIRA GERRIT-59

Environment:
Gerrit Code Review (v2.0-47-g9e727d3)
Firefox 3.0 on Dapper x86-mixed

- Go to a change like http://review.source.android.com/Gerrit#change,8718
- Click on "side-by-side" diff for a file, like
http://review.source.android.com/Gerrit#patch,sidebyside,8718,1,core/find-jdk-tools-jar.sh

Notice that on the resulting page, there are no links back to the change, so
it's not obvious how to progress to the next file or to the "publish drafts"
step.

I also missed links to the next/previous file, and information providing the
context of the change (branch, project, patch submitter, etc.)
side-by-side.png
53.4 KB   View   Download
Sep 24, 2009
#1 code-rev...@gtempaccount.com
Update by Shawn Pearce <sop@google.com> on Mon Mar 09 12:55:59 PDT 2009

Assigned to Brad Larson.
Sep 24, 2009
#2 code-rev...@gtempaccount.com
Comment by Brad Larson <bklarson@gmail.com> on Tue Mar 24 20:37:47 PDT 2009

I got a chance to look into this a bit tonight.  I'm mostly concerned with the
'back to change', 'previous file', and 'next file' links.  I see two options
for handling this:

1. Modify the Patch table to include a previous and next ID fields
2. Grab the PatchSetDetail from the DB when we load a patch page and loop
through to find the prev/next keys

1 involves a DB schema change and lots more coding when a change is uploaded,
but wouldn't require the extra DB hit(s).  I'm favoring 2 right now because it
seems more straight-forward.  Shawn, would you be OK with that approach?

Also, this is a really low priority for me right now... if somebody wants it
sooner, feel free to grab the task from me.
Sep 24, 2009
#3 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Wed Apr 08 09:14:07 PDT 2009

You're right, 1 will produce a more efficient implementation in the long run.

But I was thinking that 2 might be the way to go here.

In particular, we already have the entire PatchSetDetail loaded in the
ChangeScreen that got us to the side by side view.  So we could just pass that
data down into the side by side view if we were created from it; and if not
then we can use a background async RPC to load it.  If we were really cute we
could actually reuse the entire PatchTable UI widget from the ChangeScreen
inside the side by side view, and pop it open into a PopupPanel when the user
wants to jump to another file.

This would give instant navigation in the client to nearly any file, and we'd
only incur the database hit to load the entire list of patches once, when the
change is first viewed, or once when the side by side view is started if the
user direct linked to it.
Sep 24, 2009
#4 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Fri May 15 18:41:07 PDT 2009

Partially handled by 'u' now being available as a keyboard shortcut up, but we
still need a hyperlink.
Sep 24, 2009
#5 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Sat May 30 11:32:20 PDT 2009

Fixed by https://review.source.android.com/10019
Sep 24, 2009
#6 code-rev...@gtempaccount.com
Update by Shawn Pearce <sop@google.com> on Sat May 30 11:32:20 PDT 2009

Assigned to Cedric Beust.
Fixed in version 2.0.13.
Status: Fixed
Sep 25, 2009
#7 code-rev...@gtempaccount.com
(No comment was entered for this change.)
Labels: FixedIn-2.0.13
Oct 25, 2012
#8 sop@google.com
(No comment was entered for this change.)
Status: Released