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 <[email protected]> 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 <[email protected]> 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 >
