On Sun, 17 Jul 2022 14:11:22 GMT, Nir Lisker <nlis...@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

Reply via email to