[homematic] Reduce SAT warnings

[lsiepel]

This should reduce the SAT warnings the binding generates. While doing so i tried not to change the behaviour. This is also the reason that i could not fix all compile warnings.
To do so another refactoring pass is needed.

While working on it, some rare bugs might be fixed in the proces.

A JAR file to do some testing is here: (5.2.x) https://1drv.ms/u/c/70ea7ac46bc61c73/IQBH2Mogs5aaRLmsWgPpfpHFAfwgIZIBPg_emuVVgjHsz30?e=nbHpKc

[lsiepel]

Also tagging @MHerbst and @cmorty

[maniac103]

Damn, now that I see this, it reminds me I started similar work and completely forgot about it :-/ - see this branch. I wonder whether it would make sense to compare the approaches and see what can be incorporated here? On a first glance it looks like I kept a bit more stuff non-null, e.g. in HmChannel.

[lsiepel]

It did cost me a full day :-/
My assumption was that whenever there was a null check, i was going to respect it to prevent regressions. So this caused many fields to remain nullable. Let me see if there is an easy way to compare both branches.

[maniac103]

It did cost me a full day :-/

Same :-( I finished it to the state where it compiled, didn't have any time for testing at that point, thus pushed to a branch and forgot said branch because I was busy with other things.

Edit: I'll try to prepare a diff.

[maniac103]

So here's the diff between our branches (from yours to mine).

Unfortunately, it's huge, mainly because we made some different choices about nullability in the base classes of the parsers and the model classes. For the latter I'm somewhat certain mine are a bit more correct (my approach also makes some of their fields immutable, which helps a lot in figuring out nullability); for everything else I'm open for discussions.

Again, sorry for not making you aware of my changes so now either yours or mine is wasted - I genuinely forgot to create a draft PR for this 😞

[lsiepel]

Let's try to reduce the diff and see what actual issues remain. I made a start, but have to leave now and will continue later. I post my findings here, and will push changes while working on it, these changes contain everything i bluntly copied from your gist. I assume that is ok. WIP

-                        loadChannelValues(device.getChannel(HmChannel.CHANNEL_NUMBER_VARIABLE));
-                        loadChannelValues(device.getChannel(HmChannel.CHANNEL_NUMBER_SCRIPT));
+                        HmChannel varChannel = device.getChannel(HmChannel.CHANNEL_NUMBER_VARIABLE);
+                        if (varChannel != null) {
+                            loadChannelValues(varChannel);
+                        }
+                        HmChannel scriptChannel = device.getChannel(HmChannel.CHANNEL_NUMBER_SCRIPT);
+                        if (scriptChannel != null) {
+                            loadChannelValues(scriptChannel);
+                        }

Originally, getChannel() was used without null checks. If it commonly returned null, I’d expect more issues to have surfaced.
To clarify the contract, I introduced getChannelUnchecked() to preserve the original nullable behavior, and changed getChannel() to throw if the channel is missing.
Only call sites that already performed a null check were switched to getChannelUnchecked(), assuming those checks were intentional, if not they can also be changed to getChannel() with the null check removed.
I think this greatly reduces the diff.

===

-    public @Nullable String getCallbackHost() {
+    @Nullable
+    public String getCallbackHost() {

Semantically the same, the inline version is imo a bit cleaner.

===

Moved the xstream dto's to a subfolder to prevent them from having SAT issues.

[maniac103]

Only call sites that already performed a null check were switched to getChannelUnchecked(), assuming those checks were intentional, if not they can also be changed to getChannel() with the null check removed.

I definitely do not like having 2 methods here, because the differentiation between both is unclear (before looking it up in the source). I opted for making the method nullable for two reasons:

• semantically the nullable return value makes sense, since it's absolutely possible to ask the model for a non-existing channel (even if we usually don't do that - I'm just judging about the API of the model class here)
• it's still easy to enforce the existence of a channel (by just wrapping the method call in Objects.requireNonNull())

In addition I view the previous behavior as inconsistent: some code places (4 to be precise) assume channel 0 to be present, others (1 to be precise) check if it's present. (In reality it will always be present for a proper device) I think it's better to add a few useless null checks than to preserve this inconsistency.

Semantically the same, the inline version is imo a bit cleaner.

Yeah, the difference here is irrelevant, it's fine either way.

Moved the xstream dto's to a subfolder to prevent them from having SAT issues.

Maybe OK, but I think we should still keep null annotations correct where possible (e.g. in TclScriptList.getScripts()). Any strong reason not to just make them @NonNullByDefault? AFAICT they're only used to read XML shipped with the binding.

these changes contain everything i bluntly copied from your gist. I assume that is ok.

Sure 🙂

[lsiepel]

Would it be possible to mass replace your null annotations to be inline, and regenerate the diff, That would remove all clutter and i have adopted many parts of your code so the diff should be much less now.

[maniac103]

Would it be possible to mass replace your null annotations to be inline, and regenerate the diff,

I regenerated the diff and removed all fluff (Nullable annotation placement, imports etc.) manually. Result is here.