diff --git a/daemon/src/main/kotlin/org/matrix/vector/daemon/system/NotificationManager.kt b/daemon/src/main/kotlin/org/matrix/vector/daemon/system/NotificationManager.kt index b9db34cee..602ab3883 100644 --- a/daemon/src/main/kotlin/org/matrix/vector/daemon/system/NotificationManager.kt +++ b/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) diff --git a/manager/src/main/kotlin/org/matrix/vector/manager/data/model/ModuleDetection.kt b/manager/src/main/kotlin/org/matrix/vector/manager/data/model/ModuleDetection.kt index d7af6d3c4..8974ef1c9 100644 --- a/manager/src/main/kotlin/org/matrix/vector/manager/data/model/ModuleDetection.kt +++ b/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): List = + scope.map { when (it) { "android" -> "system" "system" -> "android" else -> it } } - } private fun String?.toIntOrZero(): Int = this?.trim()?.takeWhile { it.isDigit() }?.toIntOrNull() ?: 0 diff --git a/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/modules/ScopeScreen.kt b/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/modules/ScopeScreen.kt index db0b68c29..10dfe7dfb 100644 --- a/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/modules/ScopeScreen.kt +++ b/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)) + } } }, ) diff --git a/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/modules/ScopeViewModel.kt b/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/modules/ScopeViewModel.kt index 7e9326cc4..23eef4c82 100644 --- a/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/modules/ScopeViewModel.kt +++ b/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 diff --git a/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/repo/RepoDetailsScreen.kt b/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/repo/RepoDetailsScreen.kt index a27820153..07a66f5a4 100644 --- a/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/repo/RepoDetailsScreen.kt +++ b/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, + /** 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 = diff --git a/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/repo/RepoDetailsViewModel.kt b/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/repo/RepoDetailsViewModel.kt index 4d1ae526d..a7805c792 100644 --- a/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/repo/RepoDetailsViewModel.kt +++ b/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/repo/RepoDetailsViewModel.kt @@ -95,6 +95,24 @@ class RepoDetailsViewModel( private val _installedScope = MutableStateFlow>(emptyList()) val installedScope: StateFlow> = _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 = _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 } }