RFR: 8289394: Fix warnings: Unlikely argument type

[Kevin Rushforth]

On Sun, 17 Jul 2022 14:11:22 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> Fixes "Unlikely argument type" warning generated by the latest Eclipse IDE.
>> 
>> This warning should be reclassified as an error, as it catches bugs missed by javac.  In this case, the following places seem to contain bugs:
>> - apps/samples/3DViewer/src/main/java/com/javafx/experiments/jfx3dviewer/TimelineController.java
>> - modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/GetEvent.java
>> 
>> The fixes include:
>> - using objects of the right type
>> - adding @SuppressWarnings("unlikely-arg-type") in places where intended (e.g. assertFalse)
>> 
>> There was an earlier discussion about using IDE-specific @SuppressWarnings in the code.  While I might disagree (I think it is ok to use IDE-specific @SuppressWarnings), the tests can be reworked to avoid @SuppressWarnings.  Please let me know.  Thanks!
>
> apps/samples/3DViewer/src/main/java/com/javafx/experiments/jfx3dviewer/TimelineController.java line 79:
> 
>> 77:                 loopBtn.setDisable(false);
>> 78:                 playBtn.setSelected(t.getCurrentRate() != 0);
>> 79:                 loopBtn.setSelected(t.getCycleDuration().equals(Duration.INDEFINITE));
> 
> The correct fix is to check `t.getCycleCount() == Timeline.INDEFINITE`.

Yes, given what the button does, it should be testing cycleCount not cycleDuration.

> modules/javafx.base/src/test/java/test/javafx/collections/ObservableSubListTest.java line 161:
> 
>> 159:     @Test
>> 160:     public void testEqualsOnAnotherType() {
>> 161:         assertFalse(sublist.equals(Integer.valueOf(7)));
> 
> I'm not sure I see the point of this test. It's testing the implementation of the backing list, not the behavior of the sublist. Same for some other methods in this test, though it's not related to this PR. Let's see what Kevin thinks here.

It might be worth a separate bug to look at this.

> modules/javafx.graphics/src/test/java/test/javafx/css/Node_cssStyleMap_Test.java line 83:
> 
>> 81:     private void checkFoundStyle(Property<?> property, Map<StyleableProperty<?>, List<Style>> map, List<Declaration> decls) {
>> 82: 
>> 83:         List<Style> styles = map.get((StyleableProperty<?>)property);
> 
> If the `Property` must be a `StyleableProperty` then maybe the method signature should be that and the cast should be the responsibility of the callers.

Since this is in a unit test, I don't have a strong opinion about this. Also, since this is in a test, an `assertTrue(property instanceof StyleableProperty)` might not be a bad idea.

> modules/javafx.graphics/src/test/java/test/javafx/css/Node_cssStyleMap_Test.java line 300:
> 
>> 298:         return false;
>> 299:     }
>> 300: 
> 
> I don't mind removing this method since it's unused, but probably as part of a "remove unused methods" PR.

In general, I agree with this as a best practice. In this case, the unused method would need to be fixed up, so I can also see the argument for removing it as part of eliminating this warning. I don't mind either way.

-------------

PR: https://git.openjdk.org/jfx/pull/823