Feat: Improve "no internet" permission information#19004
Feat: Improve "no internet" permission information#19004AsH1605 wants to merge 3 commits intoankidroid:mainfrom
Conversation
|
First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible. |
|
Important Maintainers: This PR contains Strings changes
|
|
Looks like you need to add a copyright header to get through our lint checks, you can take one from one of the other files and just alter the year and email to be current and your email (you can use your github anonymous email if you prefer) I love the idea though - I like the ability to disable internet permissions in general, but yes AnkIDroid does require localhost permission. Wish the operating systems that allowed you to disable internet allowed you to disable everything but localhost as a fine-grained thing, it's definitely less dangerous to grant that then "internet in general" anyway, happy to merge this once it is in shape |
There was a problem hiding this comment.
The initial project is good. Left some change requests. Also, Lint and tests are failing. Please refer to our Development guide to know how to run them locally.
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/permissions/InternetInfoFragment.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/permissions/InternetInfoFragment.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/permissions/InternetInfoFragment.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/permissions/InternetInfoFragment.kt
Outdated
Show resolved
Hide resolved
|
@BrayanDSO @mikehardy I have already implemented the suggested changes; however, I am currently facing failing test cases and am unable to resolve them. |
340e079 to
4a48cca
Compare
5909e09 to
6a00128
Compare
|
Hi @BrayanDSO, could you please review this PR? I have incorporated all the changes you suggested. |
david-allison
left a comment
There was a problem hiding this comment.
Thanks! Could you add an instrumented test, as this is at high risk of causing regressions
|
@david-allison As mentioned earlier in the chat message: I'm currently using the following test to trace the full permission flow. The test still fails, so I assume I may be missing a part of the intended lifecycle or ViewModel interaction. Any guidance on whether this setup reflects the correct behavior chain would be really helpful. Here is the code I'm using right now: DetailsIndex: AnkiDroid/src/test/java/com/ichi2/anki/ui/windows/permissions/PermissionsActivityTest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/ui/windows/permissions/PermissionsActivityTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/ui/windows/permissions/PermissionsActivityTest.kt
--- a/AnkiDroid/src/test/java/com/ichi2/anki/ui/windows/permissions/PermissionsActivityTest.kt (revision e06af136e8c07a3338219600e71533c2badaf61d)
+++ b/AnkiDroid/src/test/java/com/ichi2/anki/ui/windows/permissions/PermissionsActivityTest.kt (date 1763114678642)
@@ -15,22 +15,96 @@
*/
package com.ichi2.anki.ui.windows.permissions
+import android.content.pm.PackageManager
import androidx.appcompat.widget.AppCompatButton
+import androidx.core.content.ContextCompat
import androidx.fragment.app.commitNow
import androidx.test.core.app.ActivityScenario
import androidx.test.core.app.ActivityScenario.ActivityAction
import androidx.test.core.app.ApplicationProvider
import androidx.test.ext.junit.runners.AndroidJUnit4
+import com.ichi2.anki.DeckPicker
import com.ichi2.anki.PermissionSet
import com.ichi2.anki.R
import com.ichi2.anki.RobolectricTest
+import com.ichi2.anki.deckpicker.DeckPickerViewModel
import com.ichi2.testutils.HamcrestUtils.containsInAnyOrder
+import com.ichi2.utils.Permissions
+import io.mockk.every
+import io.mockk.mockkStatic
+import io.mockk.unmockkStatic
import org.hamcrest.MatcherAssert.assertThat
import org.junit.Test
import org.junit.runner.RunWith
+import org.robolectric.Shadows.shadowOf
+import org.robolectric.shadows.ShadowLooper
@RunWith(AndroidJUnit4::class)
class PermissionsActivityTest : RobolectricTest() {
+
+ @Test
+ fun When_Internet_permission_is_denied_print_next_screen() {
+ println(">>> Launching app…")
+
+ mockkStatic(ContextCompat::class)
+ every {
+ ContextCompat.checkSelfPermission(any(), android.Manifest.permission.INTERNET)
+ } returns PackageManager.PERMISSION_DENIED
+
+ val deckScenario = ActivityScenario.launch(DeckPicker::class.java)
+ ShadowLooper.runUiThreadTasksIncludingDelayedTasks()
+
+ deckScenario.onActivity { deckActivity ->
+ println(">>> FIRST ACTIVITY: ${deckActivity::class.java.simpleName}")
+
+ val vm = deckActivity.viewModel
+ println(">>> ViewModel acquired: ${vm::class.java.simpleName}")
+
+ val fakeResponse = DeckPickerViewModel.StartupResponse.RequestPermissions(PermissionSet.APP_PRIVATE)
+ println(">>> Emitting StartupResponse.RequestPermissions from ViewModel")
+ vm.flowOfStartupResponse.value = fakeResponse
+
+ ShadowLooper.runUiThreadTasksIncludingDelayedTasks()
+
+ val shadow = shadowOf(deckActivity)
+ val next = shadow.nextStartedActivity
+
+ if (next != null) {
+ println(">>> DeckPicker launched: ${next.component?.className}")
+ } else {
+ println(">>> DeckPicker did NOT launch any activity")
+ }
+ }
+
+ val permScenario = ActivityScenario.launch<PermissionsActivity>(
+ PermissionsActivity.getIntent(ApplicationProvider.getApplicationContext(), PermissionSet.APP_PRIVATE)
+ )
+ ShadowLooper.runUiThreadTasksIncludingDelayedTasks()
+
+ permScenario.onActivity { perm ->
+ println(">>> IN PermissionsActivity")
+ val btn = perm.findViewById<AppCompatButton>(R.id.continue_button)
+ println(">>> Continue button enabled (DENIED): ${btn.isEnabled}")
+
+ every {
+ ContextCompat.checkSelfPermission(any(), android.Manifest.permission.INTERNET)
+ } returns PackageManager.PERMISSION_GRANTED
+
+ val frag = perm.supportFragmentManager.findFragmentById(R.id.fragment_container)
+ frag?.onResume()
+
+ println(">>> Continue button enabled (GRANTED): ${btn.isEnabled}")
+
+ println(">>> Clicking continue button…")
+ btn.performClick()
+
+ println(">>> PermissionsActivity finishing? ${perm.isFinishing}")
+ }
+
+ unmockkStatic(ContextCompat::class)
+ println(">>> Test flow complete.")
+ }
+
@Test
fun testActivityCantBeClosedByBackButton() {
testActivity(ARBITRARY_PERMISSION_SET) { activity -> |
|
@david-allison As @thedroiddiv mentioned in comment earlier, is App private not the correct place for internet permission? |
|
|
||
| protected val internetLauncher = | ||
| registerForActivityResult(ActivityResultContracts.RequestPermission()) { requestedPermission -> | ||
| if (!requestedPermission) { |
There was a problem hiding this comment.
This needs logging on both success and failure cases
This appears to do nothing if requestedPermission is true, what should happen?
| } else { | ||
| // On devices such as Xiaomi, which allow user to deny internet permissions, show internet permission item. | ||
| setOnPermissionsRequested { areAlreadyGranted -> | ||
| if (!areAlreadyGranted) internetLauncher.launch(android.Manifest.permission.INTERNET) |
|
@david-allison Thanks for the quick review. Now that I have the go-ahead and know I’m on the right track, I’ll address all the review comments. |
96a5945 to
e0b94ca
Compare
david-allison
left a comment
There was a problem hiding this comment.
Really good! Should be mergeable after my review comments are handled. Cheers!
ericli3690
left a comment
There was a problem hiding this comment.
Looking awesome! Added some comments~
| protected val internetLauncher = | ||
| registerForActivityResult(ActivityResultContracts.RequestPermission()) { requestedPermission -> | ||
| if (!requestedPermission) { | ||
| showToastAndOpenAppSettingsScreen(R.string.startup_no_internet_permission) |
There was a problem hiding this comment.
Hi! I recently merged a change improving some of the permissions request infrastructure AnkiDroid has, I encourage you to take a quick skim over #19167.
In particular, take a look at requestPermissionThroughDialogOrSettings and the submethods it calls. I wrote them to avoid the need to call showToastAndOpenAppSettingsScreen all over the place in multiple files. Also take a look at NotificationsPermissionFragment for an example of how requestPermissionThroughDialogOrSettings is used.
The benefit of requestPermissionThroughDialogOrSettings is that it prevents the user from spamming calls to internetLauncher.launch by clicking the AnkiDroid UI to grant the permission and then deciding not to grant it once they see the OS's provided permission request dialog. If we try to trigger the OS permission request dialog too many times when the user has already denied us the permission (even though we handle the case where the OS permission request dialog doesn't show up here with a showToastAndOpenAppSettingsScreen) the request has already gone through to the OS and is logged by Google, potentially causing AnkiDroid to lose Play Store discoverability.
Obviously, that's an unlikely scenario, but it's the motivation behind requestPermissionThroughDialogOrSettings: that method ensures that the OS permission launcher is not triggered when we know for a fact that the OS will refuse to show it. It launches showToastAndOpenAppSettingsScreen eagerly instead of waiting for the OS dialog to fail first.
Of course, all of this is optional, as this PR should work great with the current approach too, and the scenario requestPermissionThroughDialogOrSettings protects against as I've outlined above is really rare and unlikely. It's up to you as the PR author! Thanks for your work :)
There was a problem hiding this comment.
@ericli3690 Thanks for the detailed explanation.
I’ve been working only on this feature so far, some of the broader flows take me a bit longer to fully understand. I’ll take the time to go through requestPermissionThroughDialogOrSettings, the motivation behind it, and how it’s used in NotificationsPermissionFragment. Once I’m confident about my understanding of the flow end-to-end, I’ll be happy to incorporate it into this PR.
| this.isVisible = false | ||
| } else { | ||
| // On devices such as Xiaomi, which allow user to deny internet permissions, show internet permission item. | ||
| setOnPermissionsRequested { areAlreadyGranted -> |
There was a problem hiding this comment.
** see my other comment, this part will likely also have to be modified a bit if you switch to using requestPermissionThroughDialogOrSettings
ac57abc to
07210bb
Compare
|
@david-allison I’ve addressed the review comments. Kindly let me know if you have any further feedback when you get a chance. Thank you. |
There was a problem hiding this comment.
Cheers! Needs a de-conflict and a few minor changes, then good to go
Index: AnkiDroid/src/main/java/com/ichi2/anki/deckpicker/DeckPickerViewModel.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/deckpicker/DeckPickerViewModel.kt b/AnkiDroid/src/main/java/com/ichi2/anki/deckpicker/DeckPickerViewModel.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/deckpicker/DeckPickerViewModel.kt (revision 07210bbcfcf90c451393224c41f7ca034b664125)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/deckpicker/DeckPickerViewModel.kt (date 1769659032158)
@@ -492,7 +492,7 @@
*/
fun handleStartup(environment: AnkiDroidEnvironment) {
if (!environment.hasRequiredPermissions()) {
- Timber.i("${this.javaClass.simpleName}: postponing startup code - permission screen shown")
+ Timber.i("${this.javaClass.simpleName}: postponing startup code - permission screen shown for %s", environment.requiredPermissions.permissions)
flowOfStartupResponse.value = StartupResponse.RequestPermissions(environment.requiredPermissions)
return
}
Index: AnkiDroid/src/main/java/com/ichi2/anki/introduction/CollectionPermissionScreenLauncher.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/introduction/CollectionPermissionScreenLauncher.kt b/AnkiDroid/src/main/java/com/ichi2/anki/introduction/CollectionPermissionScreenLauncher.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/introduction/CollectionPermissionScreenLauncher.kt (revision 07210bbcfcf90c451393224c41f7ca034b664125)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/introduction/CollectionPermissionScreenLauncher.kt (date 1769658965609)
@@ -46,7 +46,7 @@
fun AnkiActivity.collectionPermissionScreenWasOpened(): Boolean {
val ankiDroidFolder = selectAnkiDroidFolder(this)
if (!ankiDroidFolder.hasRequiredPermissions(this)) {
- Timber.i("${this.javaClass.simpleName}: postponing startup code - permission screen shown")
+ Timber.i("${this.javaClass.simpleName}: postponing startup code - permission screen shown for %s", ankiDroidFolder.permissionSet.permissions)
permissionScreenLauncher.launch(PermissionsActivity.getIntent(this, ankiDroidFolder.permissionSet))
return true
}
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/permissions/InternetPermissionFragment.kt
Outdated
Show resolved
Hide resolved
| <string name="reviewer_toolbar_position_key">reviewerToolbarPosition</string> | ||
|
|
||
| <!-- Onboarding --> | ||
| <string name="internet_permission_requested_key">internetPermissionRequested</string> |
There was a problem hiding this comment.
@ankidroid/core We should pick an architectural pattern:
- is
preferences.xmlfor all string keys - or should it only be used for the settings, with other screens having their own XML?
I prefer the latter
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/permissions/PermissionsFragment.kt
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/permissions/PermissionsFragment.kt
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/permissions/PermissionsFragment.kt
Outdated
Show resolved
Hide resolved
ad56dd6 to
bd4c064
Compare
david-allison
left a comment
There was a problem hiding this comment.
Tests are failing, very likely an approve once they're fixed
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/permissions/PermissionsFragment.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/permissions/PermissionsFragment.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: David Allison <62114487+david-allison@users.noreply.github.com>
bd4c064 to
577ca5e
Compare
david-allison
left a comment
There was a problem hiding this comment.
BEAUTIFUL!!!!!
Thank you so much for all the work you'e put into this, looks perfect!
|
Thanks a lot @david-allison for guiding me throughout the process. I learnt a lot about the code base through this PR. When I started working on this, I thought it would be a small quick fix. But it stretched for exact 6 months😅. Looking forward to pushing more fixes in coming future. |
Purpose / Description
Some devices like Xiaomi and GrapheneOS allow users to manually disable Internet access, which breaks AnkiDroid’s localhost-based functionality. This PR adds a non-blocking permission info screen that appears once per app launch (if localhost access is not available), explaining why Internet access is critical and guiding the user to app settings.
Fixes
Approach
How Has This Been Tested?
Tested on a Xiaomi device by manually disabling Internet access and simulating a localhost failure by pinging a URL. This successfully triggered the Internet permission info screen.
📸 Screenshot of the permission screen:
My device does not support full Internet permission blocking, so original testing on GrapheneOS or similar devices is still required for final validation.
Learning (optional, can help others)
Reused the existing PermissionsActivity and PermissionsFragment infrastructure to maintain consistency with other permission flows.
As someone more familiar with Jetpack Compose, this task helped me gain hands-on experience with XML-based UI and understand AnkiDroid’s legacy layout system.
Checklist
Please, go through these checks before submitting the PR.