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

[Bengt Rutisson]

Coleen,

On 10/1/14 5:52 PM, Coleen Phillimore wrote:
>
> Bengt,
>
> Thanks for doing this.  I filed an issue for this a long time ago, 
> that you can close as a duplicate.
> https://bugs.openjdk.java.net/browse/CODETOOLS-7900314

Thanks for pointing that out. Didn't know about that issue. Should 
probably have used that instead of filing a new one. I closed 
CODETOOLS-7900314 as a duplicate.

>
> Does it work for added and deleted files?

Probably not the way you would like :)

It works in the sense that the webrev will be correctly generated, but I 
haven't added next/prev links to the "new" and "old" views. And it 
wasn't really clear to me what the "next" link should do once you pass 
the last diff view. Should you jump to the "new" view even though you 
had selected to view, say udiff?

Instead of trying to solve that I opted to just have next/prev links for 
the diff views (cdiff, udiff, sdiff and frames). These all come first in 
the file list for normally generated webrevs. So, when there are only 
added and removed files left the next button takes you back to the index 
page.

Hopefully this solves the most common use cases. And for the others - 
webrevs with lots of new or deleted files -  my change does not help but 
also does not make things worse than before.

Thanks,
Bengt

>
> Coleen
>
> On 10/1/14, 10:27 AM, Daniel Fuchs wrote:
>> Hi Bengt,
>>
>> The new version looks good to me, but I am not a reviewer
>> fore CODE_TOOLS. Hopefully someone who is will step in :-)
>>
>> 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
>>>>>
>>>>
>>>
>>
>