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
