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

[Semyon Sadetsky]

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