Adaptive icon support for APKs #435
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #435 +/- ##
==========================================
- Coverage 81.13% 80.66% -0.47%
==========================================
Files 158 159 +1
Lines 12900 13219 +319
Branches 1304 1378 +74
==========================================
+ Hits 10466 10663 +197
- Misses 1964 2038 +74
- Partials 470 518 +48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| items: list[GradientItem] | None = None | ||
|
|
||
|
|
||
| # TODO: Base abstraction to use for proto |
There was a problem hiding this comment.
Try to attach a Linear ticket if you can 😉
| with open(xml_file_path, "rb") as f: | ||
| vector_buffer = f.read() | ||
| vector_node = AndroidBinaryParser(vector_buffer).parse_xml() | ||
| if not vector_node: |
There was a problem hiding this comment.
Hmm def parse_xml(self) -> XmlNode doesn't return an optional and internally already raises a ValueError for this case, surprised this didn't trigger a type error.
|
|
||
| if not foreground_path and not background_path: | ||
| logger.warning( | ||
| "Could not resolve resource paths\nforeground ref: %s\nbackground ref: %s", |
There was a problem hiding this comment.
nit: would just pass the logger args as extras
| self, | ||
| root_element: XmlNode, | ||
| ) -> bytes | None: | ||
| try: |
There was a problem hiding this comment.
nit: I think all these extra try/except's per method are redundant since everything is already caught in render_from_path()
| ) -> bytes | None: | ||
| try: | ||
| # Load background layer | ||
| background_img = None |
There was a problem hiding this comment.
nit: there's probably a nice pythonic way of handling this background_img and foreground_img code but not really sure lol, maybe a nested method would help but up to you
| return None | ||
|
|
||
| # Use standard adaptive icon size (108x108 with 72x72 safe area) | ||
| size = 108 |
There was a problem hiding this comment.
I see this 108 is duplicated in a few spots, maybe extract into a constant?
|
|
||
| # Helper methods | ||
|
|
||
| def _get_optional_attr_value( |
There was a problem hiding this comment.
Could any of this be pushed down into our other XML code?
|
|
||
| binary_xml_drawable_utils = BinaryXmlDrawableParser(self._extract_dir, binary_res_tables) | ||
|
|
||
| icon = binary_xml_drawable_utils.render_from_path(icon_path) |
There was a problem hiding this comment.
I recommend adding a test case for get_app_icon () like I did for the iOS app icon parsing which basically just checks whether this creates a valid PNG image from our HackerNews fixture.
| self.extract_dir = extract_dir | ||
| self.binary_res_tables = binary_res_tables | ||
|
|
||
| # TODO: Just render the XML file, don't care if it's a vector, shape, or adaptive icon |
There was a problem hiding this comment.
is this a TODO or just explaining what this method does?
After reading it, it seems this only parses vectors, so this is a to-do to parse adaptive icons and shapes?
There was a problem hiding this comment.
A todo I forgot to remove
| logger.exception("Error parsing gradient XML") | ||
| return None | ||
|
|
||
| def _render_shape_to_buffer( |
There was a problem hiding this comment.
where is this called? 🤔
| logger.exception("Error parsing adaptive icon binary XML") | ||
| return None | ||
|
|
||
| def _render_standard_icon( |
There was a problem hiding this comment.
i think this assumes we are passing in a vector xml node so should the method be called render_vector?
|
Amazingly fast turnaround with that fancy AI! |
rbro112
force-pushed
the
ryan/adaptive_icon_support_for_apks
branch
from
89721af to
a87abdf
Compare
Adds adaptive icon support for XML icons from APKs. Admittedly used claude to migrate this from existing typescript code but rearchitected quite a bit. Checked work against quite a few local apps, all working well.