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

[Semyon Sadetsky]

Hi Like,

You suggested the next change for the spec:

-     * @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.

In my opinion highlighting legacy implementation here is not justified. 
Such a note is more suitable for the release notes.

I would change the spec to

+    * @return the <code>ButtonGroup</code> that the button belongs to 
or null otherwise

Because if the getter returns null the button doesn't actually belong to 
any group, and this is true for both legacy and after implementations.
But the current spec looks ok to me as well.

--Semyon

On 06/30/2017 02:52 AM, Luke wrote:
> 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.

You suggested the next change for the spec:

-     * @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.

In my opinion highlighting legacy implementation here is not justified. 
Such a note is more suitable for the release notes.

Instead I would change the spec to

+    * @return the <code>ButtonGroup</code> that the button belongs to 
or null otherwise

Because if the getter returns null the button doesn't actually belong to 
any group, and this is true for both legacy and after implementations.
But the current spec looks ok to me as well.

What do you mean under standard set/get behavior? Do you mean that 
getter should always return the value provided with the previous setter 
call?

>
> 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.
It doesn't seem mandatory, since the caller should be aware about the 
method to call it.
>
> 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?
I don't see a method caller mentioned in the @implSpec description. I 
think this is more related to class usage.

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