ADFA-3260: Fix plugin resource and theme resolution for custom package IDs#1103
Open
Daniel-ADFA wants to merge 7 commits intostagefrom
Open
ADFA-3260: Fix plugin resource and theme resolution for custom package IDs#1103Daniel-ADFA wants to merge 7 commits intostagefrom
Daniel-ADFA wants to merge 7 commits intostagefrom
Conversation
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
…alParameters for package-id and allow-reserved-package-id
…alParameters for package-id and allow-reserved-package-id
… ID theme attributes Plugins with auto-assigned package IDs (0x02-0x7E) crashed on dark/light toggle because their bundled Material3 theme attributes weren't added to the plugin theme. Now applies the manifest-declared theme to bridge the gap.
Contributor
📝 WalkthroughRelease Notes - ADFA-3260: Plugin Resource and Theme Resolution for Custom Package IDsMajor Features & Improvements
Styling & Resource Updates
Plugin Version Updates
Technical Improvements
Risks & Best Practices Considerations
WalkthroughThe PR introduces Material Design 3 theming across multiple plugins, refactors the keystore generator fragment to use Material components and localized resources, implements native library extraction and loading in the plugin system with permission enforcement, and updates plugin infrastructure including new permission types and context management. Changes
Sequence Diagram(s)sequenceDiagram
participant PluginManager
participant PluginLoader as PluginLoader
participant PluginManifest as Plugin Manifest
participant FileSystem as File System
participant DexClassLoader
PluginManager->>PluginLoader: extractNativeLibs(pluginId)
PluginLoader->>FileSystem: createDir(plugin_native_libs/{pluginId})
PluginLoader->>PluginManifest: open plugin APK
PluginLoader->>PluginManifest: scan for .so files (native ABI)
PluginManifest-->>PluginLoader: .so file list
loop for each native library
PluginLoader->>FileSystem: extract .so to plugin dir
end
PluginLoader-->>PluginManager: nativeLibPath (or null)
alt Native libs extracted
PluginManager->>PluginManifest: check NATIVE_CODE permission
alt Permission granted
PluginManager->>PluginLoader: loadPluginClasses(nativeLibPath)
PluginLoader->>DexClassLoader: new DexClassLoader(nativeLibPath)
DexClassLoader-->>PluginLoader: classloader ready
PluginLoader-->>PluginManager: success
else Permission denied
PluginManager->>FileSystem: deleteRecursive(plugin_native_libs/{pluginId})
PluginManager-->>PluginManager: return SecurityException
end
else No native libs
PluginManager->>PluginLoader: loadPluginClasses(null)
PluginLoader-->>PluginManager: classloader ready
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
keystore-generator-plugin/src/main/res/layout/fragment_keystore_generator.xml (1)
269-271: Optional: avoid fixed bottom spacer view.Line 269 adds static whitespace that may be inconsistent across screen sizes/IME states. Prefer handling bottom space via parent/action-container padding or insets.
♻️ Optional cleanup
- <View - android:layout_width="match_parent" - android:layout_height="16dp" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keystore-generator-plugin/src/main/res/layout/fragment_keystore_generator.xml` around lines 269 - 271, The layout currently contains a fixed-height spacer View (the anonymous <View> at the bottom of fragment_keystore_generator.xml) which can cause inconsistent gaps across screens and IME states; remove that static View and instead provide bottom spacing via the parent/container (e.g., add paddingBottom to the root or action container) and handle system/IME insets (use WindowInsets or ViewCompat.setOnApplyWindowInsetsListener on the root or container) so the UI adapts to keyboard and different screen sizes while preserving the intended bottom gap.plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginResourceContext.kt (1)
42-63: Consider narrowing the exception handling.Per retrieved learnings, prefer narrow exception handling in Kotlin files across the AndroidIDE project. The current
catch (e: Exception)at line 59 catches all exceptions, which could mask unexpected errors.However, reflection and class loading operations can throw multiple exception types (
ClassNotFoundException,IllegalAccessException,NoSuchFieldException, etc.), so a pragmatic approach would be to catchReflectiveOperationExceptionwhich covers most reflection-related failures:Suggested refinement
- } catch (e: Exception) { + } catch (e: ReflectiveOperationException) { + Log.w(TAG, "Failed to detect custom package ID for $pkg", e) + } catch (e: ClassNotFoundException) { Log.w(TAG, "Failed to detect custom package ID for $pkg", e) }Based on learnings: "prefer narrow exception handling that catches only the specific exception type reported in crashes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginResourceContext.kt` around lines 42 - 63, The catch-all in detectCustomPackageId currently swallows all exceptions; narrow it to reflection-related failures by replacing catch (e: Exception) with catch (e: ReflectiveOperationException) (and optionally catch IllegalArgumentException if you expect invalid arg access) so only reflection/class-loading errors from pluginClassLoader.loadClass("$pkg.R") and subsequent field access are handled; keep the Log.w(TAG, "Failed to detect custom package ID for $pkg", e) behavior and leave other exceptions to propagate.plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/base/PluginFragment.kt (1)
96-97: Consider documenting thread-safety assumptions.These assignments are not atomic with respect to each other. While
@Volatileensures visibility, concurrent calls togetPluginInflatercould result inactivityContextRefandactivityThemeRefpointing to different activities momentarily.This is likely acceptable if:
getPluginInflateris only called from the main thread, or- Both refs always come from the same activity in practice.
A brief comment clarifying the expected threading model would help future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/base/PluginFragment.kt` around lines 96 - 97, The two WeakReference assignments (activityContextRef and activityThemeRef) in getPluginInflater are not atomic and can briefly point to different activities if called concurrently; add a concise comment near these assignments documenting the thread-safety assumption (e.g., getPluginInflater is expected to be called on the main/UI thread or that both refs always originate from the same Activity), and to harden the contract optionally annotate getPluginInflater with `@MainThread` (or synchronize the update) so future maintainers know the intended threading model; reference activityContextRef, activityThemeRef, getPluginInflater and defaultInflater in the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@keystore-generator-plugin/src/main/kotlin/com/appdevforall/keygen/plugin/fragments/KeystoreGeneratorFragment.kt`:
- Around line 267-272: The catch block in KeystoreGeneratorFragment currently
appends e.message which can be null and show literal "null" to users; change the
error message construction used with showError(...) to only append the exception
message when it's non-null/blank (e.g., use a safe fallback like e.message ?: ""
or conditional concatenation) so hideProgress(), btnGenerate.isEnabled = true
and showError(...) are still called but the displayed text never contains
"null".
In `@keystore-generator-plugin/src/main/res/values/strings.xml`:
- Line 46: Update the user-facing error string named error_no_build_gradle to
mention Kotlin DSL files as well (e.g., "build.gradle or build.gradle.kts") so
it reflects what the code checks; open the strings.xml entry for the string
resource error_no_build_gradle and change its text to include "build.gradle.kts"
(or equivalent phrasing) to avoid misleading users.
In
`@plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/base/PluginFragment.kt`:
- Line 98: The expression pluginContext.theme in PluginFragment is unused and
should be either removed or made explicit; either delete the dead expression or
use the theme value (for example assign it to a val or call a method like
applyStyle on it) so the access has an effect. Locate the occurrence in
PluginFragment (the pluginContext.theme expression) and replace the no-op access
with a meaningful operation (remove it if unnecessary, or store it in a variable
or invoke the intended theme method such as applyStyle) so the code is not a
discarded expression.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt`:
- Around line 315-330: The extracted native libs directory (nativeLibPath set
from pluginLoader.extractNativeLibs(manifest.id)) is left on disk when the
NATIVE_CODE permission check fails; update the permission-failure branch in
PluginManager (around the nativeLibPath check) to delete or clean up the
extracted native libs before returning the Result.failure — for example, if
nativeLibPath != null call the existing cleanup routine (cleanupPluginCacheFiles
or a new pluginLoader.deleteExtractedNativeLibs(manifest.id)) or delete the
File(nativeLibPath) and still release sidebar slots via
SidebarSlotManager.releasePluginSlots(manifest.id) before returning the
SecurityException. Ensure the cleanup is performed only when extraction
succeeded (nativeLibPath != null) and do not rely solely on
unloadPlugin/cleanupPluginCacheFiles which run only for successfully loaded
plugins.
In
`@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/pluginProject/pluginBuildGradle.kt`:
- Around line 56-58: The template currently hardcodes an APK package-id via the
aaptOptions block (additionalParameters("--package-id", "0x71",
"--allow-reserved-package-id")), which overrides
PluginBuilder.configurePackageId() auto-assignment and can cause resource ID
collisions; remove the entire aaptOptions block (the
additionalParameters("--package-id", "0x71", "--allow-reserved-package-id")
call) from pluginBuildGradle.kt so that PluginBuilder.configurePackageId() can
compute and assign unique package IDs per plugin.
---
Nitpick comments:
In
`@keystore-generator-plugin/src/main/res/layout/fragment_keystore_generator.xml`:
- Around line 269-271: The layout currently contains a fixed-height spacer View
(the anonymous <View> at the bottom of fragment_keystore_generator.xml) which
can cause inconsistent gaps across screens and IME states; remove that static
View and instead provide bottom spacing via the parent/container (e.g., add
paddingBottom to the root or action container) and handle system/IME insets (use
WindowInsets or ViewCompat.setOnApplyWindowInsetsListener on the root or
container) so the UI adapts to keyboard and different screen sizes while
preserving the intended bottom gap.
In
`@plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/base/PluginFragment.kt`:
- Around line 96-97: The two WeakReference assignments (activityContextRef and
activityThemeRef) in getPluginInflater are not atomic and can briefly point to
different activities if called concurrently; add a concise comment near these
assignments documenting the thread-safety assumption (e.g., getPluginInflater is
expected to be called on the main/UI thread or that both refs always originate
from the same Activity), and to harden the contract optionally annotate
getPluginInflater with `@MainThread` (or synchronize the update) so future
maintainers know the intended threading model; reference activityContextRef,
activityThemeRef, getPluginInflater and defaultInflater in the comment.
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginResourceContext.kt`:
- Around line 42-63: The catch-all in detectCustomPackageId currently swallows
all exceptions; narrow it to reflection-related failures by replacing catch (e:
Exception) with catch (e: ReflectiveOperationException) (and optionally catch
IllegalArgumentException if you expect invalid arg access) so only
reflection/class-loading errors from pluginClassLoader.loadClass("$pkg.R") and
subsequent field access are handled; keep the Log.w(TAG, "Failed to detect
custom package ID for $pkg", e) behavior and leave other exceptions to
propagate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 879422ae-7965-428e-af94-d0e371faa38d
📒 Files selected for processing (20)
apk-viewer-plugin/build.gradle.ktsapk-viewer-plugin/src/main/AndroidManifest.xmlkeystore-generator-plugin/build.gradle.ktskeystore-generator-plugin/src/main/AndroidManifest.xmlkeystore-generator-plugin/src/main/kotlin/com/appdevforall/keygen/plugin/fragments/KeystoreGeneratorFragment.ktkeystore-generator-plugin/src/main/res/layout/fragment_keystore_generator.xmlkeystore-generator-plugin/src/main/res/values-night/colors.xmlkeystore-generator-plugin/src/main/res/values/colors.xmlkeystore-generator-plugin/src/main/res/values/strings.xmlkeystore-generator-plugin/src/main/res/values/styles.xmlmarkdown-preview-plugin/build.gradle.ktsmarkdown-preview-plugin/src/main/AndroidManifest.xmlplugin-api/plugin-builder/src/main/kotlin/com/itsaky/androidide/plugins/build/PluginBuilder.ktplugin-api/src/main/kotlin/com/itsaky/androidide/plugins/IPlugin.ktplugin-api/src/main/kotlin/com/itsaky/androidide/plugins/base/PluginFragment.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginLoader.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/loaders/PluginResourceContext.kttemplates-impl/src/main/java/com/itsaky/androidide/templates/impl/pluginProject/PluginTemplateData.kttemplates-impl/src/main/java/com/itsaky/androidide/templates/impl/pluginProject/pluginBuildGradle.kt
...plugin/src/main/kotlin/com/appdevforall/keygen/plugin/fragments/KeystoreGeneratorFragment.kt
Show resolved
Hide resolved
plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/base/PluginFragment.kt
Outdated
Show resolved
Hide resolved
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt
Show resolved
Hide resolved
...s-impl/src/main/java/com/itsaky/androidide/templates/impl/pluginProject/pluginBuildGradle.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/base/PluginFragment.kt`:
- Around line 21-31: The current global WeakReference fields activityThemeRef
and activityContextRef (and their accessors
getCurrentActivityTheme/getCurrentActivityContext) must be replaced with a
plugin-scoped snapshot map to avoid cross-plugin/activity leaks and
interleaving: introduce a Map keyed by pluginId (e.g., activitySnapshots:
MutableMap<String, Pair<WeakReference<Context>,
WeakReference<Resources.Theme>>>) and update getPluginInflater()/inflation path
to store and read the pair under the current pluginId instead of writing the two
globals; remove or stop using activityThemeRef/activityContextRef and change the
accessors to look up by pluginId. Also ensure you clear
activitySnapshots[pluginId] in unregisterPluginContext() and call
activitySnapshots.clear() in clearAllContexts() so snapshots are removed on
unload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02e7d470-d908-4e8d-8f93-f1f1a5b4d503
📒 Files selected for processing (3)
keystore-generator-plugin/src/main/res/values/strings.xmlplugin-api/src/main/kotlin/com/itsaky/androidide/plugins/base/PluginFragment.ktplugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt
✅ Files skipped from review due to trivial changes (1)
- keystore-generator-plugin/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManager.kt
Comment on lines
+21
to
+31
| @Volatile | ||
| private var activityThemeRef: WeakReference<Resources.Theme>? = null | ||
|
|
||
| @Volatile | ||
| private var activityContextRef: WeakReference<Context>? = null | ||
|
|
||
| @JvmStatic | ||
| fun getCurrentActivityTheme(): Resources.Theme? = activityThemeRef?.get() | ||
|
|
||
| @JvmStatic | ||
| fun getCurrentActivityContext(): Context? = activityContextRef?.get() |
Contributor
There was a problem hiding this comment.
Don't cache the host Context/Theme as a single global snapshot.
getPluginInflater() now overwrites one process-wide pair on every inflate, so later lookups can resolve resources against the wrong activity when multiple plugin fragments/activities are alive. Because Context and Theme are stored in separate mutable refs, callers can also observe a mismatched pair during interleaved updates. This also leaves stale host state hanging around after unload until GC happens. Prefer scoping the snapshot by pluginId (and clearing it on unregister/clear) or passing the host activity/theme through the consuming API instead of exposing global mutable state.
💡 One way to scope the snapshot correctly
+ private data class ActivitySnapshot(
+ val contextRef: WeakReference<Context>,
+ val themeRef: WeakReference<Resources.Theme>,
+ )
+
- `@Volatile`
- private var activityThemeRef: WeakReference<Resources.Theme>? = null
-
- `@Volatile`
- private var activityContextRef: WeakReference<Context>? = null
+ private val activitySnapshots = mutableMapOf<String, ActivitySnapshot>()
`@JvmStatic`
- fun getCurrentActivityTheme(): Resources.Theme? = activityThemeRef?.get()
+ fun getCurrentActivityTheme(pluginId: String): Resources.Theme? =
+ activitySnapshots[pluginId]?.themeRef?.get()
`@JvmStatic`
- fun getCurrentActivityContext(): Context? = activityContextRef?.get()
+ fun getCurrentActivityContext(pluginId: String): Context? =
+ activitySnapshots[pluginId]?.contextRef?.get()
...
fun getPluginInflater(pluginId: String, defaultInflater: LayoutInflater): LayoutInflater {
val pluginContext = getPluginContext(pluginId) ?: return defaultInflater
- activityContextRef = WeakReference(defaultInflater.context)
- activityThemeRef = WeakReference(defaultInflater.context.theme)
+ activitySnapshots[pluginId] =
+ ActivitySnapshot(
+ contextRef = WeakReference(defaultInflater.context),
+ themeRef = WeakReference(defaultInflater.context.theme),
+ )
return defaultInflater.cloneInContext(pluginContext)
}Also clear activitySnapshots[pluginId] in unregisterPluginContext() and activitySnapshots.clear() in clearAllContexts().
Also applies to: 96-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/base/PluginFragment.kt`
around lines 21 - 31, The current global WeakReference fields activityThemeRef
and activityContextRef (and their accessors
getCurrentActivityTheme/getCurrentActivityContext) must be replaced with a
plugin-scoped snapshot map to avoid cross-plugin/activity leaks and
interleaving: introduce a Map keyed by pluginId (e.g., activitySnapshots:
MutableMap<String, Pair<WeakReference<Context>,
WeakReference<Resources.Theme>>>) and update getPluginInflater()/inflation path
to store and read the pair under the current pluginId instead of writing the two
globals; remove or stop using activityThemeRef/activityContextRef and change the
accessors to look up by pluginId. Also ensure you clear
activitySnapshots[pluginId] in unregisterPluginContext() and call
activitySnapshots.clear() in clearAllContexts() so snapshots are removed on
unload.
jatezzz
requested changes
Mar 23, 2026
| createDebugTask(target, extension) | ||
| createReleaseTask(target, extension) | ||
| configurePackageId(target) | ||
| createAssembleTask(target, extension, "debug") |
Collaborator
There was a problem hiding this comment.
Maybe you could use a enum structure for the variants?
| private fun detectCustomPackageId(): Boolean { | ||
| val cl = pluginClassLoader ?: return false | ||
| val pkg = pluginPackageInfo?.packageName ?: return false | ||
| try { |
Collaborator
There was a problem hiding this comment.
Is there any way to break this nested structure?
| t.logger.lifecycle("Plugin assembled: ${outputFile.absolutePath}") | ||
| } | ||
| }) | ||
| private fun generatePackageId(applicationId: String): Int { |
Collaborator
There was a problem hiding this comment.
Could you convert 0x7FFFFFFF, 0x7D and 0x02 into constants?
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This video demonstrates keystore generator using the Material3 custom theme
Screen.Recording.2026-03-21.at.19.39.09.mov