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

[Petr Pchelko]

Hello, Joe.

The fix looks good.

>>> (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.
Ok, I was thinking about the same. Could you please file a P4 bug against swing? We will handle it further. Thank you.

With best regards. Petr.

> On Jul 3, 2014, at 7:22 PM, Joe Darcy <joe.darcy at oracle.com> wrote:
> 
> 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
>