[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] <Swing Dev> [15] Review Request: 8237746 Fixing compiler warnings in src/demo/s
From:       Marc Hoffmann <hoffmann () mountainminds ! com>
Date:       2020-03-11 20:22:32
Message-ID: 20D3C5AC-C49C-4201-9326-F3EFB8AFB481 () mountainminds ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]