diff --git a/daemon/src/main/kotlin/org/matrix/vector/daemon/VectorService.kt b/daemon/src/main/kotlin/org/matrix/vector/daemon/VectorService.kt index 7210abd0b..39ffcbde8 100644 --- a/daemon/src/main/kotlin/org/matrix/vector/daemon/VectorService.kt +++ b/daemon/src/main/kotlin/org/matrix/vector/daemon/VectorService.kt @@ -266,6 +266,20 @@ object VectorService : IDaemonService.Stub() { // Otherwise, only wipe it for the user that just uninstalled it. val targetUser = if (isRemovedForAllUsers) null else userId PreferenceStore.deleteModulePrefs(moduleName, userId, group = null) + // The one preference of a module that is not stored under the module. "Never ask again" + // writes the package into a set belonging to "lspd", so the line above — which deletes + // what is filed under the module's own name — walks straight past it, and the package + // stayed blocked after it was uninstalled. Nothing else ever removes it: the CLI does not + // know the key and the manager offers no way back, so a module blocked by a mis-tap could + // never ask again on that device, and reinstalling it did not help. + // + // Asked of every package that goes for good, not only of modules, and deliberately so: + // `moduleName` here is whatever the broadcast named, and once a package is fully removed + // its metadata is gone, so this branch cannot tell a module from anything else — + // `isXposedModule` is decided below, and only for one that was still in our database. A + // package that was never blocked costs the one read of the "lspd" config row that + // [unblockScopeRequests] starts with, and nothing else. + if (isRemovedForAllUsers) unblockScopeRequests(moduleName) if (isRemovedForAllUsers && ModuleDatabase.removeModule(moduleName)) { // If it was in our DB and we successfully removed it, we treat it as an Xposed module. isXposedModule = true @@ -356,7 +370,6 @@ object VectorService : IDaemonService.Stub() { val data = intent.data ?: return val extras = intent.extras ?: return val callbackBinder = extras.getBinder("callback") ?: return - if (!callbackBinder.isBinderAlive) return val authority = data.encodedAuthority ?: return val parts = authority.split(":", limit = 2) @@ -367,12 +380,63 @@ object VectorService : IDaemonService.Stub() { val scopePackageName = data.path?.substring(1) ?: return // remove leading '/' val action = data.getQueryParameter("action") ?: return + // A prompt outlives the process that asked for it: it sits for an hour, and the app the module + // is running inside can be killed at any point in that hour. For approve, deny and the timeout + // there is then nobody left to tell, so those are dropped where they always were. "Never ask + // again" is not like them — it is the user's decision about the module, not an answer owed to a + // caller who is still listening — so it is honoured whether or not anyone is there to hear it, + // and the claim below still takes the prompt down. + if (!callbackBinder.isBinderAlive && action != "block") return + + // One prompt reaches this receiver from four places — its three buttons and its delete intent + // — and a swipe or the one-hour timeout fires the delete intent whether or not a button was + // pressed first. Answering the module twice, an approval followed by a spurious "Request + // timeout", would be worse than the dismissal never reaching it, so whichever of the four + // arrives first is the one that answers and the rest are dropped. The request is identified by + // the module, its user and the package it asked for; the action is deliberately not part of + // that, since the whole point is that a second *different* action must not answer again. + if (!NotificationManager.claimScopeAnswer(packageName, userId, scopePackageName)) { + Log.d(TAG, "Ignoring $action of $scopePackageName for $packageName: already answered") + return + } + val iCallback = IXposedScopeCallback.Stub.asInterface(callbackBinder) runCatching { + // Answered before the requested package is looked up at all, because "never ask again" is + // a decision about the *module* and not about the package this particular prompt happened + // to name. It used to sit in the when below, under the lookup, so a prompt naming a + // package that no longer resolves — uninstalled or disabled while the prompt was up, + // hidden by a profile owner, or never installed for this user — returned at "Package not + // found" and never reached blockScopeRequests: the user said stop asking, the prompt came + // down, and the module was free to ask again a second later. + if (action == "block") { + blockScopeRequests(packageName) + // The preference only stops the *next* request. A module that asked for three packages + // has a prompt up for each, so without this the user says "never ask again" and is left + // looking at two more questions, both still approvable. Each withdrawn request is + // answered in its own right, because each of them was asked in its own right; what that + // costs a module whose listener expects one call is written out on + // withdrawScopeRequests. + NotificationManager.withdrawScopeRequests(packageName).forEach { pending -> + runCatching { pending.onScopeRequestFailed("Request blocked by configuration") } + } + // Last, and caught, because this is the only call here that reaches into the module's + // process: it may be gone by now, and neither the preference write nor the withdrawal + // above must be lost to a dead binder. They can fail in their own ways — the write goes + // through a SQLite transaction — which is why the whole branch runs inside runCatching + // as well. + runCatching { iCallback.onScopeRequestFailed("Request blocked by configuration") } + return@runCatching + } + val appInfo = packageManager?.getPackageInfoCompat(scopePackageName, 0, userId) if (appInfo == null) { + // Leaving the whole function here skipped the cancel below, which used to be merely + // untidy and is now a prompt nobody can use: the request has been answered, so every + // later press of its buttons is dropped. The request is over either way, so the + // notification goes with it. iCallback.onScopeRequestFailed("Package not found") - return + return@runCatching } when (action) { "approve" -> { @@ -389,20 +453,48 @@ object VectorService : IDaemonService.Stub() { } "deny" -> iCallback.onScopeRequestFailed("Request denied by user") "delete" -> iCallback.onScopeRequestFailed("Request timeout") - "block" -> { - val blocked = - PreferenceStore.getModulePrefs("lspd", 0, "config")["scope_request_blocked"] - as? Set ?: emptySet() - PreferenceStore.updateModulePref( - "lspd", 0, "config", "scope_request_blocked", blocked + packageName) - iCallback.onScopeRequestFailed("Request blocked by configuration") - } } } // onScopeRequestFailed declares @NonNull, and Throwable.message is frequently null. .onFailure { runCatching { iCallback.onScopeRequestFailed(it.message ?: it.toString()) } } - NotificationManager.cancelNotification( - NotificationManager.SCOPE_CHANNEL_ID, packageName, userId) + // Only this one request goes; a module that asked for several packages has a prompt still open + // for each of the others, and they are answered on their own. + NotificationManager.cancelScopeRequest(packageName, userId, scopePackageName) + } + + /** + * The modules that may not ask for scope again. + * + * Filed under "lspd" rather than under the module it names, because it records the user's decision + * about a module rather than that module's own configuration. That is also why uninstalling a + * module does not take it away — `deleteModulePrefs` deletes what is filed under the module's own + * name — and why [unblockScopeRequests] has to exist. + */ + @Suppress("UNCHECKED_CAST") + private fun blockedScopeRequests(): Set = + PreferenceStore.getModulePrefs("lspd", 0, "config")["scope_request_blocked"] as? Set + ?: emptySet() + + private fun blockScopeRequests(packageName: String) { + PreferenceStore.updateModulePref( + "lspd", 0, "config", "scope_request_blocked", blockedScopeRequests() + packageName) + } + + /** + * Lets an uninstalled module ask again if it comes back. + * + * "Never ask again" is a button in a notification, one tap from Approve, and nothing anywhere + * undid it: the socket CLI does not know the key, the manager offers no control for it, and the + * uninstall path missed it. A module blocked by a mis-tap was blocked on that device forever, and + * reinstalling it changed nothing. Uninstalling is a deliberate enough act to count as taking it + * back — and a module that is gone has no decision left to honour. + */ + private fun unblockScopeRequests(packageName: String) { + val blocked = blockedScopeRequests() + if (packageName !in blocked) return + PreferenceStore.updateModulePref( + "lspd", 0, "config", "scope_request_blocked", blocked - packageName) + Log.i(TAG, "$packageName was uninstalled; it may ask for scope again if it returns") } } diff --git a/daemon/src/main/kotlin/org/matrix/vector/daemon/data/FileSystem.kt b/daemon/src/main/kotlin/org/matrix/vector/daemon/data/FileSystem.kt index c3019d347..0aa7ff3ed 100644 --- a/daemon/src/main/kotlin/org/matrix/vector/daemon/data/FileSystem.kt +++ b/daemon/src/main/kotlin/org/matrix/vector/daemon/data/FileSystem.kt @@ -204,10 +204,25 @@ object FileSystem { } } if (!props.getProperty("staticScope").toBoolean()) return@use null - val entry = zip.getEntry("META-INF/xposed/scope.list") ?: return@use emptySet() - zip.getInputStream(entry).bufferedReader().useLines { lines -> - lines.filter { it.isNotEmpty() }.toSet() + val claimed = + zip.getEntry("META-INF/xposed/scope.list")?.let { entry -> + zip.getInputStream(entry).bufferedReader().useLines { lines -> + lines.map { it.trim() }.filter { it.isNotEmpty() }.toSet() + } + } ?: emptySet() + // A module that fixes its scope and then names nothing has fixed it at "no apps at + // all": every write through here is refused, and pruneScopeToClaimed deletes the rows + // the user had already chosen on the next cache rebuild. That is a packaging mistake + // rather than an intention — a module ships staticScope=true with a scope.list it + // forgot to generate — and the cost of reading it literally is a module that can + // never hook anything, silently. So the declaration is ignored and the scope stays + // the user's; the manager's ModuleDetection ignores it too, so the picker it draws + // and the writes accepted here agree about what the module may hook. + if (claimed.isEmpty()) { + Log.w(TAG, "$apkPath fixes its scope but names nothing; ignoring staticScope") + return@use null } + claimed } } .onFailure { Log.w(TAG, "Cannot read the scope list of $apkPath", it) } diff --git a/daemon/src/main/kotlin/org/matrix/vector/daemon/data/ModuleDatabase.kt b/daemon/src/main/kotlin/org/matrix/vector/daemon/data/ModuleDatabase.kt index 6a2c45a87..a71781d1a 100644 --- a/daemon/src/main/kotlin/org/matrix/vector/daemon/data/ModuleDatabase.kt +++ b/daemon/src/main/kotlin/org/matrix/vector/daemon/data/ModuleDatabase.kt @@ -4,6 +4,7 @@ import android.content.ContentValues import android.database.sqlite.SQLiteDatabase import android.util.Log import org.lsposed.lspd.models.Application +import org.matrix.vector.daemon.system.NotificationManager private const val TAG = "VectorModuleDatabase" @@ -209,7 +210,30 @@ object ModuleDatabase { changed = db.update("modules", values, "module_pkg_name = ?", arrayOf(packageName)) > 0 } - if (changed) ConfigCache.requestCacheUpdate() + if (changed) { + ConfigCache.requestCacheUpdate() + // The shade may be telling the user this module "is not activated yet". It has just been + // activated, and nothing else was ever going to take that notice down: it is only marked + // auto-cancel, which fires when it is tapped, and the sole cancel path belonged to the scope + // prompt. The manager cannot do it either — the AIDL exposes no cancel, and a parasitic + // manager could not cancel a notification posted as "android" in any case. + // + // It lives here, at the data layer, rather than in ManagerService because this function is + // where the activations that go through the module table converge: the manager's toggle + // (ManagerService.enableModule), the socket CLI's `modules enable`, a manager backup restore + // — which replays what it read one setModuleEnabled at a time — and setModuleScope's + // implicit enable below. Putting it one level up would cover the manager alone and leave the + // other three lying to the user. Reaching out of the database for it is the same reach + // requestCacheUpdate() above already makes, and for the same reason — a row changed, and + // something outside has to be told. + // + // Not *every* activation, though, and the exception is worth knowing: the socket CLI's + // `db restore` copies a whole database file over the live one and calls nothing here, so a + // module the restored file has enabled keeps a stale "not activated yet" notice in the shade + // until something touches it again. That is a root-shell command that replaces the + // configuration wholesale, and the notice is one of several things it does not reconcile. + NotificationManager.cancelModuleUpdated(packageName) + } return changed } 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 f05a89233..b9db34cee 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 @@ -15,6 +15,7 @@ import android.graphics.drawable.LayerDrawable import android.net.Uri import android.os.Build import android.os.Bundle +import android.os.SystemClock import android.util.Log import io.github.libxposed.service.IXposedScopeCallback import java.util.UUID @@ -28,6 +29,35 @@ private const val STATUS_CHANNEL_ID = "vector_status" private const val UPDATED_CHANNEL_ID = "vector_module_updated" private const val STATUS_NOTIF_ID = BuildConfig.MANAGER_INJECTED_UID +/** + * How long a scope request stays on screen before the platform takes it down for us and fires its + * delete intent, which reports the timeout back to the module. + * + * An hour, as it was when the prompt was first written. A module that asked and was ignored gets a + * definite answer eventually instead of a callback that never fires. + */ +private const val SCOPE_REQUEST_TIMEOUT_MS = 60L * 60 * 1000 + +/** + * How many prompts one module may have waiting for an answer at once. + * + * `IXposedService.requestScope` takes an unbounded list and there is now one prompt per package in + * it, none of them deduplicated beyond exact string equality and none checked for existence before + * it goes up. Nothing else bounds them: these are enqueued as "android", which + * NotificationManagerService exempts from its per-package limit, and they are IMPORTANCE_HIGH, so a + * module asking for a thousand packages gets a thousand heads-up prompts that each sit for + * [SCOPE_REQUEST_TIMEOUT_MS]. That was hidden before this branch only because every prompt of a + * module shared one tag and so replaced the one before it — which is the bug this branch fixed. + * + * Sixteen because it is far above anything an honest module asks for in one go, and low enough that + * the worst a module can do to the shade is a screenful. It bounds what is *unanswered*, not what + * may be asked over time: answering a prompt frees its place at once, so a module that asks a few + * questions and waits for them never meets it. A module that does meet it is told so per package + * rather than left waiting, because a callback that never fires is the failure this whole path + * exists to avoid. + */ +private const val MAX_OPEN_SCOPE_REQUESTS_PER_MODULE = 16 + object NotificationManager { val openManagerAction = UUID.randomUUID().toString() val moduleScopeAction = UUID.randomUUID().toString() @@ -122,19 +152,222 @@ object NotificationManager { } } - fun cancelNotification(channel: String, modulePkg: String, moduleUserId: Int) { - runCatching { - // We use the module package name's hash code as the notification ID - // to match how we enqueued it in requestModuleScope and notifyModuleUpdated. - val notifId = modulePkg.hashCode() + /** + * What tells one scope request apart from another. + * + * The platform identifies a notification by (package, tag, id, user), and everything posted here + * is posted as "android", so the tag and the id are the only room we have. Tagging with the + * module package alone meant a module that asked for three packages posted three notifications + * that each replaced the one before it: only the last request was ever answerable, the earlier + * ones were never granted and their callbacks were never called at all. One module running under + * two users collided in exactly the same way. + */ + private fun scopeTag(modulePkg: String, moduleUserId: Int, scopePkg: String) = + "$modulePkg:$moduleUserId:$scopePkg" + /** Cancels what we posted under [tag]; the id is derived from it exactly as it is on enqueue. */ + private fun cancelByTag(tag: String) { + runCatching { + val notifId = tag.hashCode() if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R) { - nm?.cancelNotificationWithTag("android", "android", modulePkg, notifId, 0) + nm?.cancelNotificationWithTag("android", "android", tag, notifId, 0) } else { - nm?.cancelNotificationWithTag("android", modulePkg, notifId, 0) + nm?.cancelNotificationWithTag("android", tag, notifId, 0) } } - .onFailure { Log.e(TAG, "Failed to cancel notification", it) } + .onFailure { Log.e(TAG, "Failed to cancel notification $tag", it) } + } + + /** + * Takes down the prompt for one (module, user, requested package) once it has been answered. + * + * It has to name the requested package, because a module asking for several has one prompt per + * package and answering one of them must not clear the rest. + */ + fun cancelScopeRequest(modulePkg: String, moduleUserId: Int, scopePkg: String) = + cancelByTag(scopeTag(modulePkg, moduleUserId, scopePkg)) + + /** + * The "not activated yet" half of [notifyModuleUpdated], which is the half that can go stale. + * + * Only the package, because the one place that knows the notice has become wrong — + * `ModuleDatabase.enableModule`, reached from the manager, the socket CLI and a backup restore — + * is given a package name and nothing else. + */ + private fun notActivatedTag(modulePkg: String) = "$modulePkg:not-activated" + + /** + * Takes down the "module is not activated yet" notice for [modulePkg], because it now is. + * + * Deliberately not the *other* thing [notifyModuleUpdated] posts. "Module updated, force stop and + * restart the apps in its scope" is still true after the module has been enabled — nothing in the + * daemon knows whether the user has restarted those apps — so it is left alone, and the two are + * kept under separate tags so that this cancel cannot reach it. Tagging both the same way meant + * editing a module's scope silently erased a restart reminder the user had not acted on yet. + */ + fun cancelModuleUpdated(modulePkg: String) = cancelByTag(notActivatedTag(modulePkg)) + + /** + * The scope prompts that are on screen and still unanswered, oldest first. + * + * Bookkeeping for the receiver, but it lives beside the prompt because only what was posted can + * be answered and only the poster knows what that is. Two things need it. + * + * One prompt reaches the receiver from four places — its three buttons and its delete intent — + * and a swipe or the one-hour timeout fires the delete intent whether or not a button was pressed + * first. Claiming here is what makes the first arrival the one that answers, so a module cannot + * be told its request was approved and then that it timed out. + * + * And "never ask again" has to be able to take down the module's *other* prompts, which it can + * only do if something remembers they are up. + * + * Bounded twice, and neither bound may be silent. Nothing in here is ever "long abandoned": every + * prompt carries [SCOPE_REQUEST_TIMEOUT_MS], whose expiry fires the delete intent and claims the + * entry, so an entry that is still here is a live question in front of the user. Losing one + * without answering it is therefore not harmless — [claim] would refuse its buttons, so Approve + * would write nothing, "never ask again" would block nothing and the module would be told + * nothing, all while the notification stayed on screen for up to an hour. So [post] hands back + * whatever it had to give up, and the caller takes it off the screen and fails its callback. + * + * [MAX_OPEN_SCOPE_REQUESTS_PER_MODULE] is the bound that does the work, and it refuses the *new* + * prompt rather than dropping an old one, because the questions already in front of the user are + * the ones worth keeping. [MAX_ENTRIES] is only a backstop against an unbounded number of + * modules in a daemon that runs as long as the device does; it sits far above the per-module + * ceiling precisely so that one module's burst cannot reach as far as another module's prompts, + * which is what a shared sixty-four entries let it do. + */ + private object OutstandingScopeRequests { + private const val MAX_ENTRIES = 512 + + /** One live question, and when it was asked; see [countOf] for what the time is for. */ + private class Open(val callback: IXposedScopeCallback, val postedAt: Long) + + private val open = LinkedHashMap() + + /** A prompt that left [open] unanswered: it has to come off the screen and be told so. */ + class Abandoned(val tag: String, val callback: IXposedScopeCallback) + + /** + * How many prompts [modulePkg] still has open, under any user. The ':' matters here for the + * same reason it does in [claimAllOf]. + * + * Anything older than [SCOPE_REQUEST_TIMEOUT_MS] is not counted, because by then the platform + * has taken the prompt down whatever else happened to it. An entry is removed when its prompt + * is answered or dismissed, and both of those reach us — but neither is guaranteed: an enqueue + * the platform drops, or a cancellation that never fires the delete intent, leaves a question + * nobody will ever answer holding one of the module's places. Without the age, a module could + * be locked out of asking for the rest of the boot by prompts that are not on screen. The entry + * itself is left alone: if its buttons somehow still arrive, they should still work. + */ + private fun countOf(modulePkg: String): Int { + val stillOnScreen = SystemClock.elapsedRealtime() - SCOPE_REQUEST_TIMEOUT_MS + return open.count { (tag, entry) -> + tag.startsWith("$modulePkg:") && entry.postedAt > stillOnScreen + } + } + + /** + * Registers the prompt for [tag] before it goes up. + * + * @return the prompts that had to be given up to make room, which the caller must abandon with + * no lock held, or null when [tag] is the one that must not go up at all because its module + * is already holding [MAX_OPEN_SCOPE_REQUESTS_PER_MODULE] unanswered prompts. + */ + @Synchronized + fun post(tag: String, callback: IXposedScopeCallback): List? { + // Re-posting under a tag replaces what was there: a prompt going up is unanswered by + // definition, whatever became of the last one that asked the same thing. The one it replaces + // is dropped without an answer on purpose — it is the same question, of the same module, + // still on screen under the same tag, and telling the module its request failed would fire + // the listener it is about to be asked with. For the same reason it does not count as a new + // prompt against the ceiling: the shade gains nothing. + val replaced = open.remove(tag) != null + // A package name cannot contain a ':', so the first segment of the tag is the module the + // ceiling belongs to. Per module rather than per (module, user), because all of these are + // enqueued into the same shade whichever user the module runs as. + val modulePkg = tag.substringBefore(':') + if (!replaced && countOf(modulePkg) >= MAX_OPEN_SCOPE_REQUESTS_PER_MODULE) return null + open[tag] = Open(callback, SystemClock.elapsedRealtime()) + val abandoned = mutableListOf() + while (open.size > MAX_ENTRIES) { + val oldest = open.keys.first() + abandoned += Abandoned(oldest, open.remove(oldest)!!.callback) + } + return abandoned + } + + /** Takes the right to answer one prompt; null when something already has. */ + @Synchronized fun claim(tag: String): IXposedScopeCallback? = open.remove(tag)?.callback + + /** Takes every prompt [modulePkg] still has up, in one go, so nothing can answer them after. */ + @Synchronized + fun claimAllOf(modulePkg: String): Map { + // The ':' matters: without it "com.foo" would claim the prompts of "com.foobar" as well. + val mine = open.filterKeys { it.startsWith("$modulePkg:") } + mine.keys.forEach { open.remove(it) } + return mine.mapValues { it.value.callback } + } + } + + /** + * Claims the right to answer the prompt for one (module, user, requested package). + * + * @return true for the first caller, false for every later one — the module's + * [IXposedScopeCallback] must be called exactly once per request. + */ + fun claimScopeAnswer(modulePkg: String, moduleUserId: Int, scopePkg: String) = + OutstandingScopeRequests.claim(scopeTag(modulePkg, moduleUserId, scopePkg)) != null + + /** + * Withdraws every prompt [modulePkg] still has on screen and hands back their callbacks, so the + * caller can tell each of those requests it will not be granted. + * + * What makes "never ask again" mean what it says. A module that asked for three packages now has + * a prompt for each of them; answering the user's "stop asking" by leaving two more questions on + * screen — both still approvable — would be answering it with the opposite. + * + * They are claimed before they are cancelled, and that order is load-bearing, though not for the + * reason this comment used to give: a cancel we ask for ourselves never fires the delete intent. + * `cancelNotificationWithTag` ends in NotificationManagerService's `cancelNotification` with its + * `sendDelete` argument hard-coded false and the reason `REASON_APP_CANCEL`; only a user's swipe + * (`onNotificationClear`) and the `setTimeoutAfter` expiry (`REASON_TIMEOUT`) pass `sendDelete` + * true. What the ordering really defends against is the window the cancel itself opens: it is a + * binder round trip into system_server that only *schedules* the removal on a handler, so these + * prompts keep live Approve, Deny and "never ask again" buttons for a while after we have asked + * for them to go — a tap already in flight, or a swipe or the hourly timeout landing at the same + * moment, would otherwise answer a request we are in the middle of withdrawing. Claiming first + * makes every one of those arrive at a closed door. + * + * Each withdrawn request is handed back on its own, so the caller reports one failure per + * package. That is the honest reading — the module named each package separately and each was + * asked in its own right — but it is worth knowing what it costs: `requestScope` supplies one + * callback for the whole list, so those failures all land on the same binder, and the shipped + * client library does not collapse them. Its `OnScopeEventListener` wrapper calls the listener + * every time and merely drops its map entry afterwards, so a module written to the singular + * javadoc ("invoked when the request is completed") runs its handler once per package rather than + * once per call. Reporting the withdrawal once would be the other defensible choice, but it would + * have to pick one of the packages to name and would leave the rest with no answer at all, which + * is exactly the failure this map exists to prevent. + */ + fun withdrawScopeRequests(modulePkg: String): List { + val withdrawn = OutstandingScopeRequests.claimAllOf(modulePkg) + withdrawn.keys.forEach { cancelByTag(it) } + return withdrawn.values.toList() + } + + /** + * Takes a prompt off the screen and tells its module that request is over, for the cases where + * nobody asked us to: it was given up to make room, or it never made it onto the screen at all. + * + * Both halves are binder calls — one into the notification manager, one into the module — so this + * is deliberately reached with no lock of [OutstandingScopeRequests] held. The callback is + * `oneway`, but the module it points at can be dead by now, and that must not stop the rest of a + * batch from being cleaned up. + */ + private fun abandonScopeRequest(dropped: OutstandingScopeRequests.Abandoned, reason: String) { + cancelByTag(dropped.tag) + runCatching { dropped.callback.onScopeRequestFailed(reason) } + .onFailure { Log.w(TAG, "Could not tell ${dropped.tag} that its request was dropped", it) } } fun requestModuleScope( @@ -143,6 +376,34 @@ object NotificationManager { scopePkg: String, callback: IXposedScopeCallback ) { + val tag = scopeTag(modulePkg, moduleUserId, scopePkg) + // Registered before the notification is built, let alone posted: the buttons are live from the + // moment the platform accepts it, and a prompt the receiver does not know about is one whose + // answer it drops. Registering is also what enforces the ceiling, so there is no point + // rendering an icon for a prompt that is not going up. + val abandoned = OutstandingScopeRequests.post(tag, callback) + if (abandoned == null) { + Log.w( + TAG, + "$modulePkg is already waiting on $MAX_OPEN_SCOPE_REQUESTS_PER_MODULE scope prompts;" + + " not asking about $scopePkg") + // Refused, not ignored. The module is told about this package rather than left holding a + // callback that can never fire, and the message names the package so a module developer can + // see which of their list did not make it. + runCatching { + callback.onScopeRequestFailed( + "Too many scope requests are already waiting for an answer from the user," + + " so $scopePkg was not asked about") + } + .onFailure { Log.w(TAG, "Could not tell $modulePkg its request was refused", it) } + return + } + abandoned.forEach { + Log.w(TAG, "Giving up the scope prompt ${it.tag}: too many are open across all modules") + abandonScopeRequest( + it, "Scope request dropped: too many are waiting for an answer on this device") + } + val context = FakeContext() val userName = userManager?.getUserName(moduleUserId) ?: moduleUserId.toString() @@ -189,6 +450,14 @@ object NotificationManager { context.getString(R.string.never_ask_again), createActionIntent("block", 6)) .build()) + // Swiping the prompt away, or leaving it alone until it expires, has to answer the + // module too. Without a delete intent the "delete" branch of dispatchModuleScope was + // unreachable and a dismissed prompt left the module's IXposedScopeCallback waiting + // for an answer that could no longer arrive from anywhere. It takes a request code of + // its own because that is half of what identifies a PendingIntent: 4, 5 and 6 are the + // buttons above, and 1 and 3 the status and module-updated notifications. + .setDeleteIntent(createActionIntent("delete", 7)) + .setTimeoutAfter(SCOPE_REQUEST_TIMEOUT_MS) .setAutoCancel(true) .setStyle( Notification.BigTextStyle() @@ -202,8 +471,21 @@ object NotificationManager { .apply { extras.putString("android.substName", BuildConfig.FRAMEWORK_NAME) } createChannels() - runCatching { - nm?.enqueueNotificationWithTag("android", opPkg, modulePkg, modulePkg.hashCode(), notif, 0) + val enqueued = + runCatching { + val service = checkNotNull(nm) { "the notification manager is not available" } + service.enqueueNotificationWithTag("android", opPkg, tag, tag.hashCode(), notif, 0) + } + .onFailure { Log.e(TAG, "Failed to post the scope prompt $tag", it) } + .isSuccess + if (!enqueued) { + // The registration above outlives a failed enqueue, and it would then hold one of the + // module's places against a prompt that is not on screen and that nothing can ever answer — + // a module could wedge itself out of asking again on the strength of prompts the user never + // saw. Give the place back and tell the module instead. + OutstandingScopeRequests.claim(tag)?.let { pending -> + runCatching { pending.onScopeRequestFailed("Could not show the scope request") } + } } } @@ -261,9 +543,20 @@ object NotificationManager { .apply { extras.putString("android.substName", BuildConfig.FRAMEWORK_NAME) } createChannels() + // The two notices this function posts are told apart, because only one of them can be made + // wrong by something the user does later: "not activated yet" stops being true the moment the + // module is activated, and [cancelModuleUpdated] takes it down from there; "force stop the apps + // in its scope" stays true until the user does it, which nothing here can observe. One tag for + // both meant activating a module also erased the restart reminder. + // + // Neither carries the user id, and that is deliberate: the cancel is reached from + // ModuleDatabase.enableModule, which is given a package name and nothing more. The price is + // 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. + val tag = if (enabled) modulePackageName else notActivatedTag(modulePackageName) runCatching { - nm?.enqueueNotificationWithTag( - "android", opPkg, modulePackageName, modulePackageName.hashCode(), notif, 0) + nm?.enqueueNotificationWithTag("android", opPkg, tag, tag.hashCode(), notif, 0) } } } diff --git a/manager/src/debug/kotlin/org/matrix/vector/manager/demo/FakeManagerService.kt b/manager/src/debug/kotlin/org/matrix/vector/manager/demo/FakeManagerService.kt index 1b39ac93a..a3c79f533 100644 --- a/manager/src/debug/kotlin/org/matrix/vector/manager/demo/FakeManagerService.kt +++ b/manager/src/debug/kotlin/org/matrix/vector/manager/demo/FakeManagerService.kt @@ -278,7 +278,12 @@ class FakeManagerService( override fun optimizePackage(packageName: String?): Boolean = real?.optimizePackage(packageName) ?: false - override fun enableStatusNotification(): Boolean = real?.enableStatusNotification() ?: false + // `?: true` to match the daemon, whose PreferenceStore reads this one `?: true` when nobody has + // set it — the same reason forcedLauncherIcons above answers true. A fallback here is not a + // failed read: it is handed upstream as a *successful* answer, so answering false would leave + // the status page's switch — and the ManagerPresence field HomeViewModel fills from the same + // call — showing the opposite of what an untouched device with a real daemon behind it says. + override fun enableStatusNotification(): Boolean = real?.enableStatusNotification() ?: true override fun setEnableStatusNotification(enable: Boolean) { real?.setEnableStatusNotification(enable) 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 cf222e160..ad90e9890 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 @@ -26,7 +26,15 @@ data class ModuleManifest( val targetApiVersion: Int = 0, /** Packages the module asks to hook. */ val scope: List = emptyList(), - /** True when [scope] is the whole of it and the user cannot widen it. */ + /** + * True when [scope] is the outer limit of what may be hooked, not merely what is asked for. + * + * It fixes the set the scope is drawn from and nothing more: which of those packages end up + * in the scope is still the user's answer, and the daemon's `ModuleDatabase.setModuleScope` + * refuses only targets beyond the claimed set, so any subset of it is stored. + * + * Never true while [scope] is empty, whatever module.prop says: see [ModuleDetection.inspect]. + */ val staticScope: Boolean = false, /** The module's own description, which the two generations store in different places. */ val description: String = "", @@ -103,6 +111,25 @@ object ModuleDetection { .filter { it.isNotEmpty() } } ?: emptyList() + // A module that fixes its scope and then names nothing has fixed it at + // "no apps at all": the picker narrows its list to the declared set, so + // the reader would be left with the empty-list state blaming a search + // they never typed, and + // the daemon would refuse every write and prune away the rows the user + // already has. It is a packaging mistake — staticScope=true with a + // scope.list that was never generated — so the flag is dropped and the + // scope stays the user's. FileSystem.readStaticScope ignores the same + // declaration, so the two sides agree on what such a module is allowed + // to hook. + if (static && scope.isEmpty()) { + Log.w( + Constants.TAG, + "modules: ${info.packageName} fixes its scope but names " + + "nothing; ignoring staticScope and leaving the scope open", + ) + static = false + } + ModuleManifest( isModule = true, isLegacy = false, @@ -216,8 +243,11 @@ object ModuleDetection { /** * What a module says it wants to hook. * - * [staticScope] means the module only ever applies to [packages] and the user cannot widen it — - * the scope editor shows the list read-only rather than pretending the choice is theirs. + * [staticScope] means the module fixes *which apps may be listed*, not which of them are hooked: + * [packages] is the outer limit and the user cannot widen it, but which of those packages are in + * the scope remains theirs to choose. The editor narrows its list to [packages] and leaves the + * checkboxes live for exactly that reason, and the daemon agrees — `setModuleScope` refuses only + * targets beyond the claimed set, so any subset of it is accepted. */ data class RecommendedScope(val packages: List, val staticScope: Boolean) { val isEmpty: Boolean diff --git a/manager/src/main/kotlin/org/matrix/vector/manager/data/repository/FrameworkInstaller.kt b/manager/src/main/kotlin/org/matrix/vector/manager/data/repository/FrameworkInstaller.kt index c8cd36310..6532555e2 100644 --- a/manager/src/main/kotlin/org/matrix/vector/manager/data/repository/FrameworkInstaller.kt +++ b/manager/src/main/kotlin/org/matrix/vector/manager/data/repository/FrameworkInstaller.kt @@ -5,13 +5,18 @@ import android.util.Log import java.io.File import java.io.IOException import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job +import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.currentCoroutineContext import kotlinx.coroutines.ensureActive import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.launch import kotlinx.coroutines.withContext +import okhttp3.Call import okhttp3.OkHttpClient import okhttp3.Request import org.lsposed.lspd.IFrameworkInstallCallback @@ -48,6 +53,12 @@ sealed interface FlashStep { * **The download is separate from the flash, and reported separately**, because they fail for * unrelated reasons and the reader needs to know which happened. A download that dies on a flaky * connection has changed nothing on the device; an installer that dies halfway has. + * + * **The flash belongs to this object, not to the screen that asked for it.** It is the daemon that + * is doing the work, and the daemon does not stop for anything the manager does, so the only thing + * a caller taken away mid-flash can achieve is losing the answer. The download in front of it is + * the exception — it has changed nothing on the device yet, and [cancelDownload] is where the + * difference is argued. */ class FrameworkInstaller( private val context: Context, @@ -55,38 +66,165 @@ class FrameworkInstaller( private val daemon: DaemonClient, ) { + /** + * The flash's own scope, alive for as long as the process is. + * + * A flash takes minutes, and the screen that starts one is a single back gesture away from + * being destroyed together with its view model scope. When the work ran there, that gesture + * killed the one line that reads the installer's exit code and moves off [FlashStep.Flashing]: + * the daemon finished the install regardless, and the manager went on reporting a flash that + * was already over, with no button anywhere on the bar to say otherwise, until it was force + * stopped. Supervised, so a run that ends in a throw does not take the scope down with it and + * leave the next flash nowhere to run. + */ + private val scope = CoroutineScope(SupervisorJob() + Dispatchers.IO) + private val _state = MutableStateFlow(FlashStep.Idle) val state: StateFlow = _state.asStateFlow() private val _lines = MutableStateFlow>(emptyList()) - /** Everything the installer has said, in order. Cleared when a new flash starts. */ + /** + * Everything the installer has said, in order. + * + * Cleared when a new flash starts, and when a finished one is put away with [acknowledge]. + */ val lines: StateFlow> = _lines.asStateFlow() + private var job: Job? = null + + /** + * The transfer in flight, and the only part of a flash that can be called off. + * + * The HTTP call rather than the coroutine holding it. Cancelling the coroutine would also + * cancel the wait for the installer's exit code if the press landed in the instant between the + * last byte and the daemon starting, and losing that wait is the failure this class exists to + * prevent; cancelling the call can only ever end a transfer. Written from whichever thread + * pressed the button and read on the download's own, hence volatile. + */ + @Volatile private var transfer: Call? = null + + /** + * Starts fetching [url] and flashing it, and returns straight away. + * + * There is nothing to wait for: [state] and [lines] are where the answer arrives, and they + * outlive whatever screen is watching them. + * + * One at a time. A second call while a flash is in flight is refused rather than queued behind + * it — the only way to reach one is a button on a screen that is reporting a flash in progress, + * so honouring it would mean acting on a decision made against a screen that had moved on. And + * refused means untouched: clearing [lines] on a press that starts nothing would empty the log + * of the flash that is actually running. + */ + fun start(url: String, declaredSize: Long, fileName: String) { + if (job?.isActive == true) return + _lines.value = emptyList() + // Built here rather than on the download's own thread so that [cancelDownload] has + // something to cancel from the first frame the progress row is on screen: a press that + // arrived before the coroutine had been dispatched would otherwise find nothing to stop + // and be dropped in silence. A call cancelled before it is executed refuses to run at all, + // which is the same outcome by a shorter route. + // + // Guarded because building it parses the url, and `HttpUrl` throws on one it cannot read. + // That used to happen inside the download coroutine, where the surrounding catch turned it + // into a failed step; here it would be an uncaught exception on the caller's thread, which + // is the main one. A release with an unusable asset url would take the app down on a press + // of Install rather than saying so on the bar. + val call = + runCatching { client.newCall(Request.Builder().url(url).build()) } + .getOrElse { e -> + Log.w(Constants.TAG, "update: unusable download url $url", e) + append("Download failed: ${e.message}") + _state.value = FlashStep.Failed(ILSPManagerService.INSTALL_NO_SUCH_FILE) + return + } + transfer = call + // Here rather than when the first byte lands: opening the connection can take seconds, and + // a press that leaves the Install button sitting where it was reads as a press that missed. + _state.value = FlashStep.Downloading(0, declaredSize) + job = scope.launch { flash(call, declaredSize, fileName) } + } + + /** + * Calls off a download in progress, leaving nothing of it behind. + * + * A download and nothing else. [FlashStep.Flashing] has no equivalent and must not grow one: + * an installer half way through writing a module tree cannot be recalled, so stopping the wait + * would throw away the exit code and change nothing on the device. A transfer is the opposite + * case — it has changed nothing yet, it can be tens of megabytes over mobile data, and letting + * one the reader has abandoned run to the end would flash a build they had decided against. + * + * Nothing here touches [state]. The transfer's own unwinding puts the bar back to + * [FlashStep.Idle], after it has deleted the part of the zip it had written and at the moment + * it has actually stopped, which is the only moment at which saying so is true. + * + * A press from anywhere else is a no-op: [transfer] is cleared the moment the transfer returns, + * and a press that beats it there by a hair finds a call that has already delivered every byte + * it was asked for. Either way the flash goes ahead, which is what "too late" has to mean. + */ + fun cancelDownload() { + // OkHttp's cancel closes the socket from under the read, so the transfer returns at once + // rather than when the connection's read timeout expires — the difference between a + // button that works and a button that looks broken on a stalled download. + transfer?.cancel() + } + + /** + * Puts a finished flash away, so the bar goes back to offering one. + * + * Only a finished one. A flash still running owns what [state] says about it, and clearing it + * would leave the download and the installer going with nothing on screen admitting it — which + * is the reset this class used to do to itself. A transfer the reader genuinely wants rid of + * is ended by [cancelDownload], which stops it rather than hiding it. + * + * [FlashStep.Flashing] is the state this refusal can strand, and the residual risk is worth + * stating rather than wishing away. Two things end that wait. The daemon dying with the + * install started is one, and it needs nothing from here: `Constants.setBinder` links to that + * death and exits the manager process, taking this state with it. The other is the exit code + * arriving — over a `oneway` binder call that `ManagerService.installFrameworkZip` wraps in + * `runCatching`, logging "Could not report install result" and carrying on when the transaction + * fails. A `oneway` call fails when the *receiving* process's transaction buffer is full, which + * a chatty installer's output can do to this one, and the daemon that then gives up is very + * much alive — so no death recipient fires, [state] stays here, and the bar reports a flash + * that is over for the life of the process. The install itself is unharmed and the device has + * it; what was lost is only the report, and this object holds nothing across a restart, so + * ending the manager process is the way out. That is a poor escape but a better one than + * dismissing the row would be: from here a lost report and an installer still working look + * exactly alike, so a dismissal offered for the first is a dismissal offered during every + * flash — and this class was given a life of its own precisely to stop that press existing. + */ fun acknowledge() { + val step = _state.value + if (step !is FlashStep.Done && step !is FlashStep.Failed) return _state.value = FlashStep.Idle _lines.value = emptyList() } /** - * Fetches [url] and flashes it. + * Fetches what [call] asks for and flashes it, reporting where it has got to through [state]. * - * Returns when the *installer* has exited, not when the download finishes — the caller is a - * screen that stays open across both. + * Runs to the end on [scope] whatever the screen that asked for it does. Only the download can + * be called off, and only through the call it is made on: from [FlashStep.Flashing] onwards + * there is nothing to stop, because an installer half way through writing a module tree cannot + * be recalled and abandoning the wait would throw away the exit code and change nothing else. */ - suspend fun flash(url: String, declaredSize: Long, fileName: String) { - _lines.value = emptyList() + private suspend fun flash(call: Call, declaredSize: Long, fileName: String) { val zip = try { - download(url, declaredSize, fileName) + download(call, declaredSize, fileName) + } catch (_: DownloadAbandoned) { + // Called off, not broken. Nothing reached the device and nothing is left in the + // cache, so there is no failure to report and nothing for the reader to put away: + // the bar goes back to offering the flash it was offering before the press. This + // is the one place that can say so, because it is the moment the transfer has + // actually stopped. + _state.value = FlashStep.Idle + return } catch (e: Exception) { - if (e is CancellationException) { - // Leaving the screen cancels the download, and a cancellation is not a missing - // file. It has to travel on — but the bar goes back to resting first, because - // the progress line has no button on it and nothing else would ever move it. - _state.value = FlashStep.Idle - throw e - } + // Only the process going away can cancel this coroutine — a download is called off + // through its call, which arrives above — but a cancellation is a coroutine ending, + // not a file that could not be fetched, and it must never be recorded as one. + if (e is CancellationException) throw e Log.w(Constants.TAG, "update: download failed", e) append("Download failed: ${e.message}") _state.value = FlashStep.Failed(ILSPManagerService.INSTALL_NO_SUCH_FILE) @@ -94,56 +232,70 @@ class FrameworkInstaller( } _state.value = FlashStep.Flashing - var cancelled = false try { awaitInstall(zip.absolutePath) - } catch (e: CancellationException) { - cancelled = true - throw e } finally { // Deleted once the installer has exited: a release zip left in the cache costs tens of - // megabytes that nothing else will ever clean up. A cancelled wait is not an exited - // installer — the daemon flashes on regardless, out of this very file. - if (!cancelled) runCatching { zip.delete() } + // megabytes that nothing else will ever clean up. + runCatching { zip.delete() } } } - private suspend fun download(url: String, declaredSize: Long, fileName: String): File = + private suspend fun download(call: Call, declaredSize: Long, fileName: String): File = withContext(Dispatchers.IO) { val target = File(context.cacheDir, fileName) - _state.value = FlashStep.Downloading(0, declaredSize) - - client.newCall(Request.Builder().url(url).build()).execute().use { response -> - if (!response.isSuccessful) throw IOException("HTTP ${response.code} for $url") - val body = response.body - val total = body.contentLength().takeIf { it > 0 } ?: declaredSize - - target.outputStream().use { out -> - body.byteStream().use { input -> - val buffer = ByteArray(DOWNLOAD_BUFFER) - var written = 0L - while (true) { - currentCoroutineContext().ensureActive() - val read = input.read(buffer) - if (read == -1) break - out.write(buffer, 0, read) - written += read - _state.value = FlashStep.Downloading(written, total) + + try { + call.execute().use { response -> + if (!response.isSuccessful) { + throw IOException("HTTP ${response.code} for ${call.request().url}") + } + val body = response.body + val total = body.contentLength().takeIf { it > 0 } ?: declaredSize + + target.outputStream().use { out -> + body.byteStream().use { input -> + val buffer = ByteArray(DOWNLOAD_BUFFER) + var written = 0L + while (true) { + currentCoroutineContext().ensureActive() + val read = input.read(buffer) + if (read == -1) break + out.write(buffer, 0, read) + written += read + _state.value = FlashStep.Downloading(written, total) + } } } } + target + } catch (e: Throwable) { + // Nothing will ever finish what is on disk. A transfer that stopped in the middle + // leaves a truncated zip — tens of megabytes of it for a release build — that only + // another attempt at the same file name would overwrite, and nothing else in the + // app would ever reclaim; a connection that never opened leaves no file at all, + // and deleting that costs nothing. + runCatching { target.delete() } + // A cancelled call reaches this as the socket read failing, which is true of the + // socket and false about what happened: the reader stopped it. Said in the type, + // because the caller has two entirely different things to do about the two cases. + if (call.isCanceled()) throw DownloadAbandoned() + throw e + } finally { + // Cleared here rather than by the caller so that it is cleared on every way out, + // including the successful one: from this point the flash is the daemon's and + // there must be nothing left for a cancel to reach. + transfer = null } - target } /** * Runs the daemon-side install and suspends until it reports an exit code. * * The installer's output arrives on the callback as it is produced rather than with the result, - * so the screen fills in during a flash that takes minutes. The exit code comes separately, - * over a deferred whose await is cancellable — leaving the screen stops the wait, and the - * daemon keeps flashing regardless, which is the only safe thing for it to do half way through - * writing a module tree. + * so the screen fills in during a flash that takes minutes. The exit code comes separately, on + * a deferred nobody here abandons: it is the one moment the flash can be called finished, and a + * wait that ended early left the bar spinning over an install that had long since succeeded. */ private suspend fun awaitInstall(path: String) { val done = kotlinx.coroutines.CompletableDeferred() @@ -177,6 +329,15 @@ class FrameworkInstaller( _lines.value = (_lines.value + line).takeLast(MAX_LINES) } + /** + * The transfer was called off by [cancelDownload], rather than failing. + * + * An `IOException` because that is what it is thrown in place of, and its own type because the + * two are the opposite kind of news: one is something to tell the reader about, and the other + * is the reader telling this class something. + */ + private class DownloadAbandoned : IOException("Download abandoned") + private companion object { const val DOWNLOAD_BUFFER = 64 * 1024 const val MAX_LINES = 500 diff --git a/manager/src/main/kotlin/org/matrix/vector/manager/di/ServiceLocator.kt b/manager/src/main/kotlin/org/matrix/vector/manager/di/ServiceLocator.kt index 2b3a0bf12..6d43dda60 100644 --- a/manager/src/main/kotlin/org/matrix/vector/manager/di/ServiceLocator.kt +++ b/manager/src/main/kotlin/org/matrix/vector/manager/di/ServiceLocator.kt @@ -22,6 +22,7 @@ import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.merge import kotlinx.coroutines.launch import okhttp3.OkHttpClient import org.lsposed.lspd.ILSPManagerService @@ -39,6 +40,7 @@ import org.matrix.vector.manager.data.repository.ModuleRepository import org.matrix.vector.manager.data.repository.RepoRepository import org.matrix.vector.manager.data.repository.SettingsRepository import org.matrix.vector.manager.ipc.DaemonClient +import org.matrix.vector.manager.ipc.daemonPackageEventsFlow import org.matrix.vector.manager.ipc.packageEventsFlow import org.matrix.vector.manager.net.HttpClientFactory import org.matrix.vector.manager.net.VectorDns @@ -234,12 +236,23 @@ object ServiceLocator { /** * Invalidates the caches when a package is installed, updated or removed. * - * The only collector of `packageEventsFlow`, and on [appScope] so it lasts as long as the - * process: without it a module installed while the manager is open would never appear. + * The only collector of either flow, and on [appScope] so it lasts as long as the process: + * without it a module installed while the manager is open would never appear. + * + * Two sources, because neither sees the whole device. The platform's own broadcasts arrive only + * for the user this process runs in, so an app installed into a work profile or a secondary + * user went unnoticed here and the scope editor for a module in that profile never listed it. + * The daemon watches every user and re-broadcasts what it saw, but only while it is alive, so + * it cannot replace the platform source either. + * + * A package event on the primary user therefore arrives twice and this body runs twice, which + * is left alone on purpose: it drops two cached fields, reads the enabled list once and bumps a + * revision, and the platform alone already delivers ADDED and REPLACED for a single update — a + * repeat is the case this collector was written for, not a new one. */ private fun observePackageChanges() { appScope.launch { - context.packageEventsFlow().collect { + merge(context.packageEventsFlow(), context.daemonPackageEventsFlow()).collect { apps.invalidate() modules.refresh() modules.notePackagesChanged() diff --git a/manager/src/main/kotlin/org/matrix/vector/manager/ipc/PackageBroadcasts.kt b/manager/src/main/kotlin/org/matrix/vector/manager/ipc/PackageBroadcasts.kt index ba405a2c4..af0b96446 100644 --- a/manager/src/main/kotlin/org/matrix/vector/manager/ipc/PackageBroadcasts.kt +++ b/manager/src/main/kotlin/org/matrix/vector/manager/ipc/PackageBroadcasts.kt @@ -4,9 +4,14 @@ import android.content.BroadcastReceiver import android.content.Context import android.content.Intent import android.content.IntentFilter +import android.util.Log +import androidx.core.content.ContextCompat +import androidx.core.content.IntentCompat import kotlinx.coroutines.channels.awaitClose import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.callbackFlow +import org.matrix.vector.manager.BuildConfig +import org.matrix.vector.manager.Constants import org.matrix.vector.manager.data.model.PER_USER_RANGE sealed class PackageEvent { @@ -23,6 +28,10 @@ sealed class PackageEvent { * * The receiver exists only while the flow is collected. `ServiceLocator` collects it on a scope that * lasts as long as the process, which is what keeps the manager's lists from going stale. + * + * This process's own user and nobody else's — an install in a work profile or a secondary user is + * never delivered here. Those arrive through [daemonPackageEventsFlow], which is collected + * alongside this one. */ fun Context.packageEventsFlow(): Flow = callbackFlow { val receiver = @@ -71,10 +80,112 @@ fun Context.packageEventsFlow(): Flow = callbackFlow { awaitClose { unregisterReceiver(receiver) } } +/** + * The same events as [packageEventsFlow], but for every user on the device. + * + * A dynamic receiver hears its own user's package broadcasts and nobody else's; the rest take + * `registerReceiverForAllUsers` and `INTERACT_ACROSS_USERS`. The parasitic host holds those, being + * `com.android.shell`; a standalone install holds neither, so a receiver built on them would work + * in one of the two shapes this app runs in and not the other. An app installed into a work profile + * or a secondary user therefore reached the manager not at all — nothing dropped the all-users app + * list, so the scope editor for a module in that profile showed every app except the one that had + * just been installed, and no refresh anywhere would ever have brought it in. + * + * The daemon registers its own package receiver for `USER_ALL` and re-broadcasts what it saw to + * both packages the manager can be running as. This is that re-broadcast, and it is the only way + * another user's installs are heard about here. + */ +fun Context.daemonPackageEventsFlow(): Flow = callbackFlow { + val receiver = + object : BroadcastReceiver() { + override fun onReceive(context: Context, intent: Intent) { + // Extras from a stranger are not guaranteed to unparcel in this process — one + // naming a class the manager does not have throws right here, on the main thread, + // and kills the manager rather than whoever sent it. Since the registration below + // has to accept strangers, reading them is guarded. + val event = + runCatching { intent.daemonPackageEvent() } + .onFailure { + Log.w(Constants.TAG, "ipc: unreadable package notification", it) + } + .getOrNull() + if (event != null) trySend(event) + } + } + + // Exported, and it has to be: the sender is the daemon, running as another uid, and since + // SDK 34 a dynamic receiver registered with neither export flag throws at registration. The + // action is guessable and no permission is demanded of the sender, so any app on the device + // can forge one of these. That is acceptable for exactly one reason: a delivery makes the + // manager drop a cache and ask the daemon for it again, and nothing else. Nothing is believed + // on the strength of the payload — the collector does not even read which package an event + // names, and the "isXposedModule" flag the daemon also sends is deliberately never read here, + // because whether a package is a module is settled by inspecting its APK. + val registered = + runCatching { + ContextCompat.registerReceiver( + this@daemonPackageEventsFlow, + receiver, + IntentFilter(ACTION_DAEMON_NOTIFICATION), + ContextCompat.RECEIVER_EXPORTED, + ) + } + // Throwing here would fail this flow, and the merge it is collected in would take the + // platform source down with it — losing every user's package events, on a scope whose + // failure ends the process, to save the one this flow adds. + .onFailure { Log.w(Constants.TAG, "ipc: daemon package notifications unavailable", it) } + .isSuccess + + awaitClose { if (registered) unregisterReceiver(receiver) } +} + +/** + * The daemon's notification, read back with the types it was actually written with. + * + * Not one of these extras can be read with the accessor its name implies, which is worth knowing + * before someone corrects it: the package arrives as a single `String` under + * [Intent.EXTRA_PACKAGES], whose documented type is `String[]`, so `getStringArrayExtra` answers + * null, and the user arrives as a plain `Int` under [Intent.EXTRA_USER], whose documented type is + * `UserHandle`, so `getParcelableExtra` answers null and the id quietly becomes 0 — the one user + * this whole flow exists to see past. What the platform sent is wrapped whole under + * [Intent.EXTRA_INTENT], and only its action says what happened. + */ +private fun Intent.daemonPackageEvent(): PackageEvent? { + val packageName = getStringExtra(Intent.EXTRA_PACKAGES) ?: return null + val userId = getIntExtra(Intent.EXTRA_USER, 0) + val wrapped = IntentCompat.getParcelableExtra(this, Intent.EXTRA_INTENT, Intent::class.java) + + return when (wrapped?.action) { + Intent.ACTION_PACKAGE_ADDED -> PackageEvent.Added(packageName, userId) + Intent.ACTION_PACKAGE_CHANGED -> PackageEvent.Changed(packageName, userId) + // Both removals the daemon forwards mean the package is really gone: the transient + // PACKAGE_REMOVED an update produces is not among the actions it listens for, and + // UID_REMOVED arrives once the uid itself has been reclaimed. Neither is the removal half + // of an update, which is the only thing `fullyRemoved` is there to tell apart. + Intent.ACTION_PACKAGE_FULLY_REMOVED, + Intent.ACTION_UID_REMOVED -> PackageEvent.Removed(packageName, userId, fullyRemoved = true) + else -> null + } +} + +/** + * The action the daemon sends its package notification under. + * + * Built from the standalone manager's package name rather than from whatever this process happens + * to be called, because the manager is usually not running as itself: parasitically it lives inside + * `com.android.shell`, and an action derived from that would be one nobody ever sends. The daemon + * builds this string from its own `DEFAULT_MANAGER_PACKAGE_NAME` and sends it to both hosts, so the + * two sides stay in step only as long as both keep deriving it from that same Gradle value. + */ +private const val ACTION_DAEMON_NOTIFICATION = "${BuildConfig.MANAGER_PACKAGE_NAME}.NOTIFICATION" + /** * `Intent.EXTRA_USER_HANDLE`, which is hidden. * * The public `EXTRA_USER` is a `UserHandle` parcelable, so reading it as an int always answers the * default. The id these broadcasts actually carry is under this name. + * + * Only the platform's broadcasts, mind: the daemon's notification is the other way round and puts a + * plain int under `EXTRA_USER`, so [daemonPackageEvent] reads that key and not this one. */ private const val EXTRA_USER_HANDLE = "android.intent.extra.user_handle" diff --git a/manager/src/main/kotlin/org/matrix/vector/manager/ui/MainActivity.kt b/manager/src/main/kotlin/org/matrix/vector/manager/ui/MainActivity.kt index 0e9411b64..5c4cfd38e 100644 --- a/manager/src/main/kotlin/org/matrix/vector/manager/ui/MainActivity.kt +++ b/manager/src/main/kotlin/org/matrix/vector/manager/ui/MainActivity.kt @@ -1,5 +1,6 @@ package org.matrix.vector.manager.ui +import android.content.Intent import android.os.Bundle import androidx.activity.ComponentActivity import androidx.activity.compose.setContent @@ -7,6 +8,7 @@ import androidx.activity.enableEdgeToEdge import androidx.core.splashscreen.SplashScreen.Companion.installSplashScreen import org.matrix.vector.manager.data.repository.LaunchShortcut import org.matrix.vector.manager.di.ServiceLocator +import org.matrix.vector.manager.ui.navigation.DeepLink import org.matrix.vector.manager.ui.screens.splash.SplashGate import org.matrix.vector.manager.ui.theme.LocalizedContent import org.matrix.vector.manager.ui.theme.VectorTheme @@ -48,6 +50,64 @@ class MainActivity : ComponentActivity() { // splash then plays and decides for itself when the daemon has been given long enough. splash.setKeepOnScreenCondition { false } + // The launch intent can name where to open — the module a notification was about. + // + // Offered on every creation, including a restored one. Parasitically the zygisk hooker + // saves and restores this activity's state itself, so an activity started by a notification + // arrives *with* a bundle and cannot tell itself apart from a rotation by looking at one: + // guarding on `savedInstanceState == null`, which is the obvious reading, skipped the offer + // on exactly the launch that mattered and left the previous tap's destination to be applied + // instead — one module's notification opened another module's scope editor. + // + // What separates the two is not the bundle but the destination, so that judgement belongs + // to DeepLink, which remembers the one it last applied; `intent` here is a field the + // platform keeps answering with the intent this activity was *created* with, so a rotation + // offers a destination the reader may have left behind hours ago. + DeepLink.offerFromCreate(intent) + setContent { LocalizedContent { VectorTheme { SplashGate { VectorApp() } } } } } + + /** + * A second launch while the manager is already up. + * + * Installed normally, `launchMode` is `singleTop`, so tapping a notification reuses this + * activity instead of starting another one and the new intent arrives here rather than at + * [onCreate]. Without this the app would stay on whatever it was already showing and the + * notification would look broken. + * + * Parasitically that is not guaranteed and this may never run. `ParasiticManagerSystemHooker` + * answers the resolution with a copy of the com.android.shell host activity's `ActivityInfo`, + * overriding only its process name, theme and flags, so the launch mode the system works from + * is the host's rather than this manifest's, and the daemon's `openManager` adds only + * `FLAG_ACTIVITY_NEW_TASK`. That is why nothing about the deep link is decided by which of the + * two callbacks ran: [DeepLink] judges the destination instead, and both paths lead there. + * + * [setIntent] is not bookkeeping. The platform leaves `getIntent()` answering the intent this + * activity was created with — `Activity.onNewIntent`'s own documentation says so and points at + * this call — so without it every later recreation would re-offer the *first* notification's + * module, long after a second one moved the reader somewhere else. + */ + override fun onNewIntent(intent: Intent) { + super.onNewIntent(intent) + setIntent(intent) + DeepLink.offerFromNewIntent(intent) + } + + /** + * Ends the deep link's memory along with the screen it was applied to. + * + * [DeepLink] is an object, so what it last applied outlives this activity and would go on + * suppressing a repeat of that destination in a process the platform merely kept cached — so + * backing out of the manager and then tapping a second notice about the same module would open + * Home. Once the activity is finishing there is no stack left to protect, which is the only + * thing that memory is for. + * + * Gated on [isFinishing] because a configuration change destroys this activity too, and + * forgetting there would let the recreation's replayed intent straight through. + */ + override fun onDestroy() { + super.onDestroy() + if (isFinishing) DeepLink.forget() + } } diff --git a/manager/src/main/kotlin/org/matrix/vector/manager/ui/VectorApp.kt b/manager/src/main/kotlin/org/matrix/vector/manager/ui/VectorApp.kt index 29168426b..592014dab 100644 --- a/manager/src/main/kotlin/org/matrix/vector/manager/ui/VectorApp.kt +++ b/manager/src/main/kotlin/org/matrix/vector/manager/ui/VectorApp.kt @@ -7,7 +7,9 @@ import androidx.compose.material3.adaptive.navigationsuite.rememberNavigationSui import androidx.compose.runtime.Composable import androidx.compose.runtime.CompositionLocalProvider import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.getValue import androidx.compose.ui.res.stringResource +import androidx.lifecycle.compose.collectAsStateWithLifecycle import androidx.lifecycle.viewmodel.navigation3.rememberViewModelStoreNavEntryDecorator import androidx.navigation3.runtime.EntryProviderScope import androidx.navigation3.runtime.NavKey @@ -20,6 +22,7 @@ import org.matrix.vector.manager.ui.navigation.Canary import org.matrix.vector.manager.ui.screens.canary.CanaryScreen import org.matrix.vector.manager.ui.navigation.Troubleshoot import org.matrix.vector.manager.ui.screens.report.TroubleshootScreen +import org.matrix.vector.manager.ui.navigation.DeepLink import org.matrix.vector.manager.ui.navigation.LocalNavigator import org.matrix.vector.manager.ui.navigation.Navigator import org.matrix.vector.manager.ui.navigation.Scope @@ -51,6 +54,29 @@ import org.matrix.vector.manager.ui.screens.web.WebScreen fun VectorApp() { val navigator = rememberNavigator() + // Where the launch intent asked to open. The activity has no back stack to act on, so it leaves + // the destination here and this is the first place there is one — on a cold start the splash is + // still playing when the intent arrives. + val pending by DeepLink.pending.collectAsStateWithLifecycle() + LaunchedEffect(pending) { + val destination = DeepLink.consume() ?: return@LaunchedEffect + // Already there, so nothing to do — and doing it anyway would not be nothing: switching + // tabs empties the back stack and builds it again, and the scope editor's draft lives in a + // ViewModel scoped to the entry that would be thrown away with it. The reader who taps the + // notification of the module already open in front of them is the case this covers. + // + // It is not what keeps a rotation harmless. Whether an offer is a launch or a recreation + // replaying the intent it was created with is decided in DeepLink, which knows what it last + // applied; here there is only where the reader is standing. + if (navigator.current == (destination.detail ?: destination.tab)) return@LaunchedEffect + // The tab goes down first and the screen on top of it: a notification about a module opens + // that module's scope editor, and back from there should be the module list rather than the + // door out of the app it just opened. Switching also discards whatever detail screen was + // already up, so the reader is not left with a stale one buried underneath. + navigator.switchTo(destination.tab) + destination.detail?.let { navigator.go(it) } + } + CompositionLocalProvider(LocalNavigator provides navigator) { // The bar shows only at the root of a tab. On a detail screen none of the four items is // the current destination, and a navigation bar highlighting nothing is worse than none. diff --git a/manager/src/main/kotlin/org/matrix/vector/manager/ui/navigation/DeepLink.kt b/manager/src/main/kotlin/org/matrix/vector/manager/ui/navigation/DeepLink.kt new file mode 100644 index 000000000..6cf656ca7 --- /dev/null +++ b/manager/src/main/kotlin/org/matrix/vector/manager/ui/navigation/DeepLink.kt @@ -0,0 +1,191 @@ +package org.matrix.vector.manager.ui.navigation + +import android.content.Intent +import android.net.Uri +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.getAndUpdate + +/** + * Where a launch intent asked the app to open: a tab, and at most one screen above it. + * + * [tab] is not decoration. Pushing [detail] onto whatever the reader happened to be looking at + * would leave them one back press away from a screen they never opened, and on a cold start — where + * the stack is nothing but Home — one back press away from leaving the app the notification just + * brought them into. Laying the tab down first gives the destination somewhere to go back to. + */ +data class PendingDestination(val tab: TopLevelRoute, val detail: Route? = null) + +/** + * The destination a launch intent named, handed from the activity to the composition. + * + * The framework encodes the module a notification is about in the intent's data as + * `module://:`; the daemon copies that Uri onto the manager's launch intent, + * and it survives the parasitic redirection intact. That redirection touches the intent in exactly + * one way — `ParasiticManagerHooker` rewrites the *component* of every intent passing through + * `ActivityClientRecord` to this manager's `MainActivity` — while `ParasiticManagerSystemHooker` + * answers `system_server`'s resolution with a copy of the host activity's `ActivityInfo` and leaves + * the intent alone. Neither touches the data Uri, and neither touches the extras, which is the + * answer to the next reader wondering what else may safely ride on a launch intent. (The hooker + * does more besides, including capturing and restoring saved state; none of it is the intent.) So + * by the time the activity starts, the intent really does + * say which module the reader tapped a notification about — but the activity has no back stack to + * act on. That belongs to the composition, which on a cold start does not exist yet because the + * splash is still playing. This is the hand-off across that gap. + * + * [consume] empties the flow, and emptying it is the point: a destination left behind would be + * applied again on the next recomposition after a configuration change, dragging the reader back to + * the module's scope editor every time they rotated the phone away from it. + */ +object DeepLink { + + private val _pending = MutableStateFlow(null) + + /** + * What [consume] last handed to the shell, for as long as this process lives. + * + * This is the only thing that tells a launch apart from a replay of one. `getIntent()` answers + * the intent the activity was *created* with for the whole life of the task — the platform + * documents that on `Activity.onNewIntent` itself — so every recreation of the activity offers + * that same intent again, minutes or hours after its destination was already applied. + * + * A process kill empties this, and a task restored from recents afterwards is handed the + * original intent once more, so the link is applied there a second time. What that costs is the + * restored position and nothing else: `Navigator` persists the back stack across process death + * through SavedState, so the reader does come back to a stack they will then be moved off — but + * no unapplied edit can survive the kill, the scope editor's draft being plain state flows in a + * ViewModel. Landing on the link's destination is what a cold start from that notification does + * anyway, so it is the same screen reached a different way rather than work thrown away. + */ + private var lastApplied: PendingDestination? = null + + /** Non-null while a launch intent's destination is waiting for the shell to apply it. */ + val pending: StateFlow = _pending.asStateFlow() + + /** + * Records where the intent the activity was *created* with asks the app to open. + * + * A creation is not a launch. Rotation, the automatic dark-mode flip at sunset, a font or + * locale change, an unfold and a restore from recents all build the activity again and hand it + * the intent it was originally started with, so a destination that was applied long ago is + * offered here over and over. Taking it would clear the back stack under a reader who has since + * navigated elsewhere — `Navigator.switchTo` empties the stack, and the scope editor's draft + * goes with the entry it was scoped to — which is why an offer naming [lastApplied] is dropped + * on the floor here rather than judged in the shell: by the time the shell sees a destination + * it can only ask where the reader is standing, not whether this link has already had its turn. + * + * The cost of that rule is one miss, and it is narrower than it first looks because [forget] + * closes the common half of it: an activity that finishes takes the memory with it, so backing + * out of the manager and tapping a second notice for the same module still works even though + * the process was cached in between. + * + * What is left is the parasitic shape with the activity still alive. The resolved activity is a + * copy of the host's, so this manifest's `singleTop` is not necessarily the launch mode the + * system applies and the daemon's `openManager` asks for none; a second notice for the module + * already applied can therefore arrive at a fresh [onCreate] beside the living one, be dropped + * here, and open Home. It is the smaller failure of the two — one notification that opens the + * wrong screen, against a stack emptied under the reader on every rotation — and it needs the + * same module to be announced twice inside one activity's life. + * + * An intent naming nothing this understands still clears the field, exactly as in + * [offerFromNewIntent]; only a repeat of the destination already applied is ignored. + */ + fun offerFromCreate(intent: Intent?) { + val destination = parse(intent) + if (destination != null && destination == lastApplied) return + _pending.value = destination + } + + /** + * Records where an intent delivered to the *running* activity asks the app to open. + * + * This one is a launch by definition — the system only delivers it because something started + * the manager again — so it takes even when it names the destination last applied. That is what + * makes a second tap on the same module's notification move a reader who has wandered off. + * + * The newest launch wins, and an intent naming nothing this understands clears the field rather + * than leaving it. That looks like the more destructive of the two choices and is the safer + * one: this object outlives the activity, and a destination can be offered and never applied — + * `SplashGate` holds the shell out of the composition for the best part of a second, and a back + * press inside that window finishes the activity before anything consumes it. Left in place, it + * would be applied to the *next* launch instead, so opening the app from the launcher would + * drop the reader into the scope editor of a module they were last told about. + * + * Nothing is steered by the clearing itself. The status notification carries no data at all and + * the `*#*#832867#*#*` dialer code arrives under its own `android_secret_code` scheme; neither + * has a screen in mind, and Home — or wherever the reader last was — is the right answer for + * both. + */ + fun offerFromNewIntent(intent: Intent?) { + _pending.value = parse(intent) + } + + /** + * Forgets what was last applied, because the screen it was applied to has gone. + * + * [lastApplied] exists to tell a rebuilt activity from a new launch, and that distinction stops + * meaning anything once the activity finishes: there is no stack left to empty and no reader + * standing anywhere, so whatever starts the manager next is a real launch whoever it names. + * + * Without this the memory would outlive the activity — a Kotlin object lives as long as the + * process, and backing out of the manager leaves that process cached — and the *next* + * notification about the same module would be dropped as a replay and open Home instead. That + * is the failure this whole file exists to prevent, arriving by the back door. + */ + fun forget() { + lastApplied = null + } + + /** + * Takes the waiting destination, leaving nothing for a later recomposition to re-apply. + * + * Taking one is also what marks it applied, and it counts as applied even when the shell then + * decides the reader is already standing there: either way the reader has been given what the + * link asked for, and a later recreation offering it again is a replay. + */ + fun consume(): PendingDestination? = _pending.getAndUpdate { null }?.also { lastApplied = it } + + private fun parse(intent: Intent?): PendingDestination? { + val data = intent?.data ?: return null + if (data.scheme == MODULE_SCHEME) return moduleScope(data) + + // The bare strings the pre-Compose manager accepted. Nothing in this build sends one — the + // pinned launcher shortcut carries no data, and what the framework sends is either nothing, + // the module scheme above or the dialer code's own — but they were this app's launch + // contract for years and honouring one costs a branch. "settings" is deliberately absent: + // settings are sheets raised from Home rather than a destination of their own, so there is + // no screen to open and guessing at one would be worse than ignoring the request. + return when (data.toString()) { + "modules" -> PendingDestination(TopLevelRoute.Modules) + "logs" -> PendingDestination(TopLevelRoute.Logs) + "repo" -> PendingDestination(TopLevelRoute.Store) + else -> null + } + } + + /** + * `module://:`, taken apart exactly the way the framework put it together. + * + * The authority is built with `encodedAuthority("$packageName:$userId")` and split back out + * with a plain `split(":", limit = 2)` on the daemon's own side, so doing the same here is what + * keeps the two ends agreeing about where the package name stops. + * + * [Uri.getHost] and [Uri.getPort] look like the obvious reading and quietly answer something + * else. Neither can fail: an authority the platform cannot find a numeric port in is handed + * back whole as the host, with -1 for the port — so `module://com.example.foo:bad` would open + * the scope editor for a package literally named `com.example.foo:bad` under user -1 rather + * than being rejected. Splitting by hand makes a user id that is not a number no link at all. + */ + private fun moduleScope(data: Uri): PendingDestination? { + val parts = data.encodedAuthority?.split(":", limit = 2) ?: return null + if (parts.size != 2) return null + val packageName = parts[0] + if (packageName.isEmpty()) return null + val userId = parts[1].toIntOrNull() ?: return null + return PendingDestination(TopLevelRoute.Modules, Scope(packageName, userId)) + } +} + +/** The framework's scheme for "open the manager on this module". */ +private const val MODULE_SCHEME = "module" diff --git a/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/home/HomeViewModel.kt b/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/home/HomeViewModel.kt index 098d1fd76..74cff4134 100644 --- a/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/home/HomeViewModel.kt +++ b/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/home/HomeViewModel.kt @@ -85,14 +85,48 @@ data class ManagerPresence( * showing it is a device with a tap-sized route back, and it is on by default. Leaving it out * of this made the first-launch prompt claim there was no way back in to a reader who was * looking at one. + * + * Seeded to what the daemon itself would answer for a device where nobody has touched the + * preference — PreferenceStore.isStatusNotificationEnabled reads the stored value `?: true` — + * because this seed is the answer until the daemon's own has come back, which on a launch that + * has no binder yet is nine round trips away rather than one. Seeded false it said the opposite + * of what was sitting in the shade. */ - val notificationEnabled: Boolean = false, + val notificationEnabled: Boolean = true, + /** + * False until the daemon has actually answered [notificationEnabled]. + * + * [unreachable] is a claim about a device, and it must not be made before the read that would + * settle it has come back. How long that takes depends on which of the two arrived first, this + * ViewModel or the binder. With a binder already in hand, `init` issues the toggle read through + * `refreshPresence` before `refreshStatus` asks for anything, and the wait is one cheap round + * trip; with a binder that lands later, that call found nothing to ask and returned, and the + * read that answers is the one the `service` collect makes once `refreshStatus`'s eight + * sequential round trips are done. The second window is long enough to be seen. The seed above + * is what makes both harmless today; this is what keeps them harmless if the seed is ever wrong + * again, because not knowing and knowing there is no way in are not the same thing and only the + * second is worth a modal. + */ + val notificationKnown: Boolean = false, /** One of the ILSPManagerService.ROOT_* constants, for naming the action button's owner. */ val rootImplementation: Int = 0, ) { - /** True when opening the manager currently depends on remembering how. */ + /** + * True when opening the manager currently depends on remembering how. + * + * False while [notificationKnown] is false as well. The only thing that reads this puts a + * scrimmed dialog in front of the reader, and an unanswered question is not a missing route. + * + * That term withholds nothing an answer has justified: both writers of [notificationEnabled] + * set [notificationKnown] in the same update, so a false [notificationEnabled] already implies + * the daemon answered. What keeps the dialog away from a device whose daemon never answers at + * all is the optimistic seed, and deliberately — HomeScreen already declines to draw it while + * the framework reads inactive, and the offer to install a manager APK is one only a live + * daemon can hand over. + */ val unreachable: Boolean - get() = parasitic && !shortcutPinned && !installed && !notificationEnabled + get() = + notificationKnown && parasitic && !shortcutPinned && !installed && !notificationEnabled } data class DeviceInfo( @@ -142,10 +176,32 @@ class HomeViewModel( // --- how the manager can be reached ------------------------------------------------------- // Above `init` on purpose: a Kotlin class body initialises top to bottom, so the refresh in - // `init` would run against a `_presence` that does not exist yet. + // `init` would run against a `_presence` that does not exist yet — it writes there + // synchronously, before it launches anything. The two framework toggles are here for the same + // concern rather than the same certainty: `init` reaches `refreshToggles` through + // `refreshPresence`, and `viewModelScope` dispatches on `Main.immediate`, so that body starts + // running on the thread that is still constructing this object instead of being posted for + // later. What saves it is what it does first rather than where these flows sit — it writes + // nothing until a `runIpc` has answered, and `runIpc` is a `withContext(Dispatchers.IO)` that + // always dispatches — so declared below `init` it would be one changed first statement away + // from writing to a null. private val _presence = MutableStateFlow(ManagerPresence()) + // True is the daemon's own default for `enable_status_notification` — PreferenceStore reads the + // stored value `?: true` — so the switch shows what the framework is doing on a device where + // nobody has touched it rather than reading "off" until the daemon answers. A switch that reads + // off while the notification is sitting in the shade is worse than one that briefly reads + // optimistically: the first contradicts something the reader can see. + private val _statusNotification = MutableStateFlow(true) + val statusNotification: StateFlow = _statusNotification.asStateFlow() + + // True is the platform's own default for `show_hidden_icon_apps_enabled`, so the switch shows + // what the system is doing on a device where nobody has touched it rather than reading "off" + // until the daemon answers. + private val _hiddenIcon = MutableStateFlow(true) + val hiddenIcon: StateFlow = _hiddenIcon.asStateFlow() + /** * How this manager can be opened, and how it currently is. * @@ -175,6 +231,15 @@ class HomeViewModel( ) } viewModelScope.launch { + // The toggles first, because one of them decides whether the "no way back in" prompt is + // drawn at all while the root implementation only names a button on the card that + // explains the ways in. On a launch that already has a binder this is the first daemon + // read the ViewModel starts, ahead of `refreshStatus`'s: `init` calls `refreshPresence` + // before it begins collecting, and `Main.immediate` runs this body inline on the + // constructing thread. It is also the only retry the toggle reads ever get — arriving + // at Home or at the status page is the moment a stale switch would be looked at, and it + // is a moment somebody chose, unlike a timer. + refreshToggles() val root = daemon.getRootImplementation().getOrNull() if (root != null) _presence.update { it.copy(rootImplementation = root) } } @@ -217,7 +282,12 @@ class HomeViewModel( ServiceLocator.service.collect { service -> refreshStatus(service) // Both switches on the status page hold the daemon's state rather than ours, and - // this is the moment there is a daemon to ask. + // the binder is what they need. This runs for every binder, including one already + // in hand when `refreshPresence` ran above — a second read of two idempotent + // values — and it is here for the one that arrives afterwards, which that call + // found nothing to ask about and returned. Nor is it the last such moment: + // `refreshPresence` asks again whenever a screen that shows them is opened, since + // this flow does not emit a second time while one binder stays alive. if (service != null) refreshToggles() } } @@ -360,13 +430,6 @@ class HomeViewModel( .flowOn(Dispatchers.Default) .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptyList()) - // --- framework toggles ------------------------------------------------------------------ - // Properties of the *framework* rather than of the app, so they belong on the screen that - // reports the framework's state rather than in a settings screen of their own. - - private val _statusNotification = MutableStateFlow(false) - val statusNotification: StateFlow = _statusNotification.asStateFlow() - /** * Whether a newer framework build exists, on the channel this device is actually on. * @@ -376,32 +439,51 @@ class HomeViewModel( */ val frameworkUpdate: StateFlow = ServiceLocator.frameworkUpdates.state - // True is the platform's own default for `show_hidden_icon_apps_enabled`, so the switch shows - // what the system is doing on a device where nobody has touched it rather than reading "off" - // until the daemon answers. - private val _hiddenIcon = MutableStateFlow(true) - val hiddenIcon: StateFlow = _hiddenIcon.asStateFlow() - + /** + * Re-reads both framework switches from the daemon, and is meant to be called again. + * + * `ServiceLocator.service` does not emit a second time while one binder stays alive, so the + * collect in `init` is a single shot: with no other caller, one dropped transaction pinned both + * switches to their seeds — and the launcher prompt to "not asked yet" — for the life of this + * ViewModel. [refreshPresence] is the other caller, and on a launch that already has a binder + * the earlier one, since `init` calls it before it starts collecting; it puts the retry on + * someone opening a screen that shows these rather than on a timer. A timer is the wrong shape + * here anyway: the launcher-icon read costs the daemon a `settings get global` fork every time. + * + * A read that failed writes nothing at all. It is not an answer, and letting it fall back to a + * default is how a switch read correctly on arrival ends up reporting the opposite after a + * single refused transaction. + */ private suspend fun refreshToggles() { - _statusNotification.value = - daemon - .enableStatusNotification() - .onFailure { e -> - Log.w(Constants.TAG, "status: notification toggle unreadable, showing off", e) + // Nothing to ask and nothing gained by asking: with no binder both reads fail without + // leaving the process, and the log fills with unreadable-toggle warnings about a framework + // that is simply not running. + if (!daemon.isAlive) return + daemon + .enableStatusNotification() + .onSuccess { enabled -> + _statusNotification.value = enabled + // The notification is one of the ways into the manager, so what the card offers + // has to follow the same value the switch above it shows — and it is published + // here, not at the end of this function, because the read below forks + // `settings get global` in the daemon and waits for it. Those hundreds of + // milliseconds used to be spent with the launcher prompt believing there was no + // way back into an app the reader may well have opened from this very + // notification. Moving it up is not the whole fix, which is why the seeds and + // [ManagerPresence.notificationKnown] exist: when the binder arrives after this + // ViewModel was built, the call that reaches here is the one in the `service` + // collect, and it waits out refreshStatus's eight sequential round trips first. + _presence.update { + it.copy(notificationEnabled = enabled, notificationKnown = true) } - .getOrDefault(false) + } + .onFailure { e -> Log.w(Constants.TAG, "status: notification toggle unread", e) } // Read rather than assumed: this one is a global system setting, so anything on the device // can have moved it since the manager last wrote it. - _hiddenIcon.value = - daemon - .forcedLauncherIcons() - .onFailure { e -> - Log.w(Constants.TAG, "status: launcher-icon toggle unreadable, showing on", e) - } - .getOrDefault(true) - // The notification is one of the ways into the manager, so what the card offers has to - // follow the same value the switch above it shows. - _presence.update { it.copy(notificationEnabled = _statusNotification.value) } + daemon + .forcedLauncherIcons() + .onSuccess { _hiddenIcon.value = it } + .onFailure { e -> Log.w(Constants.TAG, "status: launcher-icon toggle unread", e) } } fun setStatusNotification(enabled: Boolean) { @@ -410,7 +492,13 @@ class HomeViewModel( .setEnableStatusNotification(enabled) .onSuccess { _statusNotification.value = enabled - _presence.update { it.copy(notificationEnabled = enabled) } + // Known either way now, which matters when `enabled` is false: someone who + // turns the notification off on a parasitic install with nothing else pinned + // has just closed the last easy route back in, and that is precisely the case + // the prompt exists to catch. + _presence.update { + it.copy(notificationEnabled = enabled, notificationKnown = true) + } } .onFailure { e -> Log.e( 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 1bcb1f4f4..f39d8efa1 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 @@ -94,6 +94,7 @@ import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp import androidx.lifecycle.ViewModel import androidx.lifecycle.ViewModelProvider +import androidx.lifecycle.compose.LifecycleResumeEffect import androidx.lifecycle.compose.collectAsStateWithLifecycle import androidx.lifecycle.viewmodel.compose.viewModel import kotlinx.coroutines.launch @@ -226,6 +227,14 @@ fun ScopeScreen( } } + // The view model is scoped to the navigation entry, so it survives leaving the app entirely, + // and nothing else re-reads the scope after `init`. Without this the screen would go on showing + // what the table held when it opened. See `refreshSavedScope` for who else writes it. + LifecycleResumeEffect(packageName, userId) { + viewModel.refreshSavedScope() + onPauseOrDispose {} + } + // 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() @@ -427,7 +436,15 @@ fun ScopeScreen( items(apps, key = { "${it.packageName}:${it.userId}" }) { app -> AppRow( app = app, - enabled = !state.recommended.staticScope && !app.isImplicitInScope, + // A static scope fixes *which apps may be listed*, not which of them the + // user wants. "Users should not apply the module on apps outside the scope + // list" is the whole of what module.prop claims, the list above is already + // narrowed to that set, and the daemon refuses only targets beyond it — a + // subset is accepted. Disabling every row here went further than any of + // that and made the declared scope all or nothing: a module naming three + // apps could be given all three from the selection menu or none, and one + // of them never, with no way to drop one afterwards either. + enabled = !app.isImplicitInScope, origin = when { app.isImplicitInScope -> ScopeOrigin.Derived @@ -802,9 +819,9 @@ private fun ScopeEmptyState() { * How a row came to be in the scope, which decides the ring around its icon. * * Different mechanisms can put an app in a module's scope and they behave differently when the - * world changes — one is fixed forever, one is the module's suggestion, one is only ever what you - * ticked. Rendered as an identical checkbox, a scope the module controls looks exactly like a - * scope the user controls. + * world changes — two are the module naming a target, one is only ever what you ticked, and one is + * the framework's own doing. Rendered as an identical checkbox, a row the module asked for looks + * exactly like a row someone went and found for themselves. * * There is deliberately no "auto-included" origin. The include-new-apps setting reacts to packages * installed *from now on*, and nothing records how an app already in the scope got there, so any @@ -812,9 +829,13 @@ private fun ScopeEmptyState() { * rather than a claim about a row. */ private enum class ScopeOrigin { - /** The module fixed this scope; the user cannot change it. */ + /** + * The module asked for it and fixed the list it came from: no app it did not name reaches this + * screen. Which of the ones it did name are in the scope is still the user's — the daemon + * refuses only targets beyond the declared set, and takes any subset of it. + */ Locked, - /** The module asked for it, but it is the user's choice. */ + /** The module asked for it, and it is the user's choice. */ Requested, /** Nothing asked for it; it is in the scope because someone ticked it. */ Chosen, @@ -830,13 +851,20 @@ private enum class ScopeOrigin { @Composable private fun ScopeOrigin.color(): Color = when (this) { - // Locked reads as "not yours to change", so it borrows the disabled-ish outline rather - // than a colour that invites a tap. - ScopeOrigin.Locked -> MaterialTheme.colorScheme.outline + // Locked and Requested are the same claim by the module — it named this app — and the tick + // beside either is the reader's to give or to take back, so they share the colour that + // invites a tap. It matters most under a static scope, where the list is the declared set + // and so every row of it is Locked: an outline meaning "not yours to change" would be + // saying that about every row of a list the reader is expected to work through. What is + // fixed there is which apps may be listed at all, which is a property of the list and not + // of any row in it — the caption says so, and the dead filter button and its snackbar say + // it in full. + ScopeOrigin.Locked, ScopeOrigin.Requested -> MaterialTheme.colorScheme.primary ScopeOrigin.Chosen -> Color.Transparent - // The same outline as Locked, and for the same reason: it is a tick nobody on this screen - // owns. The caption below it says which of the two kinds of "not yours" this one is. + // The one ring that does mean "not yours": a derived row is the only row on this screen + // that refuses a tap, because nothing here writes it and nothing here can take it away. The + // disabled-ish outline says so before the caption below it does. ScopeOrigin.Derived -> MaterialTheme.colorScheme.outline } 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 a96fdb1de..3101c6688 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 @@ -87,7 +87,13 @@ class ScopeViewModel( private val allApps = MutableStateFlow>(emptyList()) - /** What the daemon currently holds. */ + /** + * What the daemon held when this screen last looked. + * + * The baseline the draft is measured against, and not an oracle: the scope table has writers + * other than this screen, so it goes stale the moment one of them runs. That is why + * [refreshSavedScope] exists, and why [apply] re-reads instead of trusting it. + */ private val savedScope = MutableStateFlow>(emptySet()) /** @@ -100,6 +106,27 @@ class ScopeViewModel( */ private val draftScope = MutableStateFlow>(emptySet()) + /** + * Every target whose tick this visit has actually changed and could otherwise be filtered away + * — one at a time from [toggle], in a batch from [clearAllVisible]. + * + * [selectAllVisible] changes ticks too and deliberately marks nothing: a row it ticks is in the + * draft, and the draft already exempts a row from every filter. Only unticking can drop a row + * out of the list, so only unticking has anything to record here. + * + * Only the list's own filters read it, and only to keep a row present. Unticking is an edit + * like any other and the row has to survive it — an app that vanishes the moment it is + * unticked cannot be re-ticked, so a slip becomes permanent for as long as the reader does not + * think to go and turn a filter on. It never shrinks: a row put in play stays in play until + * the screen is left, which is the point. + * + * Which is also why nothing may go in that was not really changed. A row the reader merely + * *saw* would be exempted from the system, game and module filters for the rest of the visit, + * and since this never shrinks there would be no way back: turning system apps off again would + * hide nothing. + */ + private val touched = MutableStateFlow>(emptySet()) + private val _uiState = MutableStateFlow(ScopeUiState()) val uiState: StateFlow = _uiState.asStateFlow() @@ -213,6 +240,7 @@ class ScopeViewModel( // The saved set as well as the draft: the difference between them is what "newly // ticked" means, and the order below leads with it. .combine(savedScope) { filters, saved -> filters.copy(saved = saved) } + .combine(touched) { filters, t -> filters.copy(touched = t) } // Two typed halves rather than one list of Any. The inputs outnumber the arities // `combine` provides, and carrying them positionally through a `List` and casting // each one back out lets a rename or a reorder compile and then fail at runtime. @@ -231,9 +259,10 @@ class ScopeViewModel( val order = view.sort val reverse = view.reverse val recommended = view.state.recommended.packages.toSet() - // A static scope is the module's whole answer, so the list *is* that set. Showing - // the other few hundred apps beneath uncheckable checkboxes offered a choice that - // does not exist. This one stays absolute: there is no choice to preserve. + // A static scope fixes which apps may be *listed*, so the list is exactly that set. + // The daemon refuses every target beyond it, so a row for one of the other few + // hundred apps would offer a choice that does not exist. Absolute, unlike the + // filters below: this is not a view of the list the reader chose, it is the list. val locked = view.state.recommended.staticScope // The row the daemon hooks whether or not the table names it. Matched on the user // as well as the package: the same module in a work profile is another copy with @@ -251,35 +280,41 @@ class ScopeViewModel( filters.query.isBlank() || app.appName.contains(filters.query, ignoreCase = true) || app.packageName.contains(filters.query, ignoreCase = true) - // An app already in the scope is never filtered away. Otherwise a default - // filter can hide a target the user deliberately chose, and the list then - // disagrees with what the module is actually hooking. - // Derived counts as chosen throughout, so the module's own row survives - // every filter — including the module filter, which is off by default and - // would otherwise hide the row this whole exemption exists to show. - val chosen = + // A row the reader is working with is never filtered away — and unticking + // it is working with it. Keying this on the draft alone meant that + // unticking a system app dropped it through the default filter and out of + // the list mid-edit, taking with it the only tick that could undo the + // mistake. So a row counts as in play if it was in force when this screen + // opened, is in the draft now, or has been touched during this visit. + // Derived counts throughout, so the module's own row survives every filter + // — including the module filter, which is off by default and would + // otherwise hide the row this whole exemption exists to show. + val target = ScopeTarget(app.packageName, app.userId) + val inPlay = implicit(app) || - ScopeTarget(app.packageName, app.userId) in filters.draft - if (filters.recommendedOnly) { - // Answers one question — what does this module want, and what have I - // given it — and the other filters have no say in it. Chrome is a - // system app, so letting them apply would show nothing at all for a + target in filters.draft || + target in filters.saved || + target in filters.touched + 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 + // a system app, so letting them apply would show nothing at all for a // module asking for Chrome unless the reader had also thought to turn - // system apps on. The screen greys the other three out while this is - // on, and this is the code that makes that honest rather than - // decorative. - return@filter matchesQuery && - (chosen || app.packageName in recommended) + // system apps on; a module that fixes its scope on system packages, + // 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) } // 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 = chosen || filters.showSystem || !app.isSystemApp - val matchesGame = chosen || filters.showGames || !app.isGame + val matchesSys = inPlay || filters.showSystem || !app.isSystemApp + val matchesGame = inPlay || filters.showGames || !app.isGame val matchesModule = - chosen || showMods || modules == null || app.packageName !in modules + inPlay || showMods || modules == null || app.packageName !in modules matchesQuery && matchesSys && matchesGame && matchesModule } .map { app -> @@ -356,6 +391,7 @@ class ScopeViewModel( val showGames: Boolean, val recommendedOnly: Boolean, val saved: Set = emptySet(), + val touched: Set = emptySet(), ) private fun comparatorFor(order: ScopeSort): Comparator = @@ -417,21 +453,10 @@ class ScopeViewModel( val userCount = withContext(Dispatchers.IO) { daemonClient.getUsers().getOrNull()?.size ?: 1 } - val savedResult = daemonClient.getModuleScope(modulePackageName) - // Keyed on the exception, not on a null: a fresh module with nothing ticked is a - // success carrying an empty list and must stay silent. - savedResult.exceptionOrNull()?.let { e -> - Log.e( - Constants.TAG, - "scope: reading the saved scope of $modulePackageName failed, showing none", - e, - ) - } - val saved = - savedResult - .getOrNull() - ?.map { ScopeTarget(it.packageName, it.userId) } - ?.toSet() ?: emptySet() + // A scope the daemon will not hand over shows as none rather than keeping the screen + // shut; [readSavedScope] logs why, and keeps that case apart from the empty list a + // module with nothing ticked legitimately has. + val saved = readSavedScope() ?: emptySet() savedScope.value = saved draftScope.value = saved @@ -482,6 +507,67 @@ class ScopeViewModel( } } + /** + * What the daemon holds for this module right now, or null when it would not say. + * + * Null and empty are different answers and every caller here has to tell them apart: a fresh + * module with nothing ticked is a success carrying an empty list, and treating a refused read + * as "no rows" would turn a broken connection into an erased scope. + */ + private suspend fun readSavedScope(): Set? { + val result = daemonClient.getModuleScope(modulePackageName) + result.exceptionOrNull()?.let { e -> + Log.e(Constants.TAG, "scope: reading the saved scope of $modulePackageName failed", e) + } + val rows = result.getOrNull() ?: return null + return rows.map { ScopeTarget(it.packageName, it.userId) }.toSet().asStored() + } + + /** + * The set spelled the way the daemon stores it, so two readings of it can be subtracted. + * + * `ModuleDatabase.setModuleScope` files the framework under user 0 whoever asked for it, so it + * always comes back as user 0 — while a scope restored from a backup written by an older + * manager still names it under the module's own user, and this screen's own row for it is + * user 0. Comparing the two spellings without this makes the framework look added and removed + * at once, and a merge built on that difference would act on both. + */ + private fun Set.asStored(): Set = + mapTo(mutableSetOf()) { + if (it.packageName == SYSTEM_FRAMEWORK_PACKAGE) it.copy(userId = 0) else it + } + + /** + * Re-reads the stored scope and folds whatever arrived from elsewhere into the draft. + * + * Called every time the screen comes back to the front, because this editor is not the only + * writer of that table and the other writer is one tap away: the button in the corner opens + * the module, a libxposed module asks for a target while it runs, and the user approves it + * from the notification shade. Nothing here would ever notice — the load runs once, from + * `init` — so the list would go on drawing an empty box beside a target the module is already + * being loaded into, and the apply bar would count a removal nobody asked for. + * + * Additive on purpose. What the user has ticked here is theirs and survives untouched; a row + * that appeared outside joins both the baseline and the draft, so it reads as in force rather + * than as a pending change. A row that *vanished* outside is left in the draft, where it shows + * honestly as something applying would put back — unticking it here would undo a choice on the + * user's behalf and say nothing. + */ + fun refreshSavedScope() { + // The screen's first resume lands while the load started in `init` is still in flight, and + // that load is this same read: running both races two answers into the same field for no + // gain, and the stale one can win. An apply is likewise mid-write and publishes its own + // result, which this would only be able to contradict. + if (_uiState.value.loading || _applying.value) return + viewModelScope.launch { + val current = readSavedScope() ?: return@launch + val baseline = savedScope.value.asStored() + if (current == baseline) return@launch + savedScope.value = current + draftScope.value = draftScope.value + (current - baseline) + } + } + /** * A stand-in for the system server. * @@ -511,6 +597,8 @@ class ScopeViewModel( // unticking it would draw an empty box beside a process the module is still loaded into. if (app.isImplicitInScope) return val target = ScopeTarget(app.packageName, app.userId) + // Before the draft changes, so the row cannot be filtered out by the very edit being made. + touched.value = touched.value + target draftScope.value = if (selected) draftScope.value + target else draftScope.value - target } @@ -527,12 +615,22 @@ class ScopeViewModel( } fun clearAllVisible() { - draftScope.value = - draftScope.value - - filteredApps.value - .filterNot { it.isImplicitInScope } - .map { ScopeTarget(it.packageName, it.userId) } - .toSet() + val draft = draftScope.value + // The rows this call really unticks, which is only the visible part of the draft — the rest + // of the list is already clear and nothing is being done to it. + val cleared = + filteredApps.value + .filterNot { it.isImplicitInScope } + .map { ScopeTarget(it.packageName, it.userId) } + .filterTo(mutableSetOf()) { it in draft } + // Unticking everything visible must not empty the list as well. Without this, clearing a + // list of system apps removes the rows along with the ticks and leaves the reader looking + // at nothing, with no way to put any of it back. Marking the whole visible list instead of + // the part that changed would buy that at the price of the filters: [touched] never + // shrinks, so every row that happened to be on screen would stay on screen however the + // filters were set for the rest of the visit. + touched.value = touched.value + cleared + draftScope.value = draft - cleared } /** Replace the draft with exactly what the module asked for. */ @@ -683,10 +781,23 @@ class ScopeViewModel( } /** - * Writes the draft, once. + * Writes what the user did, once. + * + * Not the draft as it stands, deliberately. One `setModuleScope` replaces *every* scope row of + * the module, so sending a draft built when the screen opened sends a set that has never heard + * of anything written since — and a row can arrive while this screen sits in the background, + * because the user approving a module's own `requestScope` from the notification shade adds + * one. This screen's own button for opening the module is exactly how a module gets to run and + * ask. The approval would then be deleted by the next apply, minutes after the module had + * already been told it was granted, and nothing anywhere would say so. * - * The whole draft goes out as one `setModuleScope`, which replaces every scope row of the - * module and triggers one configuration rebuild. The new scope reaches an app when its process + * So the edit is what travels: the targets ticked and unticked here, applied to whatever the + * daemon holds at the moment of writing rather than to what the user was shown. When the fresh + * read fails there is nothing better than the picture on screen — falling back to the baseline + * reduces the expression below to exactly the draft, which is the behaviour this replaces, and + * refusing to apply at all would leave an edit that can never be committed. + * + * One write means one configuration rebuild. The new scope reaches an app when its process * next starts; nothing running is restarted here. * * The daemon enables the module as a side effect of storing a scope. @@ -695,9 +806,22 @@ class ScopeViewModel( if (_applying.value) return viewModelScope.launch { _applying.value = true - val draft = draftScope.value + // The snapshot the draft was built from, so the difference between the two is exactly + // what was done on this screen and nothing else. + val baseline = savedScope.value.asStored() + val draft = draftScope.value.asStored() + val current = readSavedScope() + if (current == null) { + Log.w( + Constants.TAG, + "scope: could not re-read the scope of $modulePackageName before writing; " + + "applying the draft as it stands", + ) + } + val before = current ?: baseline + val merged = before + (draft - baseline) - (baseline - draft) val aidl = - draft.map { target -> + merged.map { target -> Application().apply { packageName = target.packageName userId = target.userId @@ -706,23 +830,30 @@ class ScopeViewModel( daemonClient .setModuleScope(modulePackageName, aidl) .onSuccess { stored -> - // The daemon's answer, not merely the fact that it answered: it refuses a - // draft that reaches beyond a scope the module fixes for itself, and keeping - // the draft on a refusal would show a scope the framework never took. + // The daemon's answer, not merely the fact that it answered: it refuses a set + // that reaches beyond a scope the module fixes for itself, and moving the saved + // set on a refusal would show a scope the framework never took. if (!stored) { Log.e( Constants.TAG, - "scope: daemon refused ${draft.size} targets for $modulePackageName", + "scope: daemon refused ${merged.size} targets for $modulePackageName", ) _message.value = ScopeMessage.ApplyFailed } else { // Whether the framework itself just joined or left this scope. Compared - // against what was stored before the write, so it is the change that is - // reported and not the mere presence of the row. + // against what the daemon actually held a moment ago rather than against + // the picture the screen was showing, so it is the change this write made + // that is reported — not the mere presence of the row, and not a change + // somebody else had already made. val framework = ScopeTarget(SYSTEM_FRAMEWORK_PACKAGE, 0) - val wasThere = framework in savedScope.value - val isThere = framework in draft - savedScope.value = draft + val wasThere = framework in before + val isThere = framework in merged + savedScope.value = merged + // The draft moves with it. What went out is now what is stored, and a row + // that arrived from elsewhere and has just been written would otherwise sit + // in the saved set but not the draft — the apply bar would come straight + // back up offering to remove it. + draftScope.value = merged // Storing a scope enables the module, so applying one to a disabled // module would leave the switch here and the row in the module list // both saying it is off. Followed through the switch's own path, so @@ -747,7 +878,7 @@ class ScopeViewModel( .onFailure { e -> Log.e( Constants.TAG, - "scope: apply of ${draft.size} targets to $modulePackageName failed", + "scope: apply of ${merged.size} targets to $modulePackageName failed", e, ) _message.value = ScopeMessage.ApplyFailed diff --git a/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/update/FrameworkUpdateScreen.kt b/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/update/FrameworkUpdateScreen.kt index 31bee01e7..b885e22ca 100644 --- a/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/update/FrameworkUpdateScreen.kt +++ b/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/update/FrameworkUpdateScreen.kt @@ -191,7 +191,9 @@ fun FrameworkUpdateScreen( rootLabel = root.label(), flash = flash, onFlash = { viewModel.flash() }, + onCancelDownload = viewModel::cancelDownload, onReboot = { scope.launch { viewModel.reboot() } }, + onDismiss = viewModel::acknowledge, ) }, ) { padding -> @@ -299,7 +301,9 @@ private fun UpdateBar( rootLabel: String?, flash: FlashStep, onFlash: () -> Unit, + onCancelDownload: () -> Unit, onReboot: () -> Unit, + onDismiss: () -> Unit, ) { val colors = MaterialTheme.colorScheme Column( @@ -310,15 +314,28 @@ private fun UpdateBar( ) { when (flash) { is FlashStep.Downloading -> { - Text( - text = - stringResource( - R.string.update_downloading, - formatSize(flash.bytes), - formatSize(flash.total), - ), - style = MaterialTheme.typography.labelMedium, - ) + // The one step of a flash that comes with a way out. A release zip is tens of + // megabytes and the reader may be paying for them; more to the point, a download + // left to finish goes straight on to flash the build they have just decided + // against, and until this row had a button on it the only way to stop that was to + // force stop the app. The button names the download rather than saying "Cancel", + // because the installer that follows genuinely cannot be called off and a bare + // "Cancel" sitting on this bar would read as an offer to do that too. + Row(verticalAlignment = Alignment.CenterVertically) { + Text( + text = + stringResource( + R.string.update_downloading, + formatSize(flash.bytes), + formatSize(flash.total), + ), + style = MaterialTheme.typography.labelMedium, + modifier = Modifier.weight(1f), + ) + TextButton(onClick = onCancelDownload) { + Text(stringResource(R.string.update_cancel_download)) + } + } Spacer(Modifier.height(8.dp)) if (flash.total > 0) { LinearProgressIndicator( @@ -338,8 +355,16 @@ private fun UpdateBar( style = MaterialTheme.typography.labelLarge, ) } + // Both finished rows are put away by tapping them, the same press the modules list + // takes on its own settled update line. The flash now outlives the screen, so a result + // stays up until somebody reads it — and a build that was installed and not restarted + // straight away would otherwise sit on the bar for the life of the process, with the + // variant picker and the Install button behind it. FlashStep.Done -> - Row(verticalAlignment = Alignment.CenterVertically) { + Row( + modifier = Modifier.fillMaxWidth().clickable(onClick = onDismiss), + verticalAlignment = Alignment.CenterVertically, + ) { Icon(Icons.Rounded.CheckCircle, contentDescription = null, tint = colors.primary) Spacer(Modifier.width(10.dp)) Column(Modifier.weight(1f)) { @@ -366,7 +391,10 @@ private fun UpdateBar( } } is FlashStep.Failed -> - Row(verticalAlignment = Alignment.CenterVertically) { + Row( + modifier = Modifier.fillMaxWidth().clickable(onClick = onDismiss), + verticalAlignment = Alignment.CenterVertically, + ) { Icon(Icons.Rounded.ErrorOutline, contentDescription = null, tint = colors.error) Spacer(Modifier.width(10.dp)) Text( diff --git a/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/update/FrameworkUpdateViewModel.kt b/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/update/FrameworkUpdateViewModel.kt index c9432bea5..827df04cd 100644 --- a/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/update/FrameworkUpdateViewModel.kt +++ b/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/update/FrameworkUpdateViewModel.kt @@ -143,8 +143,14 @@ class FrameworkUpdateViewModel : ViewModel() { fun chooseVariant(variant: ZipVariant) = settings.setUpdateVariant(variant.key) - val lines: StateFlow> = - installer.lines.stateIn(viewModelScope, SharingStarted.Eagerly, emptyList()) + /** + * Everything the installer has said, straight from the installer. + * + * Not a copy kept alive on this scope, the way it used to be: the flash outlives the screen + * now, and a screen that comes back has to find the log it left rather than an empty one that + * fills in a frame later. + */ + val lines: StateFlow> = installer.lines private val _root = MutableStateFlow(RootState()) val root: StateFlow = _root.asStateFlow() @@ -180,6 +186,14 @@ class FrameworkUpdateViewModel : ViewModel() { } } + /** + * Asks for the flash. Nothing here waits on it. + * + * The installer runs it on a scope of its own, because this one goes when the screen does: the + * view model is scoped to the nav entry, so a back gesture during a flash used to cancel the + * wait for the exit code and leave the bar spinning for the life of the process over an install + * the daemon had quietly finished. + */ fun flash() { val zip = chosenZip.value @@ -200,9 +214,29 @@ class FrameworkUpdateViewModel : ViewModel() { ) return } - viewModelScope.launch { installer.flash(url, zip.sizeInBytes, zip.name) } + installer.start(url, zip.sizeInBytes, zip.name) } + /** + * Calls off the download, and only the download. + * + * The flash behind it has no equivalent and is not meant to: once the daemon has started an + * installer there is nothing left to call off, and the wait for its exit code is the one thing + * the installer's own scope exists to protect. A transfer is the other case entirely — it can + * be tens of megabytes on a connection the reader is paying for, and one that runs to the end + * after they have changed their mind goes straight on to flash the build they turned down. + */ + fun cancelDownload() = installer.cancelDownload() + + /** + * Clears a flash that has finished, one way or the other. + * + * A result stays up until it is read, which is the whole point of the installer outliving the + * screen — but it has to be possible to put it down again, or a build that was installed and + * not restarted right away hides the picker behind itself until the process dies. + */ + fun acknowledge() = installer.acknowledge() + suspend fun reboot() { daemon.reboot().onFailure { Log.e(Constants.TAG, "update: reboot request failed", it) } } diff --git a/manager/src/main/res/values-ar/strings.xml b/manager/src/main/res/values-ar/strings.xml index fd9ac4739..56397b117 100644 --- a/manager/src/main/res/values-ar/strings.xml +++ b/manager/src/main/res/values-ar/strings.xml @@ -316,6 +316,7 @@ إصدار مستقر تثبيت تم تنزيل %1$s من %2$s + إلغاء التنزيل جارٍ التثبيت… تم التثبيت إعادة التشغيل diff --git a/manager/src/main/res/values-de/strings.xml b/manager/src/main/res/values-de/strings.xml index f7cea24d2..fb1e9cbcb 100644 --- a/manager/src/main/res/values-de/strings.xml +++ b/manager/src/main/res/values-de/strings.xml @@ -272,6 +272,7 @@ Release Installieren %1$s von %2$s geladen + Download abbrechen Wird installiert… Installiert Neu starten diff --git a/manager/src/main/res/values-es/strings.xml b/manager/src/main/res/values-es/strings.xml index 40960850b..78205c4ce 100644 --- a/manager/src/main/res/values-es/strings.xml +++ b/manager/src/main/res/values-es/strings.xml @@ -272,6 +272,7 @@ Versión estable Instalar Descargados %1$s de %2$s + Cancelar descarga Instalando… Instalado Reiniciar diff --git a/manager/src/main/res/values-fa/strings.xml b/manager/src/main/res/values-fa/strings.xml index 14a8c66f7..412a47b7b 100644 --- a/manager/src/main/res/values-fa/strings.xml +++ b/manager/src/main/res/values-fa/strings.xml @@ -272,6 +272,7 @@ نسخهٔ پایدار نصب ‏%1$s از %2$s بارگیری شد + لغو بارگیری در حال نصب… نصب شد راه‌اندازی دوباره diff --git a/manager/src/main/res/values-fr/strings.xml b/manager/src/main/res/values-fr/strings.xml index d96cbbf54..20142318f 100644 --- a/manager/src/main/res/values-fr/strings.xml +++ b/manager/src/main/res/values-fr/strings.xml @@ -272,6 +272,7 @@ Version stable Installer %1$s téléchargés sur %2$s + Annuler le téléchargement Installation… Installé Redémarrer diff --git a/manager/src/main/res/values-in/strings.xml b/manager/src/main/res/values-in/strings.xml index 8bdbb738a..86ab334fc 100644 --- a/manager/src/main/res/values-in/strings.xml +++ b/manager/src/main/res/values-in/strings.xml @@ -265,6 +265,7 @@ Rilis stabil Pasang Mengunduh %1$s dari %2$s + Batalkan unduhan Memasang… Terpasang Mulai ulang diff --git a/manager/src/main/res/values-it/strings.xml b/manager/src/main/res/values-it/strings.xml index 93e565746..24495ebaf 100644 --- a/manager/src/main/res/values-it/strings.xml +++ b/manager/src/main/res/values-it/strings.xml @@ -272,6 +272,7 @@ Versione stabile Installa Scaricati %1$s di %2$s + Annulla download Installazione… Installato Riavvia diff --git a/manager/src/main/res/values-iw/strings.xml b/manager/src/main/res/values-iw/strings.xml index 77211ef52..ed3593d79 100644 --- a/manager/src/main/res/values-iw/strings.xml +++ b/manager/src/main/res/values-iw/strings.xml @@ -298,6 +298,7 @@ גרסה יציבה התקנה הורדו %1$s מתוך %2$s + ביטול ההורדה מתקין… הותקן הפעלה מחדש diff --git a/manager/src/main/res/values-ja/strings.xml b/manager/src/main/res/values-ja/strings.xml index 69debcc27..908057435 100644 --- a/manager/src/main/res/values-ja/strings.xml +++ b/manager/src/main/res/values-ja/strings.xml @@ -261,6 +261,7 @@ 正式版 インストール %2$s 中 %1$s をダウンロード中 + ダウンロードをキャンセル インストール中… インストールしました 再起動 diff --git a/manager/src/main/res/values-ko/strings.xml b/manager/src/main/res/values-ko/strings.xml index 950099d6b..3c833f8f3 100644 --- a/manager/src/main/res/values-ko/strings.xml +++ b/manager/src/main/res/values-ko/strings.xml @@ -261,6 +261,7 @@ 정식 버전 설치 %2$s 중 %1$s 내려받는 중 + 내려받기 취소 설치하는 중… 설치했습니다 다시 시작 diff --git a/manager/src/main/res/values-pl/strings.xml b/manager/src/main/res/values-pl/strings.xml index d0679bc83..60e8cf593 100644 --- a/manager/src/main/res/values-pl/strings.xml +++ b/manager/src/main/res/values-pl/strings.xml @@ -294,6 +294,7 @@ Wydanie stabilne Zainstaluj Pobrano %1$s z %2$s + Anuluj pobieranie Instalowanie… Zainstalowano Uruchom ponownie diff --git a/manager/src/main/res/values-pt-rBR/strings.xml b/manager/src/main/res/values-pt-rBR/strings.xml index e8fcdc7fc..0020a5c80 100644 --- a/manager/src/main/res/values-pt-rBR/strings.xml +++ b/manager/src/main/res/values-pt-rBR/strings.xml @@ -272,6 +272,7 @@ Versão estável Instalar Baixados %1$s de %2$s + Cancelar download Instalando… Instalado Reiniciar diff --git a/manager/src/main/res/values-ru/strings.xml b/manager/src/main/res/values-ru/strings.xml index 5d5cd214b..0d9b94e4d 100644 --- a/manager/src/main/res/values-ru/strings.xml +++ b/manager/src/main/res/values-ru/strings.xml @@ -275,6 +275,7 @@ Релиз Установить Загружено %1$s из %2$s + Отменить загрузку Установка… Установлено Перезагрузить diff --git a/manager/src/main/res/values-tr/strings.xml b/manager/src/main/res/values-tr/strings.xml index a6c345f6f..c63d61ff3 100644 --- a/manager/src/main/res/values-tr/strings.xml +++ b/manager/src/main/res/values-tr/strings.xml @@ -272,6 +272,7 @@ Kararlı sürüm Kur %2$s içinden %1$s indirildi + İndirmeyi iptal et Kuruluyor… Kuruldu Yeniden başlat diff --git a/manager/src/main/res/values-uk/strings.xml b/manager/src/main/res/values-uk/strings.xml index 62a505a90..d302bd027 100644 --- a/manager/src/main/res/values-uk/strings.xml +++ b/manager/src/main/res/values-uk/strings.xml @@ -294,6 +294,7 @@ Стабільний випуск Встановити Завантажено %1$s з %2$s + Скасувати завантаження Встановлення… Встановлено Перезавантажити diff --git a/manager/src/main/res/values-vi/strings.xml b/manager/src/main/res/values-vi/strings.xml index a166a5853..bb3d08750 100644 --- a/manager/src/main/res/values-vi/strings.xml +++ b/manager/src/main/res/values-vi/strings.xml @@ -261,6 +261,7 @@ Bản chính thức Cài đặt Đã tải %1$s trên %2$s + Huỷ tải xuống Đang cài… Đã cài Khởi động lại diff --git a/manager/src/main/res/values-zh-rCN/strings.xml b/manager/src/main/res/values-zh-rCN/strings.xml index a3adf7306..ceb86b4e2 100644 --- a/manager/src/main/res/values-zh-rCN/strings.xml +++ b/manager/src/main/res/values-zh-rCN/strings.xml @@ -262,6 +262,7 @@ 正式版 安装 正在下载 %1$s / %2$s + 取消下载 正在安装… 已安装 重启 diff --git a/manager/src/main/res/values-zh-rTW/strings.xml b/manager/src/main/res/values-zh-rTW/strings.xml index 23862e712..768c2fe59 100644 --- a/manager/src/main/res/values-zh-rTW/strings.xml +++ b/manager/src/main/res/values-zh-rTW/strings.xml @@ -262,6 +262,7 @@ 正式版 安裝 正在下載 %1$s / %2$s + 取消下載 正在安裝… 已安裝 重新啟動 diff --git a/manager/src/main/res/values/strings.xml b/manager/src/main/res/values/strings.xml index 9a2c00eb8..d9903abae 100644 --- a/manager/src/main/res/values/strings.xml +++ b/manager/src/main/res/values/strings.xml @@ -393,6 +393,7 @@ Release Install Downloading %1$s of %2$s + Cancel download Installing… Installed Restart