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

[Bengt Rutisson]

On 2014-10-02 23:39, John Coomes wrote:
> 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.

Thanks for pushing this, John!

Bengt

>
> -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
>>>>>>>>