Refactor Kotlin code to improve error handling

[krocard]

Problem (PRN-74 PRN-92)

The codebase had many issues:

• invalid null parameters were ignored instead of reporting an error
• invalid state (module not loaded) were ignored instead of reporting an error
• many functions were taking unnecessary null parameters, returning null unnecessarily
• errors were not reported
• some promise were not resolve (deadlock if awaited)
• exception do not reject promisses

Changes

Simplify the code and improve error visibility by:

• Errors are bubbled up to the caller with @ReactNative's promise.reject instead of ignoring them or logging them.
• Refactor json serialization to use null operator ?. rather than operating on null values.
• Make toJson extension function to easily chain ?. operations
• Make Json.toXXX extension function to easily chain ?. operations
• Add XXXModule variant that throws an exception if the module are not loaded, which should never happen
• Wrap all @ReactNative methods in a catch to report any exception instead of crashing the app
• Introduce a base Bitmovin module for helper functions
• Wrap promise in a strongly typed interface
• All @ReactCommand promisses are guarantied to be resolved or rejected
• Exception in the PlayerViewManager are logged instead of crashing the app

📚 Other PRs for this issue

• 📖 Refactor Kotlin code to improve error handling #317
• 📘 Exception are now always caught, remove compile time check #345
• 📙 Align PR handling of default values #346
• 📒 Simplify getters by always throwing #347
• 📜 Reject invalid command from React Native #343

Checklist

• 🗒 CHANGELOG entry if applicable

[krocard]

Make sure Hide whitespace is ticked to review this file.

[krocard]

The @param and @return documentation was only describing the type and name of the parameter, which is against our style guide. They have thus been removed.

[matamegger]

Very nice improvements!

I think we need some more work on the uiManager/Promise thing, but even that is already a good starting point!

[matamegger]

Neat that you found a solution for the Promises. This makes future review and coding a lot easier 👍

As discussed, there are still some errors, but apparently the fault is on the Arguments.fromList.
Also, I am not sure if the nullability is right on all the promises. I think we have to align with the type that is defined in the TS API.

[krocard]

The alternative is to default in the Kotlin code on null.
Chose to default in the TS code aligned with isFullscreenRequest.

[zigavehovec]

I guess this could change the behaviour a bit, E.g. on iOS we don't call anything on the playerView if nothing is set:

switch scalingMode {
case "Zoom":
    playerView.scalingMode = .zoom
case "Stretch":
    playerView.scalingMode = .stretch
case "Fit":
    playerView.scalingMode = .fit
default:
    break
}

I think I would prefer not to change this, at last within this PR.

[krocard]

I don't think ignoring the command on the native side if it's invalid is the right approach (and it goes against the PR goal of reporting invalid states).
The alternative is to avoid sending the command if no scaling mode is requested: d77c955

[zigavehovec]

As discussed - since using a default value makes a call on playerView that didn't happen before, emitting an additional event (ScalingModeChangedEvent (_DefaultScalingModeService.swift:38)) let's use the alternative approach 🙂

We can improve on this in a follow up PR.

[zigavehovec]

Awesome improvements 💪
I noticed some things that I think we should address, but looking very good already!

[zigavehovec]

I guess this could change the behaviour a bit, E.g. on iOS we don't call anything on the playerView if nothing is set:

switch scalingMode {
case "Zoom":
    playerView.scalingMode = .zoom
case "Stretch":
    playerView.scalingMode = .stretch
case "Fit":
    playerView.scalingMode = .fit
default:
    break
}

I think I would prefer not to change this, at last within this PR.

[matamegger]

Nothing to add to @zigavehovec s comments.
After those topics are resolved this is fine to be merged.

[zigavehovec]

Nice! Let's get this merged 💪