Open chRyNaN opened 8 years ago
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.
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.
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.
I added a static shared state implementation in #134.
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.
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?
When attempting to set an ActionBar MenuItem with an IconDrawable, such as:
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: https://github.com/JoanZapata/android-iconify/issues/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.