RFR: CODETOOLS-7901044: Add next/prev links to navigate between diffs

[John Coomes]

Bengt Rutisson (bengt.rutisson at oracle.com) wrote:
> 
> On 2014-10-02 01:08, John Coomes wrote:
> > Bengt Rutisson (bengt.rutisson at oracle.com) wrote:
> >> Hi Daniel,
> >>
> >> On 10/1/14 4:27 PM, Daniel Fuchs wrote:
> >>> Hi Bengt,
> >>>
> >>> The new version looks good to me, but I am not a reviewer
> >>> fore CODE_TOOLS.
> >> Thanks for looking at this Daniel!
> >>
> >>> Hopefully someone who is will step in :-)
> >> Yes, and I am not a committer in code-tools so I need a sponsor for the
> >> patch too.
> > Looks good to me; I'll sponsor it.  Send me (privately) the result of
> > "hg export --git" and I'll push that.
> 
> Thanks for looking at this, John! And thanks for sponsoring the change. 
> I'll send you the export in a separate email.

Got it; it's been pushed.

-John

> >>> best regards,
> >>>
> >>> -- daniel
> >>>
> >>> On 01/10/14 15:57, Bengt Rutisson wrote:
> >>>> Hi Daniel,
> >>>>
> >>>> Thanks for looking at this!
> >>>>
> >>>> On 2014-10-01 15:53, Daniel Fuchs wrote:
> >>>>> Hi Bengt,
> >>>>>
> >>>>> This is a really cool feature!
> >>>> Thanks!
> >>>>
> >>>>> I have downloaded your patch and applied it in my
> >>>>> local webrev repo - and played with the new
> >>>>> webrev in a couple of repositories where I have
> >>>>> pending uncommitted changes.
> >>>>>
> >>>>> This seems to work well.
> >>>>>
> >>>>> I have used:
> >>>>>
> >>>>>    webrev -fN from top level repo
> >>>>>    webrev -N  from jdk repo
> >>>>>    webrev -fN from jdk repo (with changes in test/closed)
> >>>>>
> >>>> Great that you tired it out.
> >>>>
> >>>>> I also had a cursory look at your patch. The only thing
> >>>>> I noticed is that 'typeset' is not consistently used in
> >>>>> the original webrev file. I'm not sure whether that's
> >>>>> intended or not.
> >>>>> I believe that's ok - but I've been bitten by not using
> >>>>> typeset before ;-( - maybe - for the sake of uniformity -
> >>>>> you could use typeset at least in those functions that
> >>>>> already use  it for their parameters? For instance
> >>>>> lines 532-534, and possibly line 560 as well?
> >>>>> Or did you intentionally not used it?
> >>>> I also noticed the inconsistent use of typeset. I fixed the lines you
> >>>> pointed out. I wasn't sure which way to go. But I think your suggestion
> >>>> is good.
> >>>>
> >>>>> Also you should increment the version of webrev.
> >>>>> IIRC that would be 25.6:
> >>>>>
> >>>>> - WEBREV_UPDATED=25.5-hg+openjdk.java.net
> >>>>> + WEBREV_UPDATED=25.6-hg+openjdk.java.net
> >>>> Thanks, didn't think of that.
> >>>>
> >>>> New webrev:
> >>>> http://cr.openjdk.java.net/~brutisso/CODETOOLS-7901044/webrev.01/
> >>>>
> >>>> Diff compared to the first version:
> >>>> http://cr.openjdk.java.net/~brutisso/CODETOOLS-7901044/webrev.00-01.diff/
> >>>>
> >>>>
> >>>> Thanks,
> >>>> Bengt
> >>>>
> >>>>> best regards,
> >>>>>
> >>>>> -- daniel
> >>>>>
> >>>>> On 01/10/14 10:56, Bengt Rutisson wrote:
> >>>>>> Hi everyone,
> >>>>>>
> >>>>>> (This is a patch for webrev, but I am cross posting to the JDK 9 dev
> >>>>>> list since I'm not sure how many are on the webrev-dev list and
> >>>>>> most JDK
> >>>>>> developers might have opinions about this change.)
> >>>>>>
> >>>>>> Can I get a couple of reviews for this small enhancement to the webrev
> >>>>>> tool?
> >>>>>>
> >>>>>> CODETOOLS-7901044: Add next/prev links to navigate between diffs
> >>>>>> https://bugs.openjdk.java.net/browse/CODETOOLS-7901044
> >>>>>>
> >>>>>> Webrev:
> >>>>>> http://cr.openjdk.java.net/~brutisso/CODETOOLS-7901044/webrev.00/
> >>>>>>
> >>>>>>
> >>>>>> Background:
> >>>>>> When browsing review requests that span many files it is quite time
> >>>>>> consuming to have to go back to the index page to view to the next or
> >>>>>> previous file. This patch adds links to the next and previous files in
> >>>>>> the diff views (including frames).
> >>>>>>
> >>>>>> The above webrev is generated with this new version, but since it only
> >>>>>> contains a single file it is not that interesting. Here is an
> >>>>>> example of
> >>>>>> what the new webrev looks like for a larger change (the G1 class
> >>>>>> unloading change):
> >>>>>>
> >>>>>> http://cr.openjdk.java.net/~brutisso/CODETOOLS-7901044/webrev.00/example/
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Bengt
> >>>>>>
>