Add api level check to request Notification permission

[sabercodic]

Fix lint warning related to notification permission minimum api level.

---

This change is 

[linear]

DROID-386 Notification permission api check

<issue
    id="InlinedApi"
    message="Field requires API level 33 (current min is 26): `android.Manifest.permission#POST_NOTIFICATIONS`"
    errorLine1="            requestNotificationPermissionLauncher.launch(Manifest.permission.POST_NOTIFICATIONS)"
    errorLine2="                                                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
    <location
        file="src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt"
        line="359"
        column="58"/>
</issue>

[albin-mullvad]

Please use proper formatting in the description (punctuation, capitalization etc). Also, this PR is about fixing the issue rather than suppressing it, right?

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @sabercodic)

---

android/lib/common/src/main/kotlin/net/mullvad/mullvadvpn/lib/common/util/SdkUtils.kt line 27 at r1 (raw file):

    }

    fun Context.notificationPermissionIsNotGranted(): Boolean {

Why do we need to (more or less) duplicate the above function? And can you elaborate on why this fixes the issue. Seems like it might be more of a issue with the lint tool itself.

[albin-mullvad]

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @sabercodic)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt line 355 at r2 (raw file):

    private fun checkForNotificationPermission() {
        if (!isNotificationPermissionGranted().not()) {

Why is there a double negative (! .. .not())?

Code quote:

!isNotificationPermissionGranted().not()

[Pururun]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @sabercodic)

---

android/lib/common/src/main/kotlin/net/mullvad/mullvadvpn/lib/common/util/SdkUtils.kt line 28 at r4 (raw file):

    fun getNotificationPermissionResource(): String? {
        return if ((Build.VERSION.SDK_INT > Build.VERSION_CODES.TIRAMISU)) {

Redundant parenthesis?

[sabercodic]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt line 355 at r2 (raw file):

Previously, albin-mullvad wrote…

Why is there a double negative (! .. .not())?

Done.

---

android/lib/common/src/main/kotlin/net/mullvad/mullvadvpn/lib/common/util/SdkUtils.kt line 27 at r1 (raw file):

Previously, albin-mullvad wrote…

Why do we need to (more or less) duplicate the above function? And can you elaborate on why this fixes the issue. Seems like it might be more of a issue with the lint tool itself.

Done.

---

android/lib/common/src/main/kotlin/net/mullvad/mullvadvpn/lib/common/util/SdkUtils.kt line 28 at r4 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Redundant parenthesis?

Done.

[Pururun]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @albin-mullvad)

[Rawa]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @sabercodic)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt line 357 at r5 (raw file):

    private fun checkForNotificationPermission() {
        if (isNotificationPermissionGranted().not()) {
            getNotificationPermissionResource()?.let {

Looks kind of weird to be that we do the requestNotificationPermissionLauncher thingy given that we get a resource.

I think it would be more clear to just say:

if (Build.VERSION.SDK_INT > Build.VERSION_CODES.TIRAMISU) {
    requestNotificationPermissionLauncher.launch(Manifest.permission.POST_NOTIFICATIONS)
    
}

or maybe it can also be more integrated into isNotificationPermission granted? Because that should also require the same API level?

[Pururun]

Reviewed 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @sabercodic)

[albin-mullvad]

Reviewed 1 of 3 files at r4, 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @sabercodic)

---

-- commits line 2 at r7:
Please clarify in the commit and PR title what lint issue this fixes. "InlinedApi" doesn't say much.

Code quote:

Fix InlinedApi lint issue

[sabercodic]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @albin-mullvad)

---

-- commits line 2 at r7:

Previously, albin-mullvad wrote…

Please clarify in the commit and PR title what lint issue this fixes. "InlinedApi" doesn't say much.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt line 357 at r5 (raw file):

Previously, Rawa (David Göransson) wrote…

Looks kind of weird to be that we do the requestNotificationPermissionLauncher thingy given that we get a resource.

I think it would be more clear to just say:

if (Build.VERSION.SDK_INT > Build.VERSION_CODES.TIRAMISU) {
    requestNotificationPermissionLauncher.launch(Manifest.permission.POST_NOTIFICATIONS)
    
}

or maybe it can also be more integrated into isNotificationPermission granted? Because that should also require the same API level?

We try to keep all the sdk check inside SDKUtil, @albin-mullvad what is your idea here?

[albin-mullvad]

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Rawa and @sabercodic)

---

-- commits line 2 at r7:

Previously, sabercodic wrote…

Done.

The PR title is still the same, I asked for both to be updated

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt line 357 at r5 (raw file):

Previously, sabercodic wrote…

We try to keep all the sdk check inside SDKUtil, @albin-mullvad what is your idea here?

After looking a bit at the documentation, I think that we would want to make use of the ChecksSdkIntAtLeast annotation (https://developer.android.com/reference/androidx/annotation/ChecksSdkIntAtLeast). I tried it out, but for some reason it doesn't seem to work. Perhaps due to the classes being part of different modules. Here's how it could look: 0425a9b

[sabercodic]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @Rawa)

---

-- commits line 2 at r7:

Previously, albin-mullvad wrote…

The PR title is still the same, I asked for both to be updated

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt line 357 at r5 (raw file):

Previously, albin-mullvad wrote…

After looking a bit at the documentation, I think that we would want to make use of the ChecksSdkIntAtLeast annotation (https://developer.android.com/reference/androidx/annotation/ChecksSdkIntAtLeast). I tried it out, but for some reason it doesn't seem to work. Perhaps due to the classes being part of different modules. Here's how it could look: 0425a9b

Is it ok to add new dependencies to basic modules?

[Rawa]

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad, @Pururun, and @sabercodic)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt line 357 at r5 (raw file):

Previously, sabercodic wrote…

Is it ok to add new dependencies to basic modules?

I don't see why it would be a problem if it is a dependency we need and it seems reliable. What dependency are you thinking about?

[sabercodic]

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad, @Pururun, and @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt line 357 at r5 (raw file):

Previously, Rawa (David Göransson) wrote…

I don't see why it would be a problem if it is a dependency we need and it seems reliable. What dependency are you thinking about?

@albin-mullvad I tested this by creating temp util file in app module and it worked fine but when I moved it to other module, as you said it was not working.

@Rawa I was talking about adding ChecksSdkIntAtLeast (androidx.annotation:annotation-jvm) to the 'lib' module

[albin-mullvad]

Reviewed 2 of 2 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt line 357 at r5 (raw file):

Previously, sabercodic wrote…

@albin-mullvad I tested this by creating temp util file in app module and it worked fine but when I moved it to other module, as you said it was not working.

@Rawa I was talking about adding ChecksSdkIntAtLeast (androidx.annotation:annotation-jvm) to the 'lib' module

It's completely fine to add that dependency, especially since it's part of androidx. Unfortunate that it doesn't seem to work though. @sabercodic can see if there's a way to make it work?

[albin-mullvad]

Dismissed @Rawa and @albin-mullvad from a discussion.
Reviewable status: 0 of 8 files reviewed, all discussions resolved

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt line 357 at r5 (raw file):

Previously, albin-mullvad wrote…

It's completely fine to add that dependency, especially since it's part of androidx. Unfortunate that it doesn't seem to work though. @sabercodic can see if there's a way to make it work?

I've inverted the logic and it seems to work better with the AGP linting now.

[Pururun]

Reviewed 2 of 4 files at r11, 2 of 6 files at r13, 4 of 4 files at r14, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sabercodic)

---

android/lib/common/build.gradle.kts line 32 at r14 (raw file):

    implementation(project(Dependencies.Mullvad.talpidLib))

    implementation(Dependencies.AndroidX.appcompat)

We should probably have a discussion/create a meeting in regards to how we want to structure the app and what the different modules should do. But for now I guess this is fine.

[albin-mullvad]

Reviewable status: complete! all files reviewed, all discussions resolved

---

android/lib/common/build.gradle.kts line 32 at r14 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

We should probably have a discussion/create a meeting in regards to how we want to structure the app and what the different modules should do. But for now I guess this is fine.

Yes, will do that!