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

[Semyon Sadetsky]

+1

--Semyon


On 12/05/2017 04:51 AM, Prasanta Sadhukhan wrote:
> Hi Semyon,
>
> Ok. added index check
> and followed previous review comment of returning index immediately 
> instead of additional indentation
> http://cr.openjdk.java.net/~psadhukhan/8191639/webrev.04/
>
> Regards
> Prasanta
> On 12/4/2017 9:39 PM, Semyon Sadetsky wrote:
>> Hi Prasanta,
>>
>> I'm not suggesting to remove the null check. I suggested to add check 
>> for -1 since the bounds of the cell with index -1 should never exists 
>> and it doesn't make scene to call the method to calculate its bounds.
>>
>> --Semyon
>>
>>
>> On 12/03/2017 10:33 PM, Prasanta Sadhukhan wrote:
>>> HI Semyon,
>>>
>>> I guess app can also override getCellBounds and return null (maybe 
>>> mistakenly) so we need to check for return null.
>>>
>>> Regards
>>> Prasanta
>>> On 11/29/2017 10:39 PM, Semyon Sadetsky wrote:
>>>> Hi Prasanta,
>>>>
>>>> I suggest to call list.getCellBounds() only if index != -1 since 
>>>> the result will be always null in that case.
>>>>
>>>> --Semyon
>>>>
>>>>
>>>> On 11/29/2017 01:04 AM, 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>