DON-2008 PoC for hiding resource icons #2428
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a proof-of-concept for hiding resource icons by extracting icon resources into a separate internal module. The goal is to prevent direct access to icon drawables while maintaining icon functionality through the BpkIcon API.
Key changes include:
- Created a new
backpack-internal-iconsmodule containing icon resources - Moved icon-related classes from
backpack-composetobackpack-common - Updated icon token generation to reference the internal module
- Refactored import statements across the codebase to use new icon packages
Reviewed Changes
Copilot reviewed 176 out of 753 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| backpack-internal-icons/build.gradle | New module build configuration for internal icon resources |
| backpack-compose/tokens.gradle.kts | Updated token generation to reference internal icons module |
| backpack-common/tokens.gradle.kts | Modified to generate icons in internal module and reference them |
| backpack-common/src/main/kotlin/net/skyscanner/backpack/icon/BpkIcon.kt | New core icon classes moved from compose module |
| backpack-common/src/main/kotlin/net/skyscanner/backpack/icon/tokens/BpkIcon.kt | Updated icon token generation with new package structure |
| Multiple compose files | Updated imports to use new icon package structure |
| Test files | Updated icon resource references to use internal module |
Files not reviewed (2)
- .idea/copilot.data.migration.agent.xml: Language not supported
- .idea/copilot.data.migration.edit.xml: Language not supported
| // Note: This module is intentionally NOT published as it contains internal icon resources | ||
| // that should only be accessed through the BpkIcon API in backpack-common |
There was a problem hiding this comment.
Consider adding more detailed documentation about the module's purpose and how it relates to the overall architecture. The comment should explain the security/encapsulation benefits and provide guidance for developers on proper icon usage.
| // Note: This module is intentionally NOT published as it contains internal icon resources | |
| // that should only be accessed through the BpkIcon API in backpack-common | |
| /* | |
| * Module purpose: This module contains internal icon resources used by Backpack for Android. | |
| * | |
| * Architectural context: These icons are not intended for direct use by application code or external modules. | |
| * Instead, they are encapsulated within this module and are only exposed through the public BpkIcon API | |
| * provided by the backpack-common module. This design ensures that icon usage remains consistent across | |
| * the codebase and that any updates or changes to icon assets are centrally managed. | |
| * | |
| * Security/encapsulation benefits: By not publishing this module and restricting direct access to its resources, | |
| * we prevent accidental or unauthorized usage of internal icons. This encapsulation reduces the risk of | |
| * resource misuse, enforces design consistency, and simplifies maintenance. | |
| * | |
| * Developer guidance: Do NOT access icon resources in this module directly. Always use the BpkIcon API | |
| * in backpack-common to reference and use icons in your application code. If you need a new icon or | |
| * modification, follow the established contribution process and update the BpkIcon API accordingly. | |
| */ |
Copilot uses AI. Check for mistakes.
|
|
||
| val tokensPackage = "net.skyscanner.backpack.compose.tokens" | ||
| val rClass = ClassName("net.skyscanner.backpack.compose", "R") | ||
| val rClass = ClassName("net.skyscanner.backpack.internal.icons", "R") |
There was a problem hiding this comment.
The R class reference is hardcoded. Consider extracting this to a variable or configuration to make it easier to maintain if the internal module name changes in the future.
| val rClass = ClassName("net.skyscanner.backpack.internal.icons", "R") | |
| val rClassPackage = "net.skyscanner.backpack.internal.icons" | |
| val rClass = ClassName(rClassPackage, "R") |
Copilot uses AI. Check for mistakes.
| .parseAs(BpkIcon.Parser.Svg) | ||
| .transformTo(BpkIcon.Format.Xml(rootDir, metadata)) | ||
| .saveTo(BpkOutput.XmlIconFiles(iconSrc)) | ||
| .saveTo(BpkOutput.XmlIconFiles(project.projectDir.resolve("../backpack-internal-icons/src/main/res/drawable-nodpi").path)) |
There was a problem hiding this comment.
The relative path '../backpack-internal-icons/src/main/res/drawable-nodpi' is fragile and could break if the project structure changes. Consider using a more robust path resolution approach or extracting this to a configuration variable.
| .saveTo(BpkOutput.XmlIconFiles(project.projectDir.resolve("../backpack-internal-icons/src/main/res/drawable-nodpi").path)) | |
| .saveTo(BpkOutput.XmlIconFiles(project(":backpack-internal-icons").projectDir.resolve("src/main/res/drawable-nodpi").path)) |
Copilot uses AI. Check for mistakes.
| */ | ||
|
|
||
| package net.skyscanner.backpack.compose.utils | ||
| package net.skyscanner.backpack.util |
There was a problem hiding this comment.
The package name has changed from net.skyscanner.backpack.compose.utils to net.skyscanner.backpack.util. Ensure all references to the old package are updated throughout the codebase to prevent import issues.
Copilot uses AI. Check for mistakes.
Remember to include the following changes:
README.mdIf you are curious about how we review, please read through the code review guidelines