Refresh tracker icon assets #298
Conversation
Change-Id: I41d4376a1da53b1ed100c6ff9dccded0ee483877
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @alabiaga, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
As gemini-code-assist, I've reviewed this pull request. Based on the title "Refresh tracker icon assets" and the changes presented, this PR introduces new vector drawable icon assets intended for use within the live-updates sample user interface. The changes consist of adding three new XML files defining these icons.
Highlights
- New Icon Assets Added: This pull request adds three new Android Vector Drawable XML files to the
samples/user-interface/live-updates/src/main/res/drawable/directory. These files define icons for a check circle, a delivery truck, and a shopping bag. - Vector Drawable Format: The new icons are defined using the
<vector>XML format, which allows them to scale cleanly without pixelation across different screen densities.
Changelog
- samples/user-interface/live-updates/src/main/res/drawable/check_circle.xml
- Added a new vector drawable XML file defining a check circle icon.
- samples/user-interface/live-updates/src/main/res/drawable/delivery_truck.xml
- Added a new vector drawable XML file defining a delivery truck icon.
- samples/user-interface/live-updates/src/main/res/drawable/shopping_bag.xml
- Added a new vector drawable XML file defining a shopping bag icon.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Vector paths unfold,
Icons sharp on screens so bright,
XML draws the way.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces three new vector drawable assets for tracker icons: check_circle.xml, delivery_truck.xml, and shopping_bag.xml. The addition of these assets is clear.
My review has identified a critical issue in the pathData for the background elements in delivery_truck.xml and shopping_bag.xml, which will likely cause rendering errors. Additionally, there's a medium-severity concern regarding the use of hardcoded colors across all three new drawables, which could be improved for better maintainability by using color resources.
Details and suggestions are provided in the specific comments.
Summary of Findings
- Malformed Vector Path Data: A critical issue was found in
delivery_truck.xmlandshopping_bag.xml. ThepathDatafor their background rounded rectangles uses an invalidsweep-flagvalue of40in the arc commands. This flag must be0or1and will likely cause rendering errors. - Hardcoded Colors in Drawables: A medium severity issue: all three new vector drawables use hardcoded hex color values (e.g.,
#9d75b1,#000000). It's recommended to define these colors inres/values/colors.xmland reference them in the drawables for better maintainability, consistency, and easier theming.
Merge Readiness
Due to the critical issue identified with the malformed pathData in delivery_truck.xml and shopping_bag.xml, I recommend that these changes not be merged until this is corrected, as it will likely lead to runtime errors or visual defects when these drawables are used. Addressing the suggestions for using color resources would also improve the overall quality and maintainability of the codebase.
I am unable to approve this pull request directly; please ensure the critical issue is resolved and consider the other feedback before seeking further reviews and merging.
|
|
||
| <path | ||
| android:fillColor="#9d75b1" | ||
| android:pathData="M0,160 A160,160 0 0,40 160,0 H800 A160,160 0 0,40 960,160 V800 A160,160 0 0,40 800,960 H160 A160,160 0 0,40 0,800 Z" /> |
There was a problem hiding this comment.
The pathData for this rounded rectangle background appears to be malformed. The arc command A (e.g., in A160,160 0 0,40 160,0) uses a sweep-flag value of 40 (the second number in 0,40). However, the sweep-flag must be either 0 or 1.
This will likely lead to parsing errors or incorrect rendering of the drawable. Assuming the intention is a standard rounded rectangle similar to the one in check_circle.xml (which uses 0,1 for large-arc-flag,sweep-flag respectively), this path needs correction.
Could you please verify the intended path and update the flags accordingly? A common configuration for such arcs is large-arc-flag=0 and sweep-flag=1.
android:pathData="M0,160 A160,160 0 0,1 160,0 H800 A160,160 0 0,1 960,160 V800 A160,160 0 0,1 800,960 H160 A160,160 0 0,1 0,800 Z" />
|
|
||
| <path | ||
| android:fillColor="#9d75b1" | ||
| android:pathData="M0,160 A160,160 0 0,40 160,0 H800 A160,160 0 0,40 960,160 V800 A160,160 0 0,40 800,960 H160 A160,160 0 0,40 0,800 Z" /> |
There was a problem hiding this comment.
Similar to delivery_truck.xml, the pathData for this rounded rectangle background also appears to be malformed. The arc command A (e.g., in A160,160 0 0,40 160,0) uses an invalid sweep-flag value of 40. This flag must be 0 or 1.
This will likely cause rendering issues. If this is intended to be a standard rounded rectangle, the flags need to be corrected (e.g., to 0,1 for large-arc-flag,sweep-flag).
Could you please review and correct this path data?
android:pathData="M0,160 A160,160 0 0,1 160,0 H800 A160,160 0 0,1 960,160 V800 A160,160 0 0,1 800,960 H160 A160,160 0 0,1 0,800 Z" />
| android:viewportHeight="24"> | ||
|
|
||
| <path | ||
| android:fillColor="#9d75b1" |
There was a problem hiding this comment.
The fill color #9d75b1 is hardcoded here. For improved maintainability, consistency, and easier theming, it's generally a good practice to define such colors in res/values/colors.xml.
For example, you could define:
<!-- in res/values/colors.xml -->
<color name="tracker_icon_background_purple">#9d75b1</color>And then reference it in the drawable as android:fillColor="@color/tracker_icon_background_purple".
Would it be feasible to centralize this color definition?
| android:pathData="M0,4 A4,4 0 0,1 4,0 H20 A4,4 0 0,1 24,4 V20 A4,4 0 0,1 20,24 H4 A4,4 0 0,1 0,20 Z" /> | ||
|
|
||
| <path | ||
| android:fillColor="#000000" |
There was a problem hiding this comment.
The fill color #000000 (black) is hardcoded. While android:fillColor="@android:color/black" is an option for standard black, if this specific black is part of your app's design palette for icons, defining it in colors.xml (e.g., <color name="icon_foreground_primary">#000000</color>) and referencing it here (e.g., android:fillColor="@color/icon_foreground_primary") would offer better consistency and maintainability. What are your thoughts on this approach?
| android:viewportHeight="960"> | ||
|
|
||
| <path | ||
| android:fillColor="#9d75b1" |
There was a problem hiding this comment.
The fill color #9d75b1 is hardcoded. As mentioned for other drawables, consider defining this as a color resource in colors.xml (e.g., <color name="tracker_icon_background_purple">#9d75b1</color>) and referencing it via android:fillColor="@color/tracker_icon_background_purple". This promotes consistency and makes future color updates easier. Is this something that can be adopted here?
| android:pathData="M0,160 A160,160 0 0,40 160,0 H800 A160,160 0 0,40 960,160 V800 A160,160 0 0,40 800,960 H160 A160,160 0 0,40 0,800 Z" /> | ||
|
|
||
| <path | ||
| android:fillColor="#000000" |
There was a problem hiding this comment.
The fill color #000000 is hardcoded. For better maintainability, consider using a color resource from colors.xml. This could be @android:color/black if appropriate, or a custom defined color like <color name="icon_foreground_primary">#000000</color> referenced as android:fillColor="@color/icon_foreground_primary". Would this be a suitable change?
| android:viewportHeight="960"> | ||
|
|
||
| <path | ||
| android:fillColor="#9d75b1" |
There was a problem hiding this comment.
The fill color #9d75b1 is hardcoded. To enhance maintainability and ensure color consistency, it's advisable to define this in colors.xml (e.g., as <color name="tracker_icon_background_purple">#9d75b1</color>) and use android:fillColor="@color/tracker_icon_background_purple" here. Could this be refactored?
| android:pathData="M0,160 A160,160 0 0,40 160,0 H800 A160,160 0 0,40 960,160 V800 A160,160 0 0,40 800,960 H160 A160,160 0 0,40 0,800 Z" /> | ||
|
|
||
| <path | ||
| android:fillColor="#000000" |
There was a problem hiding this comment.
The fill color #000000 is hardcoded. Using a color resource (e.g., @android:color/black or a custom one like @color/icon_foreground_primary defined in colors.xml) would be more maintainable. Is it possible to make this change?
2 participants
Change-Id: I41d4376a1da53b1ed100c6ff9dccded0ee483877