-
-
Notifications
You must be signed in to change notification settings - Fork 618
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
make it possible to use IconicsDrawable to define xml drawable #481
Comments
Thank you so much for this issue, I was not aware that this is possible, but looks absolutely like an amazing thing to introduce. For the Sow e had this possibility via the provider: https://github.com/mikepenz/Android-Iconics/blob/develop/library-core/src/main/java/com/mikepenz/iconics/IconicsContentProvider.kt#L27 I wonder if there is a different way to get the context inside this drawable? how will it resolve colors for example if set as a theme references like Having a variant of the |
My understanding is that you can resolve them using the argument to the inflate method (that Android magically call with the correct argument)
I don't understand what making the method extension functions would change? |
I made a quick-and-dirty proto to see what works or not (using the approach of making the context field nullable, as it is the simplest for a quick and dirty proto) (https://github.com/dzamlo/Android-Iconics branch drawable-from-xml-proto) Regarding the theme attributes, you can't (always) resolve them in the inflate method. The theme argument is null. But later, the https://cs.android.com/android/platform/superproject/+/master:frameworks/base/graphics/java/android/graphics/drawable/ColorDrawable.java seems to be a good example of how to handle that correctly |
Thank you very much. I should have a chance later today to look at it and experiment with it too. Could you perhaps open a PR with it. So when we get it in that you'll get the contributors badge here. Really appreciate your idea and effort |
@dzamlo thanks for the initial changes. Which were already great to see how it is supposed to be used. So I actually figured it makes sense to bring this in v5 with a major refactor, removing context requirement for drawables altogether. and requiring resources and theme which is the same the inflate method offers. You can see the current (initial) commit of this refactor here: #490 (based on top your initial changes) I will continue this refactor in the coming days. thank you so much again for the initial changes! It is amazing to finally be able to use iconics directly as drawable (even though if it is just API 24+) |
This is now supported in the first a01 of v5 👍 |
Starting with API 24, you can define drawable in xmls that are implemented in code instead of being limited to the few existing drawable.
If
IconicsDrawable
supported that, you could create a drawable with this content (inres/drawable
):And then, you could use it in a lot of place that need a drawable, including in your menu definition. This would replace most, if not all, use of
IconicsMenuInflaterUtil
for API >= 24.There is three things that the
Drawable
needs to do for this to work:public void inflate(@NonNull Resources r, @NonNull XmlPullParser parser, @NonNull AttributeSet attrs, @Nullable Theme theme)
. This method will be called on the drawable instance. It's in this method that you get all the values from the XML.attrs.xml
For
IconicsDrawable
, the issue is the need for theContext
. We need a way to make it not need the context.If I read the code correctly, it used for two things: initializing Iconics and for some of the setter.
I see two way of making the
Context
not mandatory:Making the
context
field nullableThe
Iconics.init(context)
call will throw an exception, ifIconics.init
was not called before: I thing this is what we wantFor all the setter that use the context: make them crash (use
context!!
). Provides an alternative that take the context as an argument. The problem is that we loose some safetyMove all the code from
IconicsDrawable
to a new class (IconicsDrawableNoContext
for example). Remove the context field. CallIconics.init(null)
instead ofIconics.init(context)
. Modify all the setters that use the context so that they take the context as a parameter.Make IconicsDrawable inherit from this new class. Implement all the setter that need the context by calling the version with the context in parameter. You could then use
IconicsDrawableNoContext
in the XMLs.I prefer the second solution.
What do you think of all that?
The text was updated successfully, but these errors were encountered: