fix(core): don't overwrite existing icons when no ico_ attribute is set - #669
Merged
Conversation
IconicsAttrsApplier.getIconicsDrawable() always returned a drawable, even for an AttributeSet carrying no `ico_*` attribute at all, because the extractor falls back to creating an empty IconicsDrawable. Every caller treats a non-null result as "there is iconics data here" and assigns it, so a menu item's `android:icon` or an ImageView's `android:src` got replaced by an empty drawable. Return null when the styled attributes are empty, which is what all three call sites (IconicsMenuInflaterUtil, and the ActionMenuItemView / ImageView branches of IconicsFactory) already handle. Adds regression tests over `menu_playground`, which covers an item with a plain `android:icon`, an item defined via `ico_*` attributes and an item with no icon at all. Fixes #666 Supersedes #667 Co-authored-by: PrOF-kk <valerio.colella.2001@gmail.com>
Superseded by the null return in IconicsAttrsApplier: matching attribute names by their `ico_` prefix breaks if an attribute is ever renamed, and it only covered the menu inflater while IconicsFactory had the same problem.
mikepenz
force-pushed
the
fix/menu-inflater-keeps-existing-icons
branch
from
August 20, 2026 16:34
b1630e3 to
ae354b7
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| implementation project(':weather-icons-typeface-library') | ||
|
|
||
| testImplementation 'junit:junit:4.13.2' | ||
| testImplementation 'org.robolectric:robolectric:4.16' |
There was a problem hiding this comment.
| A newer version of org.robolectric:robolectric than 4.16 is available: 4.16.1 |
| <!-- neither an `android:icon` nor any `ico_*` attribute: must stay without an icon --> | ||
| <item | ||
| android:id="@+id/menu_item_3" | ||
| android:title="Item 3" |
There was a problem hiding this comment.
Hardcoded string "Item 3", should use @string resource |
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
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.
Follow-up on top of #667, which fixed the reported symptom (#666). Same bug, addressed one layer down so it covers every call site.
Root cause
IconicsAttrsApplier.getIconicsDrawable()is declared nullable and all three call sites treatnullas "no iconics data here, leave the target alone":But it never returns
null.IconicsAttrsExtractor.extract()ends increateIfNeeds(), so anAttributeSetwith noico_*attribute still yields an emptyIconicsDrawable— and that empty drawable then replaces whatever icon the target already had.That is why a menu item declaring only
android:iconcame back blank. The same applies to theActionMenuItemViewandImageViewbranches ofIconicsFactory, so anandroid:srcis clobbered too wheneverIconicsLayoutInflateris in use — which #667's menu-inflater-local fix does not reach.Changes
Return
nullwhen the obtainedTypedArrayholds none of the styleable's attributes:R.styleable.Iconicscontains onlyico_*attributes, soindexCount == 0means exactly "this tag declares no iconics data". No behaviour change for anything that does declare one.With that in place the
name.startsWith("ico_")check from #667 is redundant, and is removed in the second commit — the author flagged it as brittle under attribute renames, and it covered the menu inflater only.Tests
app/src/test/.../IconicsMenuInflaterUtilTest.kt, Robolectric, over the existingmenu_playgroundresource, which already had the two interesting cases and now also has an item with no icon at all:menu_item_1— plainandroid:icon, noico_*→ drawable must be left untouched (asserted by identity)menu_item_2—ico_icon/ico_color/ico_size→ must become anIconicsDrawablemenu_item_3— no icon at all → must stay without oneBoth regression tests fail with the fix reverted and pass with it. This is the first unit test in the repo, so it also brings in Robolectric and
includeAndroidResourcesfor the app module.