<Swing Dev> JDK 9 RFR of JDK-8043550 : Fix raw and unchecked lint warnings in javax.swing.*

[Petr Pchelko]

Hello, Joe.

> (Is is correct that this code is using "==" rather than ".equals" to compare strings? If .equals were correct, I would change this method to use strings in switch.)
Interesting question. The history of this code is lost 10 years ago, so I don’t know why == is used here instead of equals. 
Just looking at the code I do not see any real reason to use ==, so I would say it’s a bug. So I would say we can replace it with switch.

> (This degree of explanation struck me as overkill for a comment, but I can work on adding something along these lines if you think it would be helpful to future readers of the code.)
Thank you for the detailed explanation, could you please make the comment slightly more explanatory? I think that adding the info that Number subclasses are not comparable with each other as Numbers would be enough.

> The changes in javax.swing.tree and javax.swing.undo are new in this request.
I’ve looked at these too, no new comments about them.

Thank you.
With best regards. Petr.

> On Jul 1, 2014, at 8:22 PM, Joe Darcy <joe.darcy at oracle.com> wrote:
> 
> Hi Petr,
> 
> On 07/01/2014 01:17 AM, Petr Pchelko wrote:
>> Hello, Joe.
>> 
>> It's hard to understand what to review and what not to review. Could you please generate a separate webrev for the changes that needs to be reviewed next time?
>> I've looked only to the javax.swing package, so I'm not sure I've reviewed everything that needed to be reviewed.
>> 
>> 1. JComboBox: 1983 - no need to suppress warning
> 
> Unnecessary suppression removed.
> 
>> 2. JComponent: 4121 - the change is incorrect. Value can be not an instance of Set. It depends on the property name. See line 4124 where it's cast to Boolean
> 
> Noted; I've changed this to have a declaration of strokeSet in each of the if block where the unchecked cast used to occur.
> 
> (Is is correct that this code is using "==" rather than ".equals" to compare strings? If .equals were correct, I would change this method to use strings in switch.)
> 
>> 3. SpinnerNumberModel: 90 - There's a typo in the comment with braces. Also, could you please clarify what do you mean under "awkward to use"?
> 
> Typo fixed.
> 
> The actual Comparable mins and max people are likely to pass in are values of type java.lang.Double, java.lang.Integer, etc. Such types are correctly declared as Comparable to themselves:
> 
>    public final class Double extends Number implements Comparable<Double> {...}
> 
>    public final class Integer extends Number implements Comparable<Integer> {...}
> 
> This means that in the class for Double, there are two compareTo methods, one that takes a Double argument and one that takes an Object argument; the latter is the bridge method for the former. Of note is that there is *not* a compareTo method taking a Number as an argument.
> 
> The Number type is a bit misnamed as an abstraction; it just means "convertible to primitive." Therefore, two arbitrary Number types are not mutually Comparable so it is sensible that Double doesn't have a compareTo bridge method for Number.
> 
> If the minimum and maximum values were declared as Comparable<? extends Number> with the aim of comparing two Number instances with them, in practice that would mean calling a compareTo(Number) method which doesn't exist. In that sense, it would be awkward to use Comparable<? extends Number> ;-)
> 
> (This degree of explanation struck me as overkill for a comment, but I can work on adding something along these lines if you think it would be helpful to future readers of the code.)
> 
> The changes in javax.swing.tree and javax.swing.undo are new in this request.
> 
> Thanks,
> 
> -Joe
> 
>> 
>> With best regards. Petr.
>> 
>> On 30 июня 2014 г., at 20:20, Joe Darcy <joe.darcy at oracle.com> wrote:
>> 
>>> Hello,
>>> 
>>> Please review the final batch of changes to resolve all the unchecked and rawtypes warnings in swing:
>>> 
>>>    JDK-8043550 : Fix raw and unchecked lint warnings in javax.swing.*
>>>    http://cr.openjdk.java.net/~darcy/8043550.0/
>>> 
>>> For completeness, this webrev also includes the in-progress changes for
>>> 
>>>    8043548: Fix raw and unchecked lint warnings in javax.swing.plaf.*
>>>    8042849: Fix raw and unchecked warnings in com.sun.java.swing
>>> 
>>> For 8043548, there is still some review work going on to sort out some of the details; 8042849 has been approved, but not pushed yet as it could conceivably need updating based on other changes in swing.
>>> 
>>> It was not immediately clear how javax.swing.tree.TreeNode.children(), which returns a raw Enumeration, should be generified. I changed it to
>>> 
>>>    Enumeration<TreeNode> children();
>>> 
>>> and that seems to work fine. Something like
>>> 
>>>    Enumeration<? extends TreeNode>
>>> 
>>> with a covariant override would save a few casts in a private implementation, but generally doesn't seem to be a good trade-off since many normal clients could be inconvenienced dealing with the wildcard.
>>> 
>>> How to best generify the field javax.swing.KeyboardManager.containerMap was not immediately clear either. The documentation implies the nested Hashtable could be something like a Hashtable<KeyStroke, Object>
>>> 
>>>  68     /**
>>>  69       * maps top-level containers to a sub-hashtable full of keystrokes
>>>  70       */
>>>  71     Hashtable<Container, Hashtable<Object, Object>> containerMap = new Hashtable<>();
>>> 
>>> but that doesn't work out throughout the entire file since there is code that add entries using a Class object as the key. In the end, I just used the not-very-precise Hashtable<Object, Object>.
>>> 
>>> Thanks,
>>> 
>>> -Joe
>