Fix the long-press sheet's dropped actions and mismatched rows - #814
Merged
Conversation
Every row on PackageActionSheet goes through finish(), which dismisses the sheet and then launches the work on rememberCoroutineScope(). The dismissal sets menuOpen = false, the sheet leaves the composition on the next frame, and a scope the composition remembered is cancelled when it does — so the coroutine dies at the first withContext hop inside DaemonClient, usually before the binder transaction is made. Nothing is logged, onResult never runs, and the button did nothing. Whether the work beats the frame is a race, which is why it worked often enough to look flaky rather than broken. The daemon was never involved. In the reporter's logs every start that reached the ActivityManager returned 0, and the manager logged nothing at all across two minutes of pressing — neither the "refused by the activity manager" line nor the "row had resolved a target" one, and a real failure prints one of them. The sheet's actions now run on ServiceLocator.appScope, which belongs to the process, with Dispatchers.Main to keep onResult on the thread the composition scope resumed on. This was never specific to "Open companion app": app info, force stop, re-optimize, uninstall and the framework's soft reboot were all launched the same way. The Scope screen's own companion button was unaffected — it goes through ScopeViewModel on viewModelScope. Fixes#810
Three things were off on that sheet, and each came from a row borrowing its shape from somewhere else. A Material list item paints its container `surface`, while a ModalBottomSheet is drawn on `surfaceContainerLow`, a shade darker. On a screen those two agree, so the default looks right in the place a row is usually written and wrong the moment it is put in a sheet — a pale full-width band across the sheet, ending wherever the row ends. Every list item on a sheet is now given the transparent `sheetRowColors`, which takes whatever it is placed on: the mute switch here, the log settings sheet, the batch update sheet, the store's asset picker and the framework versions sheet. ScopeScreen's AppRow had already worked this out for itself and nothing had carried it across. The mute switch was also the only row here built from the generic ToggleRow, so its icon had no disc and its title started seventy pixels to the left of every other row. It now takes the sheet's own shape, factored out of ActionRow as ActionRowLayout with a trailing slot, with the click behaviour arriving through the modifier so a switch row can still announce itself to a screen reader as a switch rather than as a button. The gap after the disc goes from 18dp to 20dp, which puts every title on 84dp — where the header already puts the app's name over its 44dp icon. Uninstall was drawn with DeleteOutline, a stroked glyph among filled ones, here and again in the module list's selection bar beside CheckCircle and SaveAlt. One verb, one glyph: Delete in all three places. Last, "not in the store" is a statement rather than an action, and it rippled under a thumb and then did nothing. ActionRow's onClick is nullable now and that row passes null.
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two things about the long-press sheet on the modules list, one a bug and one the look of it.
"Open companion app" fails silently and intermittently, and so does every other row. finish() dismisses the sheet before it starts working and launches on rememberCoroutineScope(), so the sheet leaves the composition on the next frame and takes that scope with it — the daemon call is cancelled at its first withContext hop, usually before the transaction is made. No transaction, no error branch, no snackbar, and a race against the frame rather than a reliable failure, which is what made it look flaky. The logs on #810 agree: six starts reached the ActivityManager and all six returned 0, while the manager logged nothing across the window the reporter was pressing. Both failure paths log, so the presses that failed never got that far. The work now runs on ServiceLocator.appScope, which outlives the sheet, with Dispatchers.Main so onResult still reaches the snackbar on the UI thread. App info, force stop, re-optimize, uninstall and soft reboot were all affected; the Scope screen's companion button was not, since it goes through viewModelScope.
The sheet also did not read as one list. The mute switch was the only row built from the generic ToggleRow, so it had no icon disc, its title sat seventy pixels left of every other row, and it painted a pale band across the sheet — a Material list item's container is surface, a sheet is surfaceContainerLow. Uninstall used a stroked glyph among filled ones, and "not in the store" rippled under a thumb while doing nothing. The transparent-container fix goes to every list item on a sheet, not only this one: log settings, batch updates, the store's asset picker and the framework versions sheet had the same band.
Fixes#810