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

Wed Oct 1 21:38:01 UTC 2014

On 10/1/14, 5:24 PM, Bengt Rutisson wrote:
>
> 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 :)

That's ok.   The next person can enhance that.   Thanks for adding the 
navigation.

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