fix: apps screen a11y state description and pending-install button#486
Conversation
Updates `ShizukuServiceManager` to recognize Sui as a valid service provider alongside the standard Shizuku implementation. The `computeStatus` logic is refactored to prioritize binder connectivity; if the binder is unresponsive, the manager now checks for the presence of either the Shizuku or Sui packages to accurately differentiate between `NOT_RUNNING` and `NOT_INSTALLED` states. * Added `com.sui` package constant. * Introduced `isPackageInstalled` helper to check for specific package availability. * Renamed `isShizukuInstalled` to `isShizukuOrSuiInstalled` to reflect broader compatibility. * Restructured `computeStatus` to reduce redundant checks when the service is already active.
WalkthroughThis PR contains two independent changes: refactoring Shizuku service status computation to distinguish between not-installed and not-running states when the binder is dead, and updating the apps section UI with localized accessibility labels and simplified busy state logic. ChangesShizuku Service Status Distinction
Apps Section UI Localization and Logic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/presentation/src/commonMain/composeResources/values/strings.xml`:
- Around line 961-962: The new accessibility string keys
apps_section_state_expanded and apps_section_state_collapsed were only added to
the default values/strings.xml; update every existing locale-specific strings
file (e.g., values-es, values-fr, values-ja, values-zh-rCN, etc.) to include
these two keys with correct translated text so non-English TalkBack announces
localized “expanded”/“collapsed” instead of falling back to English — add the
same string names (apps_section_state_expanded, apps_section_state_collapsed)
with appropriate translations in each locale file.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/components/CompactAppRow.kt`:
- Around line 94-97: Update the busy/interaction model to be consistent: make
AppsRoot.kt use the same isBusy logic as CompactAppRow.kt (remove
app.isPendingInstall from its isBusy calculation) OR revert CompactAppRow.kt
change so both files match; then explicitly guard the "Open" shortcut rendering
by checking the pending-install substate (use both isPendingInstall and
pendingInstallFilePath) instead of relying only on !isBusy — adjust the
conditional that currently shows Open when !isBusy to also require
pendingInstallFilePath != null (or explicitly permit Open only when not in the
pending-install-without-path state), and update StatusDotCluster-related checks
if needed so StatusDotCluster, CompactAppRow (isBusy), and AppsRoot (isBusy)
share the same semantics for isPendingInstall and pendingInstallFilePath.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1b8aa7a-d565-471b-939d-ff8ba113d003
📒 Files selected for processing (4)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.ktcore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/components/AppsSectionHeader.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/components/CompactAppRow.kt
| <string name="apps_section_state_expanded">Expanded</string> | ||
| <string name="apps_section_state_collapsed">Collapsed</string> |
There was a problem hiding this comment.
New a11y strings are absent from all language-specific resource files — non-English TalkBack still announces English text
The PR's stated goal is that "screen readers in non-English locales no longer announce English 'expanded' / 'collapsed'". Adding keys only to the default values/strings.xml enables future translation without a code change, but until the entries are also added to the locale-specific files (values-es, values-fr, values-ja, values-zh-rCN, etc.), those locales fall back to the default English values — the same outcome as the old hard-coded strings.
The two entries need to be added (with appropriate translations) to every existing language-specific strings file to actually fulfill the PR objective.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/presentation/src/commonMain/composeResources/values/strings.xml` around
lines 961 - 962, The new accessibility string keys apps_section_state_expanded
and apps_section_state_collapsed were only added to the default
values/strings.xml; update every existing locale-specific strings file (e.g.,
values-es, values-fr, values-ja, values-zh-rCN, etc.) to include these two keys
with correct translated text so non-English TalkBack announces localized
“expanded”/“collapsed” instead of falling back to English — add the same string
names (apps_section_state_expanded, apps_section_state_collapsed) with
appropriate translations in each locale file.
| val isBusy = | ||
| app.isPendingInstall || | ||
| appItem.updateState is UpdateState.Downloading || | ||
| appItem.updateState is UpdateState.Downloading || | ||
| appItem.updateState is UpdateState.Installing || | ||
| appItem.updateState is UpdateState.CheckingUpdate |
There was a problem hiding this comment.
isBusy is now inconsistent with AppsRoot.kt, and the "Open" shortcut silently appears for isPendingInstall=true, pendingInstallFilePath=null
The intended fix (enable the Install button when pendingInstallFilePath != null) is correct — the Install button only renders inside if (app.pendingInstallFilePath != null), so removing isPendingInstall from isBusy is safe for that branch.
However, two knock-on effects need attention:
-
Inconsistency with
AppsRoot.kt:762-765, which still reads:val isBusy = app.isPendingInstall || appItem.updateState is UpdateState.Downloading || ...
The same app entry in the full card disables interactions when
isPendingInstall=true, but the compact row now enables them. If removingisPendingInstallfromisBusyis the correct model,AppsRoot.ktshould be updated to match. -
"Open" shortcut appears for
isPendingInstall=true, pendingInstallFilePath=null— the sub-state where the system installer was triggered but no file path is present (seeStatusDotCluster.ktsnippet:pendingInstall = isPendingInstall && pendingInstallFilePath == null). WithisBusy=false,else if (!isBusy)at line 175 now fires, surfacing the Open shortcut in a state it was previously hidden for. This may be intentional (the current app version is still openable), but it should be explicitly verified since it's an undocumented behavior change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/components/CompactAppRow.kt`
around lines 94 - 97, Update the busy/interaction model to be consistent: make
AppsRoot.kt use the same isBusy logic as CompactAppRow.kt (remove
app.isPendingInstall from its isBusy calculation) OR revert CompactAppRow.kt
change so both files match; then explicitly guard the "Open" shortcut rendering
by checking the pending-install substate (use both isPendingInstall and
pendingInstallFilePath) instead of relying only on !isBusy — adjust the
conditional that currently shows Open when !isBusy to also require
pendingInstallFilePath != null (or explicitly permit Open only when not in the
pending-install-without-path state), and update StatusDotCluster-related checks
if needed so StatusDotCluster, CompactAppRow (isBusy), and AppsRoot (isBusy)
share the same semantics for isPendingInstall and pendingInstallFilePath.
…-fixes # Conflicts: # core/presentation/src/commonMain/composeResources/values/strings.xml # feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/components/AppsSectionHeader.kt # feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/components/CompactAppRow.kt
1 participant
Summary
stateDescriptionso screen readers in non-English locales no longer hear English"expanded"/"collapsed". Addsapps_section_state_expanded/apps_section_state_collapsedresources.isPendingInstallis a quiescent "bytes parked on disk" flag, not an active-work flag — including it inisBusyblocked the very action the row is rendered to surface.Test plan
pendingInstallFilePath != null,isPendingInstall = true) shows the Install button enabled and the overflow menu enabled.Downloading/Installing/CheckingUpdatestill disable the action controls as before.Summary by CodeRabbit
Bug Fixes
New Features
Accessibility