<Swing Dev> [10] RFR: 8182577: Crash when Tab key moves focus to a JCheckbox with a custom ButtonModel

[Luke]

Hi Semyon,

On 29/06/2017 17:16, Semyon Sadetsky wrote:
> Hi Luke,
>
> I think a newly created method that accepts the ButtonModel instance 
> argument and calls setGroup/getGroup on it should always check the 
> getGroup() return for null before using it just from general 
> consideration because the real implementation is unknown.

I agree, callers should & hopefully will.

> Your suggestion to highlight the legacy implementation as a possible 
> source for NPE seems unreasonable in that it suggests an assumption 
> that after ButtonModel implementations cannot return null from 
> getGroup(), which is not true.

I'm not sure I follow this sentence. My Javadoc suggestion was aiming to 
make the *two* situations in which a null is returned more explicit, 
since null has two meanings now. Since it does have real implications 
for the caller (don't assume standard set/get behaviour) I felt it 
should be mentioned in the API Specification[1] section of the Javadoc 
to draw attention to it.

Generalising to a rule: a default method pulled-up with a stub 
implementation will always _broaden_ the original contract. Even if all 
known sub-classes _refine_ it back to the original contract there is now 
a possibility of unknown sub-classes, which callers will now need to 
take into consideration.

I'm not sure what "standard practice" is for documenting such corner 
cases arising from interface evolution. Does @implSpec form part of the 
API specification for callers too?

Anyway, I feel I'm nit-picking, since (for this specific interface) it 
is very unlikely to cause real-world bugs. Don't take this email as an 
objection to proceeding "as it is", I just wanted to clarify any points 
that might have been misunderstood.

-- Luke

[1]  http://openjdk.java.net/jeps/8068562


> On 06/29/2017 05:50 AM, Luke wrote:
>
>> Hi Semyon,
>>
>> On 29/06/2017 02:51, Semyon Sadetsky wrote:
>>> Hello Luke,
>>>
>>> DefaultButtonModel ::getGroup has never been being required to 
>>> return not-null. Can you clarify which new pitfall is introduced by 
>>> the fix, so that we should specify it explicitly?
>>>
>>> --Semyon
>>
>> Code using a ButtonModel might assume that after calling setGroup 
>> that the same instance would thereafter be returned by getGroup. 
>> However any code taking advantage of getGroup having being "pulled 
>> up" into ButtonModel should not make that natural assumption, as a 
>> legacy ButtonModel may still return null.
>>
>> So this forms part of the method's contract for the caller too. If 
>> the caller is expected to read and understand the implications of the 
>> implSpec, then what is written already may be sufficient. I guess 
>> that's the question.
>>
>> How about these changes (based on webrev.05)?
>>
>>      /**
>>       * Returns the group that the button belongs to.
>>       * Normally used with radio buttons, which are mutually
>>       * exclusive within their group.
>>       *
>>       * @implSpec The default implementation of this method returns 
>> {@code null}.
>> -     * Subclasses should return the group set by setGroup() to avoid 
>> NPE.
>> +     * Subclasses should return the group set by setGroup().
>>       *
>> -     * @return the <code>ButtonGroup</code> that the button belongs to
>> +     * @return the <code>ButtonGroup</code> that the button belongs 
>> to, if any.
>> +     * Null may also be returned where a legacy ButtonGroup inherits 
>> the
>> +     * default implementation.
>>       * @since 10
>>       */
>>      default ButtonGroup getGroup() {
>>          return null;
>>      }
>>
>> I think this makes the contract more explicit.
>>
>> Kind regards,
>> Luke