[hdpowerview] Fix SAT warnings

[jlaur]

Signed-off-by: Jacob Laursen [email protected]

Fixes #12027

Changes done:

1. Fix SAT warnings about missing @NonNullByDefault annotations on classes.

Next steps (in context of new PR):

1. Refactor error handling: Throw exception if contract is breached.
2. Consider separating deserialization classes so they are not exposed directly to handlers (with @Nullable when contract doesn't allow this). Or find other way to make sure handlers are not bloated with null handling which shouldn't be needed for valid responses.

[andrewfg]

I will look at you code tomorrow, but here is some quick feedback..

Personally I would not bother with your proposal 3. above, since what you call “bloat” is actually “good practice” (against the possibility of some device in future not honouring the contract).

In addition, while you are refactoring your classes, I would also refactor the Shade fields position0 and positionKind0 from int to @nullable Integer, and add the respective null checks on those, at the next higher level. There may be other similar cases too..

[jlaur]

I will look at you code tomorrow, but here is some quick feedback..

Thanks, no rush. This is so far just some initial steps. Will probably await #11853 and #12002 before doing anything major, since I'll be changing all methods in HDPowerViewWebTargets as well as all callers, so will collide with that work. Hopefully they can be merged soon. I was even just giving getShadeSurvey() same treatment as getFirmwareVersions(), but realized that I was on collision course on caller side with one of those PR's. Conflicts are part of life, but I would rather avoid having to do everything all over if I can.

Personally I would not bother with your proposal 3. above, since what you call “bloat” is actually “good practice” (against the possibility of some device in future not honouring the contract).

One of the problems, as I see it, is that if something is actually required in a response for the response to make sense at all, this should be verified and caller shouldn't have to deal with this, but just provide generic error handling, i.e. catch some exception.

Example: For the firmware response to make sense, mainProcessor is required. radio, on the other hand, is optional as it was added in Hub v2, apparently.

So let's say we did this validation in getFirmwareVersions:

if (firmwareVersions.mainProcessor == null) {
    throw new JsonParseException("Missing 'mainProcessor' element");
}

Unfortunately this wouldn't help. Caller would still have to perform the exact same check, since everything is by definition @Nullable:

@Nullable
public Firmware mainProcessor;

That's why I was thinking about possibly introducing a class with stricter annotation adhering to a contract instead of just returning objects of FirmwareVersions where everything is nullable because of reflection/gson deserialization.

This would introduce some overhead with maintaining additional model classes, but on the other hand it would simplify the handlers/logic because they wouldn't have to check for nulls everywhere only because of undesired nullness annotation from deserialization classes. That's what I consider bloat, or at least cluttering. Logic is harder to read IMHO when entangled with checks needed because of nullness annotations.

Maybe there are some JSON deserialization alternatives that supports annotations and/or have runtime checks. I'll look into other bindings also as this must be a common challenge. :-)

In addition, while you are refactoring your classes, I would also refactor the Shade fields position0 and positionKind0 from int to @nullable Integer, and add the respective null checks on those, at the next higher level. There may be other similar cases too..

Sure, plan would be to refactor all of them when having found a reasonable solution. The concrete example you are mentioning would have the same problem as described above. Since they would be nullable, we would have to provide duplicate checks to avoid warnings, I think. :-(

[andrewfg]

The OH developers / maintainers decided on GSON as the standard JSON (de)serialization librarary. And this library does not (yet) support Required or Optional annotations (google/gson#61) so it seems to me that this discussion about replacing or overwriting the standard OH library should not be specific to this binding, but rather should be addressed to OH developers / maintainers for the whole OH eco-system.

[lolodomo]

LGTM

[lolodomo]

Is it ready for a merge? In this case, please remove draft/WIP?

[andrewfg]

Is it ready for a merge? In this case, please remove draft/WIP?

@lolodomo this PR is still a draft, and not ready to merge; you are probably confusing it with #11853 which is indeed ready.

[jlaur]

Is it ready for a merge? In this case, please remove draft/WIP?

No, it's only getting started, but now a little bit stuck because of these other PR's that would need to go first to avoid major conflicts and rework:

• [hdpowerview] Refactor dynamic channels #11853
• [hdpowerview] Add support for calibrating a shade #12002

[lolodomo]

No, I am not confusing it with another PR, I reviewed the code for this PR and it looks good to me.
Let me know when this is finished.

[andrewfg]

No, I am not confusing it with another PR, I reviewed the code for this PR and it looks good to me. Let me know when this is finished.

@lolodomo, IMHO this is far from finished, so perhaps your "LGTM" is premature (??) - please look at my comment a few posts above this #12032 (comment)

[lolodomo]

If you can have several small PRs rather one very big PR, that is really better for reviewing.

[jlaur]

If you can have several small PRs rather one very big PR, that is really better for reviewing.

Sure, I'll split it. This one can be merged, I'll update the title and description.