Hi Alexey, I’m happy to add more small fixes at a later point in time. Please understand that I’m not a Swing expert. But I could help to make the code look cleaner and more like modern Java.
Also I think the example code is used in some regression tests, right? So I need to make sure not to break those. Is there documentation or any pointers how to run those tests? Regards, -marc > On 11. Mar 2020, at 21:07, Alexey Ivanov <alexey.iva...@oracle.com> wrote: > > Thank you to Marc for updating webrev and to Sergey for uploading it. > > The changes look fine to me as I already stated. > > I just wanted to share more comments: > > On 22/02/2020 09:50, Sergey Bylokhov wrote: >> Thank you, an updated version is upload: >> http://cr.openjdk.java.net/~serb/8237746/webrev.01 >> >> On 2/17/20 11:55 am, Marc Hoffmann wrote: >>> Thanks Alexey for the detailed review! I attached a updated version. >>> >>> The examples have many cleanup opportunities. I wanted to focus on compiler >>> warnings for now and keep the changeset minimal. > > Yes, I agree. It's better to fix one problem at a time. > >>> <SNIP> >>> >>>> *ButtonDemo.java* >>>> 64 Vector<Component> buttons = new Vector<>(); >>>> Shall it be JComponent? >>> >>> Doesn’t because JPanel.add() returns Component: >>> >>> buttons.add(p2.add(new JButton(getString("ButtonDemo.button1")))); >>> >>> Should I introduce a local variable? > > It could be another opportunity to contribute and clean up. > > Vector can be replaced with ArrayList; and Component with AbstractButton, > then the type casts and instanceof checks in listeners can be cleaned up too. > >>>> *ComboBoxDemo.java* >>>> 60 JComboBox<?> hairCB; >>>> Why not JComboBox<String> ? >>>> All createXXX methods use this type. >>>> Then the cast below would be unnecessary: >>>> 282 String name = (String) parts.get(hairCB.getSelectedItem()); >>> >>> This comes from the lookup in the parts Hashtable. Unfortunately it has >>> String an ImageIcon values. > > Probably this could also be made cleaner then… Or maybe it's not worth it. > >>> >>>> >>>> >>>> 114 presetCB = (JComboBox<?>) >>>> comboBoxPanel.add(createPresetComboBox()); >>>> To avoid cast, you can use two statements: >>>> presetCB = createPresetComboBox(); >>>> comboBoxPanel.add(presetCB); >>> >>> Done for all 4 occurrences. >>> >>>> >>>> >>>> *DirectionPanel.java* >>>> 97 AbstractButton b = e.nextElement(); >>>> 98 if( b.getActionCommand().equals(selection) ) { >>>> Indentation on line 97 seems incorrect, it should align to line 98, >>>> shouldn't it? >>> >>> Done (replace tab with spaces). >>> >>>> >>>> >>>> *SliderDemo.java* >>>> 167 @SuppressWarnings("unchecked") >>>> 168 Dictionary<Object, Object> labelTable = >>>> s.getLabelTable(); >>>> Would using Dictionary<?, ?> suppress the warning automatically? >>>> I mean that @SuppressWarning would become unnecessary. >>> >>> Dictionary<?,?> does not allow put of specific types in the next line. But >>> fixed tabs in the same line. > > I see, it comes from JSlider.get/setLabelTable which use the raw type > Dictionary. > So probably the API of JSlider itself can be updated to accept > Dictionary<Integer, ? extends JComponent>. > > The generification of these two methods of JSlider was reverted to raw types > under https://bugs.openjdk.java.net/browse/JDK-8055254 because SwingSet2 did > not compile. So it should be a coordinated change to both the API and the > demo. > >>>> <SNIP> > -- > Regards, > Alexey
