[Compile Warnings As Errors] Resolve Warnings & Enable All Warnings as Errors

[ParaskP7]

This PR resolves/suppresses all warnings for all modules and then enables all warnings as errors as the default for this project (a27076dad1c437d27e2dbfd5eef68330ea319c26).

---

Warnings Resolution List:

1. Resolve kotlin stdlib's to lower case deprecated warnings
2. Resolve parameter is never and could be renamed to _ warnings
3. Resolve variable is never used warning
4. Resolve type mismatch type warnings
5. Resolve unsafe use of a nullable receiver type warning
6. Resolve kotlin stdlib's sum by deprecated warnings
7. Resolve name shadowed warning
8. Resolve parameter supertype naming type warnings
9. Resolve non exhaustive when statements when warning
10. Resolve open has no effect in an object warnings
11. Resolve type mismatch type warning for app module
12. Resolve unnecessary safe call type warning for app module
13. Resolve unnecessary safe call type warning for ui tests

Warnings Suppression List:

1. Suppress get color deprecated warning
2. Suppress variable initializer is redundant warning
3. Suppress android junit4 deprecated warnings for ui tests

---

Test

1. There is nothing much to test here.
2. Verifying that all the CI checks are successful should be enough.
3. However, if you want to be thorough about reviewing this change, you could quickly smoke test the example app and see if it is working as expected.

---

Review

@planarvoid and @danilo04 as you were also involved in #982.

---

Make sure strings will be translated:

• [x] If there are new strings that have to be translated, I have added them to the client's strings.xml as a part of the integration PR.

[ParaskP7]

👋 @oguzkocer !

Thank you so much for the review and testing, much appreciated! 🙇 ❤️ 🥇

For full disclosure, there are a few cases where some code can be moved to the ?.let block, such as here or here, but I don't think they are particularly important - especially for this PR. So, I deliberately avoided a line comment, but then I felt uneasy that I am not mentioning it at all 😓

True, I agree, especially on those two cases you mentioned. 💯

PS: I will go ahead and make the change, those seems easy enough to update anyway, thank you for spotting that! 🙇

Related to null-ability changes; I wonder if there are cases where it'd be better to ensure that the value we are dealing is not null (by throwing an exception) rather than only doing the assignment if it's not null. However, I am not familiar enough with the codebase to tell where that'd make sense without investigating each change.

Ha, this is a good point and I was thinking the same while applying the code changes myself, as I am not sure about the implication of suppressing the nullability by ignoring it like that vs. throwing an exception instead. The reason that I went ahead by applying changes that are suppressing the nullability is that in this #982 PR @planarvoid created a while back, he followed the same pattern. As such, on every change that I did, I went back to this PR and made sure that the same change were applied here too.

@planarvoid wdyt? 🤔

I suggest waiting for another approval from someone who has more experience with the codebase.

I agree, maybe @planarvoid can help with that extra approval too. 😊

It might also be a good idea to ask the editor to be beta tested after this is merged for the extra peace of mind.

I am shamelessly pinging @fluiddot here in case you can help with this, or at least so that you are aware what's coming in (also, the following PR should be the minSdkVersion = 24).

I'll also add it to my list of things to ask for an extra beta test of the editor on WPAndroid, after this PR is merged and when WPAndroid starts using that new version of Aztec that contain the changes.

That's all what you meant Oguz, right? 🤔

[fluiddot]

It might also be a good idea to ask the editor to be beta tested after this is merged for the extra peace of mind.

I am shamelessly pinging @fluiddot here in case you can help with this, or at least so that you are aware what's coming in (also, the following PR should be the minSdkVersion = 24).

I'll also add it to my list of things to ask for an extra beta test of the editor on WPAndroid, after this PR is merged and when WPAndroid starts using that new version of Aztec that contain the changes.

Thanks for the ping @ParaskP7 🙇 ! I can perform a quick check in Gutenberg using this PR as a double-check, although I don't expect any issues based on the changes outlined in the PR's description 👍 .

[ParaskP7]

Thanks for the ping @ParaskP7 🙇 ! I can perform a quick check in Gutenberg using this PR as a double-check.

That would be awesome, thank you so much for the extra hand @fluiddot ! 🙇 x 💯

...although I don't expect any issues based on the changes outlined in the PR's description 👍

Famous last words... 😅 🤞 🤞 🤞

[oguzkocer]

That's all what you meant Oguz, right? 🤔

Yeap, that's all. Thank you @ParaskP7 🙇‍♂️

[ParaskP7]

👋 @oguzkocer !

For full disclosure, there are a few cases where some code can be moved to the ?.let block, such as here or here, but I don't think they are particularly important - especially for this PR. So, I deliberately avoided a line comment, but then I felt uneasy that I am not mentioning it at all 😓

True, I agree, especially on those two cases you mentioned. 💯

PS: I will go ahead and make the change, those seems easy enough to update anyway, thank you for spotting that! 🙇

After looking at how to move some of the code to the ?.let { ... } block, I am now unsure about your suggestion as I can't think of a good way of moving this code and making it better. 🤔

Can you help me understand your suggestion, for example let's focus on this case here and see if we can improve it anyhow, thank you for your help! 🙏

[fluiddot]

Thanks for the ping @ParaskP7 🙇 ! I can perform a quick check in Gutenberg using this PR as a double-check, although I don't expect any issues based on the changes outlined in the PR's description 👍 .

I've checked this using https://github.com/wordpress-mobile/AztecEditor-Android/pull/1017 and shared this comment. I still need further time to test this as we'd also need to bump the minSdkVersion to 24 in Gutenberg. As I discussed internally with @ParaskP7, I won't have the bandwidth to take a look until Nov 25th, 2022. I'll let you know when I have the results, thanks 🙇 !

[oguzkocer]

Can you help me understand your suggestion, for example let's focus on this case here and see if we can improve it anyhow, thank you for your help! 🙏

@ParaskP7 As I mentioned in my original review, I don't think this is particularly important for this PR. I just mentioned it because I feel very uneasy when I see something and don't mention it at all. In any case, below is how I would have "improved" this function, but note that, although I believe this is more Kotlin-y, there is still some personal taste involved.

override fun onSaveInstanceState(): Parcelable? =
    super.onSaveInstanceState()?.let { superState ->
        SourceViewEditText.SavedState(superState).apply {
            state = Bundle().apply {
                putBoolean("isSourceVisible", sourceEditor?.visibility == View.VISIBLE)
                putBoolean("isMediaMode", isMediaModeEnabled)
                putBoolean("isExpanded", isExpanded)
                putBoolean("isMediaToolbarVisible", isMediaToolbarVisible)
                putByteArray(RETAINED_EDITOR_HTML_PARSED_SHA256_KEY, editorContentParsedSHA256LastSwitch)
                putByteArray(RETAINED_SOURCE_HTML_PARSED_SHA256_KEY, sourceContentParsedSHA256LastSwitch)
            }
        }
    }

I hope you find it useful. Let me know what you think!

P.S: Here is the documentation I consulted to double check that my suggestion is following Kotlin conventions. Hopefully I interpreted it correctly. 🤞

[ParaskP7]

👋 @fluiddot !

I've checked this using https://github.com/wordpress-mobile/AztecEditor-Android/pull/1017 and shared https://github.com/wordpress-mobile/AztecEditor-Android/pull/1017#pullrequestreview-1186576213. I still need further time to test this as we'd also need to bump the minSdkVersion to 24 in Gutenberg. As I discussed internally with @ParaskP7, I won't have the bandwidth to take a look until Nov 25th, 2022. I'll let you know when I have the results, thanks 🙇 !

As per my response here, thank you so much for all your help, let's pick it up again after your return, this is not urgent work! 🙇 ❤️ 🙏

[ParaskP7]

👋 @oguzkocer !

@ParaskP7 As I mentioned in my original review, I don't think this is particularly important for this PR. I just mentioned it because I feel very uneasy when I see something and don't mention it at all. In any case, below is how I would have "improved" this function, but note that, although I believe this is more Kotlin-y, there is still some personal taste involved.

Ah got it, thank you so much for sharing the code snippet, much appreciated, I now understand what you meant! 🙏

I was trying to figure out how to refactor by still keeping the number of nesting levels the same, naively thinking that this is what you meant, a quick win. But, I now see your recommendation more clearly. The only disadvantage of doing this refactor is the levels of nesting and the fact that using 3 chained scope functions like that might increase the mental power needed to understand what is going on. Thus, I might have stopped at the below alternative to keep the nesting at level 1, but as you said, with those refactoring, there is always personal taste involved.

    override fun onSaveInstanceState(): Parcelable? {
        val superState = super.onSaveInstanceState()
        val savedState = superState?.let { SourceViewEditText.SavedState(it) }
        savedState?.state = Bundle().apply {
            putBoolean("isSourceVisible", sourceEditor?.visibility == View.VISIBLE)
            putBoolean("isMediaMode", isMediaModeEnabled)
            putBoolean("isExpanded", isExpanded)
            putBoolean("isMediaToolbarVisible", isMediaToolbarVisible)
            putByteArray(RETAINED_EDITOR_HTML_PARSED_SHA256_KEY, editorContentParsedSHA256LastSwitch)
            putByteArray(RETAINED_SOURCE_HTML_PARSED_SHA256_KEY, sourceContentParsedSHA256LastSwitch)
        }
        return savedState
    }

No matter, thanks for bouncing ideas off that and how these functions can be further improved. I recommend keeping it as is for now, without introducing more changes, just because I think it is better to keep this PR connected to this #982 PR @planarvoid created, along with its changes, and chained with this upstream #1017 PR, without including unnecessary* extra changes in between.

(*) By unnecessary I don't mean that they are not valuable and welcome, I just mean that they won't do much to help with this PR's scope.

[ParaskP7]

Thank you so much for reviewing, testing and merging this @planarvoid , great stuff! 🚀 ❤️ 🥇