manager: optimized all libsu shell calls #2736
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes libsu shell calls by standardizing the shell API usage throughout the KernelSU manager application. The changes centralize shell configuration and remove the need for maintaining persistent shell instances.
- Replaced individual shell creation calls with centralized Shell configuration
- Removed persistent shell instances in favor of on-demand shell usage
- Simplified shell command execution by using the default Shell.cmd() API
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| WebViewInterface.kt | Replaced StringBuilder with buildString for command construction |
| WebUIActivity.kt | Simplified shell instance management and removed manual cleanup |
| SuperUserViewModel.kt | Replaced custom KsuCli.SHELL with standard Shell.getShell() |
| LogEvent.kt | Converted from job-based commands to direct Shell.cmd() calls |
| KsuCli.kt | Removed KsuCli singleton, simplified shell creation, and standardized command execution |
| MainActivity.kt | Used direct packageName instead of ksuApp.packageName |
| KernelSUApplication.kt | Added default shell builder configuration during app initialization |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| val moduleId = intent.getStringExtra("id")!! | ||
| val name = intent.getStringExtra("name")!! | ||
| val moduleId = intent.getStringExtra("id") ?: run { finishAndRemoveTask(); return } | ||
| val name = intent.getStringExtra("name") ?: run { finishAndRemoveTask(); return } |
There was a problem hiding this comment.
[nitpick] The elvis operator with run block containing return is less readable than traditional null checks. Consider using early return with if-null checks or throwing an exception for missing required intent extras.
| val name = intent.getStringExtra("name") ?: run { finishAndRemoveTask(); return } | |
| val name = intent.getStringExtra("name") | |
| if (name == null) { | |
| finishAndRemoveTask() | |
| return | |
| } |
| override fun onDestroy() { | ||
| super.onDestroy() | ||
| runCatching { rootShell?.close() } | ||
| rootShell.runCatching { close() } |
There was a problem hiding this comment.
The runCatching block should handle the shell instance safely. Consider using 'runCatching { rootShell.close() }' instead to avoid potential issues if rootShell becomes null.
| rootShell.runCatching { close() } | |
| runCatching { rootShell.close() } |
| createRootShellBuilder(globalMnt).build() | ||
| }.getOrElse { e -> | ||
| Log.w(TAG, "su failed: ", e) | ||
| Shell.Builder.create().apply { |
There was a problem hiding this comment.
[nitpick] The fallback shell creation logic should be consistent with the primary shell builder. Consider using createRootShellBuilder(globalMnt).setFlags() to maintain consistency.
| Shell.Builder.create().apply { | |
| createRootShellBuilder(globalMnt).apply { |
2 participants
#2731