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

[Joe Darcy]

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