<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:51 PM, Petr Pchelko wrote:
> 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.

Upon further reflection, I'd prefer the change to using a .equals based 
comparison being done under a different changeset. Doing so will make it 
easier to isolate any problems which may arise strictly from 
generification and warning removal.

>
>> (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.

Comment expanded.

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

Update webrev:

     http://cr.openjdk.java.net/~darcy/8043550.1/

Thanks,

-Joe

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