Fix the scope targets no filter can reveal and the routes that lead nowhere

daemon/src/main/kotlin/org/matrix/vector/daemon/system/NotificationManager.kt

@@ -517,13 +517,36 @@ object NotificationManager {
             modulePackageName,
             userName)
 
+    // Which user the link opens, which is not necessarily the one whose update raised this.
+    //
+    // Every notice this function posts is enqueued for user 0 — see the call at the end — so the
+    // reader tapping one is standing in user 0 whichever user's PACKAGE_REPLACED fired it. A link
+    // naming a secondary user therefore lands them somewhere they cannot see: a module installed
+    // in a private space as well as the main user raised two of these, the tag is the package
+    // alone so the second overwrote the first, and the survivor pointed into a profile that is
+    // locked more often than not. The scope editor then listed that profile's apps, which for a
+    // locked private space is nothing at all, and blamed a filter nobody had set.
+    //
+    // Preferring user 0 gives up nothing, because this id does not choose a configuration. A
+    // module is one package and one APK for the whole device with one scope set and one enabled
+    // flag; what the id selects is only which user's apps are offered as targets. So the same
+    // rule applies to the "not activated yet" half, where the act waiting to be done — turning
+    // the module on — is likewise device-wide.
+    //
+    // The triggering user is kept when the module is not in user 0 at all, which is a module
+    // that lives only in a secondary profile. There is no better answer for that one, and the
+    // notice is at least still about somewhere the module exists.
+    val linkUserId =
+        if (packageManager?.isPackageAvailable(modulePackageName, 0, true) == true) 0
+        else moduleUserId
+
     val intent =
         Intent(openManagerAction).apply {
           setPackage("android")
           data =
               Uri.Builder()
                   .scheme("module")
-                  .encodedAuthority("$modulePackageName:$moduleUserId")
+                  .encodedAuthority("$modulePackageName:$linkUserId")
                   .build()
         }
     val pi =
@@ -554,6 +577,11 @@ object NotificationManager {
     // that a module installed for two users shows one notice rather than two — which is what it did
     // before this as well. The collision with the scope prompt is gone either way, now that a scope
     // tag carries its ":user:target" suffix.
+    //
+    // What that price used to include, and no longer does: the two raisings overwrite each other,
+    // so the surviving notice carried whichever user's link happened to be written last. That is
+    // what [linkUserId] above is for — the tag stays user-free and the destination stops depending
+    // on the order two broadcasts arrived in.
     val tag = if (enabled) modulePackageName else notActivatedTag(modulePackageName)
     runCatching {
       nm?.enqueueNotificationWithTag("android", opPkg, tag, tag.hashCode(), notif, 0)

manager/src/main/kotlin/org/matrix/vector/manager/data/model/ModuleDetection.kt

@@ -219,18 +219,33 @@ object ModuleDetection {
                 .getOrNull()
                 ?.filter { it.isNotEmpty() } ?: return emptyList()
 
-        // Legacy modules name the system server the other way round: their "android" is the
-        // daemon's "system", and their "system" is the ordinary "android" package. The convention
-        // is as old as XposedBridge and universal among legacy modules, so the swap is
-        // unconditional.
-        return raw.map {
+        return swapLegacyFrameworkNames(raw)
+    }
+
+    /**
+     * A legacy module's declared scope, spelled the way everything else here spells it.
+     *
+     * Legacy modules name the system server the other way round: their "android" is the daemon's
+     * "system", and their "system" is the ordinary "android" package. XposedBridge reported
+     * `packageName` as "system" for the system dialogues so that a module testing for "android"
+     * found system_server alone, and the scope vocabulary grew up around that; LSPosed later made
+     * "system" the system server and left "android" as the real package, which is what every
+     * modern module and the whole of the daemon mean by the two words today. The convention is
+     * universal among legacy modules, so the swap is unconditional for them.
+     *
+     * Not private, and not applied at the point of reading alone: the store shows a module's
+     * declared scope from the catalogue rather than from the APK, and that list is written in the
+     * module's own vocabulary too — so it has to pass through here before it is put on screen
+     * beside a list that already has.
+     */
+    fun swapLegacyFrameworkNames(scope: List<String>): List<String> =
+        scope.map {
             when (it) {
                 "android" -> "system"
                 "system" -> "android"
                 else -> it
             }
         }
-    }
 
     private fun String?.toIntOrZero(): Int =
         this?.trim()?.takeWhile { it.isDigit() }?.toIntOrNull() ?: 0

manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/modules/ScopeScreen.kt

@@ -197,6 +197,13 @@ fun ScopeScreen(
         }
     val haptics = LocalHapticFeedback.current
     var confirmStranded by remember { mutableStateOf(false) }
+    // Whether the stranding question has already been put this visit, and answered by neither
+    // button. Two slots cannot hold three answers, so when the module has asked for something the
+    // buttons are "give it that" and "switch it off" and there is none that says "leave it exactly
+    // as it is" — cancelling the dialog is the only way to say that, and a warning that comes
+    // straight back on the next back press turns cancelling into a wall the reader cannot get
+    // past. Asked once, then believed.
+    var strandWarned by remember { mutableStateOf(false) }
     val frameworkRestartNeeded by viewModel.frameworkRestartNeeded.collectAsStateWithLifecycle()
 
     val staticScopeNotice = stringResource(R.string.scope_static)
@@ -238,7 +245,8 @@ fun ScopeScreen(
 
     // Leaving a module enabled with nothing to hook does nothing at all but looks like it works.
     fun attemptBack() {
-        if (viewModel.wouldStrandModule()) confirmStranded = true else onNavigateBack()
+        if (!strandWarned && viewModel.wouldStrandModule()) confirmStranded = true
+        else onNavigateBack()
     }
 
     // The gesture leaves this screen exactly as the arrow does, so it asks the same question
@@ -505,24 +513,76 @@ fun ScopeScreen(
     }
 
     if (confirmStranded) {
+        // Three things the reader might mean and two slots to say them in — `VectorAlertDialog`
+        // wraps Material's `AlertDialog`, which has a confirm button and a dismiss button and
+        // nothing else. Which two are offered depends on whether the module asked for anything,
+        // and the third is always reachable by cancelling the dialog.
+        //
+        // A module with a recommendation is the interesting case: the useful answer there is not
+        // "switch it off" but "give it what it asked for", which is what the pre-Compose manager
+        // offered as its positive button whenever a recommendation existed, keeping disable for
+        // the negative one. Offering to disable a module that has told us exactly which apps it
+        // wants is offering to throw away the answer while holding it.
+        val hasRecommended = !state.recommended.isEmpty
+        // Written once and dropped into whichever slot is free: the same act — turn the module off
+        // and leave — is the positive answer when there is nothing better to offer and the
+        // negative one when there is.
+        val disableAndLeave: @Composable () -> Unit = {
+            TextButton(
+                onClick = {
+                    viewModel.setModuleEnabled(false)
+                    confirmStranded = false
+                    onNavigateBack()
+                }
+            ) {
+                Text(stringResource(R.string.scope_empty_disable))
+            }
+        }
         VectorAlertDialog(
-            onDismissRequest = { confirmStranded = false },
+            // Tapping outside, or the system back the dialog handles itself, is a cancel and not
+            // an answer — so it goes back to the list being edited rather than off the screen.
+            // It is also the only way to say "leave it exactly as it is" when the buttons are
+            // taken, which is why it records that the question has now been asked; see
+            // [strandWarned].
+            onDismissRequest = {
+                strandWarned = true
+                confirmStranded = false
+            },
             title = { Text(stringResource(R.string.scope_empty_title)) },
             text = { Text(stringResource(R.string.scope_empty_message)) },
             confirmButton = {
-                TextButton(
-                    onClick = {
-                        viewModel.setModuleEnabled(false)
-                        confirmStranded = false
-                        onNavigateBack()
+                if (hasRecommended) {
+                    // Ticks the recommendation and returns the reader to the list, deliberately
+                    // without leaving: this is an edit like every other on this screen and still
+                    // has to be applied, and navigating away from it would drop the draft on the
+                    // floor a moment after offering it.
+                    TextButton(
+                        onClick = {
+                            viewModel.useRecommended()
+                            confirmStranded = false
+                        }
+                    ) {
+                        Text(stringResource(R.string.scope_use_recommended))
                     }
-                ) {
-                    Text(stringResource(R.string.scope_empty_disable))
+                } else {
+                    disableAndLeave()
                 }
             },
             dismissButton = {
-                TextButton(onClick = { confirmStranded = false }) {
-                    Text(stringResource(R.string.scope_empty_keep))
+                if (hasRecommended) {
+                    disableAndLeave()
+                } else {
+                    // Leaves, which the label has always promised and the button never did:
+                    // dismissing the dialog alone put the reader back on the page they were trying
+                    // to leave, where pressing back asked them the same question again.
+                    TextButton(
+                        onClick = {
+                            confirmStranded = false
+                            onNavigateBack()
+                        }
+                    ) {
+                        Text(stringResource(R.string.scope_empty_keep))
+                    }
                 }
             },
         )

manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/modules/ScopeViewModel.kt

@@ -305,6 +305,7 @@ class ScopeViewModel(
                                 target in filters.draft ||
                                 target in filters.saved ||
                                 target in filters.touched
+                        val requested = app.packageName in recommended
                         if (locked || filters.recommendedOnly) {
                             // Both answer one question — what does this module want, and what have
                             // I given it — and the other filters have no say in either. Chrome is
@@ -314,14 +315,31 @@ class ScopeViewModel(
                             // which is most of them, showed an empty list for exactly that reason.
                             // The screen greys the other three out in both cases, and this is the
                             // code that makes that honest rather than decorative.
-                            return@filter matchesQuery && (inPlay || app.packageName in recommended)
+                            return@filter matchesQuery && (inPlay || requested)
                         }
-                        // The framework is a system target and is filtered like one. It needs no
-                        // exemption of its own: once it is in the scope the line above puts it
-                        // beyond every filter, and before it is chosen it is simply the most
-                        // system of system apps, so someone who has asked not to see those has
-                        // asked not to see it.
-                        val matchesSys = inPlay || filters.showSystem || !app.isSystemApp
+                        // The framework, when the module has asked for it, and nothing else.
+                        //
+                        // A request does not generally outrank the reader's filters: a module may
+                        // name dozens of system packages, and exempting all of them would leave
+                        // the system-apps switch turning nothing off on exactly the modules whose
+                        // lists are longest. The reader has "What the module asks for" for that
+                        // view, and it already overrides all three.
+                        //
+                        // The framework is the exception because it is not one of the several
+                        // hundred rows the filters exist to thin out. It is not an installed
+                        // package at all — it is a synthetic row this view model adds — so it
+                        // cannot be found by turning any filter on and hunting for it, and a
+                        // reader who has never seen it has no reason to think it exists. Hidden,
+                        // a module whose whole declared scope is the framework shows an empty
+                        // list, which is the one case where the filters do not thin a list but
+                        // erase it.
+                        val frameworkRequested =
+                            requested && app.packageName == SYSTEM_FRAMEWORK_PACKAGE
+                        val matchesSys =
+                            inPlay || frameworkRequested || filters.showSystem || !app.isSystemApp
+                        // No exemption needed on either of these: the framework row is built with
+                        // `isGame = false` and is not an installed package, so it is not in the
+                        // module set. Both already pass it.
                         val matchesGame = inPlay || filters.showGames || !app.isGame
                         val matchesModule =
                             inPlay || showMods || modules == null || app.packageName !in modules

manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/repo/RepoDetailsScreen.kt

@@ -93,6 +93,7 @@ import kotlinx.coroutines.launch
 import org.matrix.vector.manager.ui.theme.LocalizedOverlay
 import org.matrix.vector.manager.R
 import org.matrix.vector.manager.ui.theme.currentLocale
+import org.matrix.vector.manager.data.model.ModuleDetection
 import org.matrix.vector.manager.data.model.OnlineModule
 import org.matrix.vector.manager.data.model.Release
 import org.matrix.vector.manager.data.model.ReleaseAsset
@@ -130,6 +131,7 @@ fun RepoDetailsScreen(packageName: String, onNavigateBack: () -> Unit) {
         viewModel(factory = RepoDetailsViewModelFactory(packageName))
     val state by viewModel.state.collectAsState()
     val installedScope by viewModel.installedScope.collectAsState()
+    val installedIsLegacy by viewModel.installedIsLegacy.collectAsState()
     val install by viewModel.installState.collectAsState()
 
     val context = LocalContext.current
@@ -319,6 +321,7 @@ fun RepoDetailsScreen(packageName: String, onNavigateBack: () -> Unit) {
                             module = module,
                             listState = informationScroll,
                             installedScope = installedScope,
+                            installedIsLegacy = installedIsLegacy,
                             onOpenUrl = openUrl,
                         )
                 }
@@ -748,6 +751,8 @@ private fun InformationTab(
     listState: LazyListState,
     /** What the copy on this device declares, when the catalogue declares nothing. */
     installedScope: List<String>,
+    /** Whether that copy is a legacy module, which decides how to read the catalogue's scope. */
+    installedIsLegacy: Boolean,
     onOpenUrl: (String) -> Unit,
 ) {
     // Hoisted: the rows below are emitted from a LazyListScope, which is not a composable.
@@ -767,7 +772,18 @@ private fun InformationTab(
             // into. The catalogue first, because it describes the published module. Failing that,
             // what the installed copy declares in its own APK — accurate for the build actually on
             // this device, and labelled as such so the two are not confused.
-            val published = module.scope?.takeIf { it.isNotEmpty() }
+            //
+            // The catalogue's list is written in the module's own vocabulary, and a legacy
+            // module's is the reverse of everything else here: its "android" is the system server
+            // and its "system" is the ordinary android package. The installed list beside it has
+            // already been through that swap on its way out of the APK, so without this the same
+            // module can name the same target two different ways in two adjacent lines.
+            val published =
+                module.scope
+                    ?.takeIf { it.isNotEmpty() }
+                    ?.let {
+                        if (installedIsLegacy) ModuleDetection.swapLegacyFrameworkNames(it) else it
+                    }
             InfoRow(
                 icon = Icons.Rounded.TrackChanges,
                 label =

manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/repo/RepoDetailsViewModel.kt

@@ -95,6 +95,24 @@ class RepoDetailsViewModel(
     private val _installedScope = MutableStateFlow<List<String>>(emptyList())
     val installedScope: StateFlow<List<String>> = _installedScope.asStateFlow()
 
+    /**
+     * Whether the copy on this device is a legacy module, which decides how to read the
+     * *catalogue's* scope.
+     *
+     * The two generations spell the framework differently — see
+     * `ModuleDetection.swapLegacyFrameworkNames` — and the catalogue entry is written in whichever
+     * vocabulary its module belongs to. Nothing in the payload says which that is, so the installed
+     * copy is the only thing that can answer it, and only for a module that is installed at all.
+     *
+     * False while the module is absent, which is the honest answer rather than a safe one: with
+     * nothing on the device to inspect there is no way to know, and guessing would relabel a
+     * target on the strength of nothing. The consequence is a legacy module whose catalogue names
+     * `android` reading as `android` until it is installed and as `system` afterwards — visibly
+     * odd, and less misleading than the alternative, which is asserting one of the two at random.
+     */
+    private val _installedIsLegacy = MutableStateFlow(false)
+    val installedIsLegacy: StateFlow<Boolean> = _installedIsLegacy.asStateFlow()
+
     private fun readInstalledScope() {
         viewModelScope.launch(Dispatchers.IO) {
             val packageManager = ServiceLocator.context.packageManager
@@ -110,6 +128,7 @@ class RepoDetailsViewModel(
                     info.lastUpdateTime,
                 )
             _installedScope.value = manifest.scope
+            _installedIsLegacy.value = manifest.isLegacy
         }
     }