Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed issue 93: https://github.com/JoanZapata/android-iconify/issues/93 #121

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

chRyNaN
Copy link

@chRyNaN chRyNaN commented Sep 14, 2015

When attempting to set an ActionBar MenuItem with an IconDrawable, such as:

MenuItem n = menu.findItem(R.id.action_notifications).setIcon(new IconDrawable(this,
                Iconify.IconValue.fa_globe).colorRes(R.color.white).actionBarSize());

causes a NullPointerException on the setIcon() call. This is due to the internal setIcon() code calling getConstantState().newDrawable(). Since this is not supplied in IconDrawable it causes a NullPointerException. For more details see the issue here: #93. The issue has wrongfully been closed but should now be fixed with the supplied update. Please test and verify that my update is correct and causes no other issues.

@JoanZapata
Copy link
Owner

Thanks for the merge request, and sorry for closing issue #93, thought it wasn't related to iconify.
About your code, I don't think that's a proper implementation and use of a ConstantState. It's not a big deal since it wasn't implemented at all before, but as long as this state is not properly implemented it doesn't need to be implemented at all, a static no-op ConstantState would have been enough IMO. I'll make some tests myself and see what I can do.

@1zaman
Copy link
Contributor

1zaman commented Oct 19, 2015

I had thought that it was allowed to return null in getConstantState() if no constant state is supported by the implementation, as here. However, looking at the documentation now, I don't see this mentioned anywhere. I also had to implement a constant state in the edX app's implementation to make it work with a combination of LayerDrawable and StateListDrawable, although I only provided a fake instance that was bound to the drawable instance to work around the issue: https://github.com/edx/edx-app-android/blob/bab109adb51995fa62e74b282957f4bdc5ba4763/VideoLocker/src/main/java/org/edx/mobile/third_party/iconify/IconDrawable.java#L276

It should be easy enough to implement an actual static state holder in order to provide a proper implementation of the API, although it would need to contain all the drawable state for it to actually be useful.

@1zaman
Copy link
Contributor

1zaman commented Oct 25, 2015

I was just looking through the source code of Drawable, and saw that the documentation of the hidden clearMutated() method states that these methods don't need to be supported by third-party drawable implementations:

This is hidden because only framework drawables can be cached, so custom drawables don't need to support constant state, mutate(), or clearMutated().

Therefore, I guess #93 should be classified and reported as a bug in the design support library.

@1zaman
Copy link
Contributor

1zaman commented Oct 27, 2015

I added a static shared state implementation in #134.

@chRyNaN
Copy link
Author

chRyNaN commented Oct 27, 2015

I haven't had time to test #134 yet, but if it works and no one has any objections I'll close this pull request upon #134 merge.

@1zaman
Copy link
Contributor

1zaman commented Oct 27, 2015

I tried to have this auto-closed by referencing it as resolved by my pull request, but I don't see the indicator for it here. Maybe it doesn't work between pull requests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants