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
