Changes in the FSInternetRadio binding.

[m-memova]

Some changes are made in org.eclipse.smarthome.bindig.fsinternetradio.
They are mainly related to cleaning up some unneccessary checks and
code lines that can never be reached.

Signed-off-by: Mihaela Memova mihaela.memova@musala.com

[maggu2810]

I agree that the framework should call this function only if the thing type is part of the supported ones.
@kaikreuzer If there is only one supported thing type, I think we could rely on the fact that the framework calls this function only if the thing consists of that respective type.
We also trust that the OSGi framwork calls our functions (activate, bind, etc.) with the correct arguments.

[m-memova]

I removed that check from here because when the ThingManager class is trying to register a Handler it calls the findThingHandlerFactory(ThingTypeUID thingTypeUID) method and the registerHandler method is never called if the factory does not support the given thingTypeUID. I tried to test it and that return null; line can never be covered.

[sjsf]

I'm not Kai, but to my knowledge the check should be and is done in the framework consistently. Therefore I don't see an issue removing the check here unless supportsThingType() would return more than one entry.

[kaikreuzer]

Well, the fact that the framework currently only calls it for thing types that were returned on supportedThingTypes might not be a solid fact - at least I didn't find any such assertions in the JavaDocs or in the code of the BaseThingHandlerFactory (where this method is defined and used).
Looking from a pure Java perspective, this is a method of an interface and as the implementor, I should make the implementation robust within the range that the interface and its JavaDoc allows. I therefore would not approve this change. Besides this, it actually also makes it more difficult for future contributors to figure out what needs to be changed if a new supported thing type is added to the binding - with the old code, this is very obvious.

[m-memova]

Thank you, @kaikreuzer ! What do you think about the other changes? Are you OK with them? Should I revert this particular line the way it was before and update my PR? Or just leave everything in the binding the way it is?

[kaikreuzer]

What do you think about the other changes? Are you OK with them?

Yes, absolutely!

Should I revert this particular line the way it was before and update my PR?

Yes, please only revert this single change and we can merge the PR - many thanks :-)

[maggu2810]

Now it fits to the comment 😉

[maggu2810]

It is a public function but a bundle internal one.
So I am fine if you know that every caller already ensures (has this been checked?) that the volume is in range [0;100]
I would prefer to add this to the JavaDoc, too (e.g. @param volume the volume that must be in range [0;100]).

[m-memova]

I removed this check from here because the PercentType itself does it. In the handleCommandmethod we check if the given command is a PercentType, so we are sure that it is a value in the range [0-100] before getting in the setVolumePercentmethod

[maggu2810]

Okay, perhaps the JavaDoc comment would be nice. As you state that a value < 0 || > 100 is not allowed (perhaps some other person change the code some time).

[maggu2810]

Why do you remove the fallback to details.getModelDetails().getModelDescription(); and details.getFriendlyName();?

[m-memova]

Well, it's been a long code journey here :) I hope I can explain it.

1. createResult(RemoteDevice device) returns something different from null only if getThingUID(RemoteDevice device) != null
2. getThingUID(RemoteDevice device) can return something different from null only if getDetails!=null, getModelDetails!=null and getModelNumber!=null.
3. getLabel(RemoteDevice device) is calling the getDisplayString() method which can never return null. It returns either empty or non-empty String. It returns an empty String if getDetails==null (but here we are already sure that it's not true) or if getModelDetails==null && getManifacturerDetails==null) (but here we are already sure that it's not true, either)
4. So, basically the code in the [113-122] code line range can never be reached. I tried to implement tests in order to cover it but I could not do it. getLabel(RemoteDevice device) can never return null, so in my opinion we can use just the getDisplayString() method.

[maggu2810]

Ok, thanks for the explanation.

[maggu2810]

From my side the minor JavaDoc comment is the only one.

Let's wait for @kaikreuzer if he agree with our both oppinion about createThing (that the framework already checks if the type fits).

[m-memova]

Thank you, @maggu2810 !

[m-memova]

There's another thing I would like to discuss before updating my PR and I think it would be appropriate to do it here. Currently, the HTTP client is started before every single request and it's stopped after it. Starting and stopping the client are asynchronous by their nature. So the following situation is possible : one thread is trying to stop the client after a request and another thread is trying to start it again for another request. Currently some of my implemented tests are failing once in 30-40-50 times with [ESH-thingHandler-1] DEBUG o.eclipse.jetty.client.HttpReceiver - Response failure HttpResponse[null 0 null]@78e6f075 java.nio.channels.AsynchronousCloseException. In my opinion the reason for that error is exactly the HTTP client's current behaviour. Has anyone ever have a similar problem?

[kaikreuzer]

the HTTP client is started before every single request and it's stopped after it

Good catch. The HTTP requests were last refactored by @svilenvul and the startHttpClient actually checks whether it is already started and does not try to start again, if it is already started.
What I missed when reviewing it is that after a request, it is indeed always stopped again - I would consider this a bug that indeed needs to be fixed.

[m-memova]

@kaikreuzer , I think I found a way to fix it. I moved the stopHtttpClient() method into the bundles's deactivate() method and everything seems to be working fine on my machine.

[kaikreuzer]

looks good, many thanks!