[TrimmableTypeMap] Fix manifest builder gaps: SupportsGLTexture, UsesPermissionFlags, RoundIcon, dedup#11044
[TrimmableTypeMap] Fix manifest builder gaps: SupportsGLTexture, UsesPermissionFlags, RoundIcon, dedup#11044simonrozsival wants to merge 5 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Closes feature gaps in the TrimmableTypeMap assembly-level manifest generation path so it matches the legacy ManifestDocument behavior (adds missing attribute scanning/emission and improves template dedup/perf).
Changes:
- Add scanning + manifest emission for
SupportsGLTexture,UsesPermissionFlags, and permissionRoundIconattributes. - Add assembly-level attribute filtering (
KnownAssemblyAttributes) to reduce scanning overhead. - Update unit tests/fixtures to cover new assembly-attribute scanning and manifest generation.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/TestFixtures/StubAttributes.cs | Adds stub attribute types used by scanning tests (UsesFeature/Permission/Library/MetaData/IntentFilter/SupportsGLTexture). |
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/TestFixtures/AssemblyAttributes.cs | Adds assembly-level attributes for scanner fixture coverage. |
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Scanner/AssemblyAttributeScanningTests.cs | New tests for assembly-level attribute scanning (including UsesPermissionFlags + SupportsGLTexture). |
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/ManifestGeneratorTests.cs | Updates generator test helper to pass preloaded template; adds tests for new manifest emissions. |
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/FixtureTestBase.cs | Caches both peers + scanned AssemblyManifestInfo for reuse across tests. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/UsesPermissionInfo.cs | Adds UsesPermissionFlags support to uses-permission model. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/UsesLibraryInfo.cs | Model for <uses-library>. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/UsesFeatureInfo.cs | Model for <uses-feature> including GLESVersion/Required. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/UsesConfigurationInfo.cs | Model for <uses-configuration>. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/SupportsGLTextureInfo.cs | Model for <supports-gl-texture>. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/PropertyInfo.cs | Model for <property>. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/PermissionTreeInfo.cs | Model for <permission-tree>. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/PermissionInfo.cs | Model for <permission>. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/PermissionGroupInfo.cs | Model for <permission-group>. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/MetaDataInfo.cs | Model for <meta-data> (record with required Name). |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/JavaPeerScanner.cs | Adds ScanAssemblyManifestInfo() and wires per-type ComponentAttribute into peers. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/JavaPeerInfo.cs | Clarifies comment for ComponentAttribute. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/IntentFilterInfo.cs | Model for intent filters (actions/categories/properties). |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/ComponentInfo.cs | Public component attribute model used for manifest generation. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/AssemblyManifestInfo.cs | Aggregates assembly-level manifest info lists + application properties. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/AssemblyIndex.cs | Implements assembly attribute scanning, known-attribute guard, and intent-filter/meta-data ordering fix. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/ManifestModel.cs | Removes legacy manifest model types (replaced by scanner records). |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/ManifestGenerator.cs | Changes Generate() to accept optional preloaded template XDocument and refactors default manifest creation. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/AssemblyLevelElementBuilder.cs | Emits new attributes/elements (RoundIcon, UsesPermissionFlags, supports-gl-texture) and adds dedup checks. |
tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/FixtureTestBase.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/JavaPeerScanner.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/AssemblyLevelElementBuilder.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/AssemblyLevelElementBuilder.cs
Show resolved
Hide resolved
eadfbb3 to
ab21c88
Compare
- Add SupportsGLTextureAttribute scanning + <supports-gl-texture> emission - Add UsesPermissionFlags to UsesPermissionInfo + android:usesPermissionFlags emission - Add RoundIcon mapping for <permission>, <permission-group>, <permission-tree> - Add TODO comment for deferred BackupAgent/ManageSpaceActivity emission - Add scanner + generator tests for all three gaps - Add stub attributes + assembly-level usages in TestFixtures Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add KnownAssemblyAttributes guard to skip non-manifest attributes - Add dedup for <permission-group> and <permission-tree> against template - Remove redundant #nullable enable from AssemblyLevelElementBuilder.cs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the TODO with actual implementation: resolve managed type name strings from ApplicationProperties to JNI names via the scanned peers list, and emit android:backupAgent / android:manageSpaceActivity on the <application> element. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Dispose JavaPeerScanner in FixtureTestBase after scan completes - Remove unused typeDef/index params from ToComponentInfo - Use HashSet.Add() pattern for all dedup sets so cross-assembly duplicates are also prevented (not just template duplicates) - Fix squashed closing brace in permission-group block Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ab21c88 to
2b54030
Compare
simonrozsival
left a comment
There was a problem hiding this comment.
🤖 AI Review Summary
Verdict:
Found 1 issue (1 warning):
⚠️ Error handling:ApplyTypePropertysilently does nothing when the managed type cannot be resolved to a Java peer (AssemblyLevelElementBuilder.cs:207-212)
👍 Good changes overall:
HashSet.Add()pattern for dedup is correct per Postmortem#41KnownAssemblyAttributesHashSet guard is a smart perf optimization to skip non-manifest attributes earlySupportsGLTextureandUsesPermissionFlagsfollow existing patterns well- Proper
using var scannerin test base - Assembly-qualified name stripping handles "TypeName, AssemblyName" format correctly
Review generated by android-reviewer from review guidelines.
| app.SetAttributeValue (AndroidNs + xmlAttrName, peer.JavaName.Replace ('/', '.')); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🤖 foreach loop ends without finding a peer matching managedName, the method silently returns. The user's BackupAgent or ManageSpaceActivity attribute is ignored with no diagnostic. This is hard to debug — the manifest will be generated without android:backupAgent / android:manageSpaceActivity and the user won't know why.
Consider logging a warning when the type cannot be resolved:
foreach (var peer in allPeers) {
if (peer.ManagedTypeName == managedName) {
app.SetAttributeValue (AndroidNs + xmlAttrName, peer.JavaName.Replace ('/', '.'));
return;
}
}
// Type not found — user misconfiguration or missing assembly
log?.Invoke ($"Could not resolve {propertyName} type '{managedName}' to a Java peer for android:{xmlAttrName}.");This would require adding a logging callback to the method (or using the builder's existing log mechanism).
Rule: Log messages must have context (Postmortem #6)
Add warning callback to ApplyTypeProperty so users get diagnostic output when a managed type name cannot be resolved to a Java peer for android:backupAgent or android:manageSpaceActivity attributes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Fix gaps in the assembly-level manifest builder compared to the old
ManifestDocumentpath.Changes
Scanner
SupportsGLTextureInforecord andSupportsGLTextureAttributehandling inScanAssemblyAttributesUsesPermissionFlagsproperty toUsesPermissionInfo, extract in scannerParseNameAndPropertieswithKnownAssemblyAttributesHashSet to skip non-manifest attributes earlyGenerator
<supports-gl-texture>element inAssemblyLevelElementBuilderandroid:usesPermissionFlagsattribute on<uses-permission>android:roundIconmapping for<permission>,<permission-group>, and<permission-tree>elementsandroid:backupAgent/android:manageSpaceActivityon<application><permission-group>and<permission-tree>against manifest template (matching existing<permission>and<uses-permission>dedup)Cleanup
#nullable enablefromAssemblyLevelElementBuilder.csJavaPeerScannerinFixtureTestBaseafter scan completesToComponentInfoHashSet.Add()pattern for all dedup sets to prevent cross-assembly duplicatesTests
SupportsGLTexture_ConstructorArg,UsesPermission_FlagsAssemblyLevel_SupportsGLTexture,AssemblyLevel_UsesPermissionFlags,AssemblyLevel_PermissionRoundIcon