<Swing Dev> [10] RFR JDK-8191639:NPE from BasicListUI.Actions.getNextPageIndex

[Sergey Bylokhov]

Looks fine.
Two small issues should be fixed before the push.
  - Typo in the summary "Verifies no NPE is thrown wjen pageup/down is 
pressed in a JList"
  - "@headful" should be "* @key headful"

On 29/11/2017 01:04, Prasanta Sadhukhan wrote:
> OK. http://cr.openjdk.java.net/~psadhukhan/8191639/webrev.03/
> 
> Regards
> Prasanta
> 
> 
> On 11/29/2017 12:35 AM, Sergey Bylokhov wrote:
>> On 27/11/2017 23:45, Prasanta Sadhukhan wrote:
>>> Ok. I have added the check for scrollRectToVisible() too.
>>>
>>> http://cr.openjdk.java.net/~psadhukhan/8191639/webrev.02
>>
>> Should the test use some specific L&F? Currently it is pass on macOS 
>> and Aqua L&F before the fix.
>>
>>>
>>> Regards
>>> Prasanta
>>> On 11/28/2017 1:01 PM, Sergey Bylokhov wrote:
>>>> On 27/11/2017 22:40, Prasanta Sadhukhan wrote:
>>>>> On 11/28/2017 3:33 AM, Sergey Bylokhov wrote:
>>>>>> Hi, Prasanta.
>>>>>> A few notes about the fix:
>>>>>>  - Can we get an NPE in adjustScrollPositionIfNecessary at
>>>>>>        2391 list.scrollRectToVisible(cellBounds); Not sure that 
>>>>>> scrollRectToVisible is ready for null.
>>>>> cellBounds is not null for this call due to this check in
>>>>>
>>>>> 2308             if (cellBounds != null && 
>>>>> !visRect.contains(cellBounds)) {
>>>>
>>>> In the fix you added a check at line
>>>> 2384 if (cellBounds != null) {
>>>>
>>>> And later you will pass the cellBounds as a parameter:
>>>> 2393                 list.scrollRectToVisible(cellBounds);
>>>>
>>>> So either cellBounds can be null or the new check is not needed.
>>>>
>>>>
>>>>>
>>>>>>  - I think the assignment below can be simplified
>>>>>>        cellBounds = startRect != null ? startRect : null;
>>>>>>  - In the getNextPageIndex() probably we can "return index" 
>>>>>> immediately instead of additional indentation?
>>>>>>
>>>>> Done. Modified webrev
>>>>> http://cr.openjdk.java.net/~psadhukhan/8191639/webrev.01/
>>>>>
>>>>> Regards
>>>>> Prasanta
>>>>>> On 22/11/2017 03:05, Prasanta Sadhukhan wrote:
>>>>>>> Hi All,
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8191639
>>>>>>>
>>>>>>> It is seen that when JList.locationToIndex() or getCellBounds() 
>>>>>>> is overridden to return -1 or null respectively,
>>>>>>> it causes NPE when PageUp/PageDown is pressed in JList.
>>>>>>>
>>>>>>>  From the spec 
>>>>>>> [https://docs.oracle.com/javase/9/docs/api/javax/swing/JList.html#getCellBounds-int-int-] 
>>>>>>>
>>>>>>> of getCellBounds(), it is seen that it can return null under some 
>>>>>>> circumstances. But, JList jdk code assumes it is always non-null.
>>>>>>>
>>>>>>> Proposed fix is to check for null return value of getCellBounds() 
>>>>>>> and bail out.
>>>>>>> webrev: http://cr.openjdk.java.net/~psadhukhan/8191639/webrev.00/
>>>>>>>
>>>>>>> Regards
>>>>>>> Prasanta
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
> 


-- 
Best regards, Sergey.