-
Notifications
You must be signed in to change notification settings - Fork 525
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
Fix and enhance IconDrawable #134
base: develop
Are you sure you want to change the base?
Conversation
6e81cb3
to
83886b1
Compare
We might consider removing these methods, since at this point they are just top-level utility methods. On the other hand, they might still prove useful. |
83886b1
to
6b99750
Compare
Note that issue #93 has been fixed in version 23.1.0 of the design support library. However, there are similar bugs in the framework as well (e.g. in |
6b99750
to
87eb194
Compare
This commit adds lots of enhancements and fixes to IconDrawable: - Adds spin animation functionality. - Adds constant state in order to work around bugs in the framework and support library where this is expected. - Handle the case where the width of the icon is not the same as it's height by returning the correct intrinsic width, and changing the drawing logic to always fit the icon within it's bounds. - Adds support for color state lists. - Adds support for tint, and sets the default to implement the half translucency on disabled state, and removed the custom logic to do it by changing the alpha. - Modulates the existing alpha from the color instead of replacing it. - Removes the changing of the bounds upon size change, as that should only be done by the view. - Adds a public method to get the icon. - Adds missing implementations of base methods. Note that because we need to be able to generate the drawable without a Context from the constant state, the methods that resolved resources (i.e. colors, dimensions) now have to take a Context as a parameter. This unfortunately makes the change backwards-incompatible. The spin animation logic is also slightly changed in CustomTypefaceSpan to initialize the start time upon first draw instead of upon instantiation, since that is the actual start time of the animation. This matches it with the implementation in IconDrawable.
87eb194
to
179918d
Compare
Thanks for this huge work! tv.setText("fa-arrow-left");
tv.setCompoundDrawables(
new IconDrawable(context, FontAwesomeIcons.fa_arrow_left).actionBarSize(context),
null, null, null); I'll need more time to understand what you did entirely. I'm not sure I want that level of implementation, especially since Android says subclasses of Drawable shouldn't implement it. |
The issue with the compound drawable is related to this change:
Drawables should only define their intrinsic dimensions. The bounds are meant to be set by the controller view. If you use As for the constant state, you're right that it is only meant to be used by platform drawables that are inflated from XML definitions. However, as discussed in #93, there are bugs in the platform and support libraries where this is expected to be implemented by all drawables, probably owing to the fact that it is implemented in all the platform implementations. As there is no harm in implementing the state, we might as well do it to maximize compatibility and reduce issues. |
Yeah, you're right, sorry. I should really get some time to read more about
IMHO, there is. More code means harder to maintain and to track bugs. I was thinking, what if, instead of extending In the meanwhile I'll reopen the PR and take some more time to understand it entirely. |
The constant state implementation doesn't really add much code. It's basically just a place to store the global state of the drawable (the local state and derived data can be retained at the base object level). Even this is only necessary if we want to implement this strictly in accordance with it's documented behaviour. We could also simply implement a dummy state that holds a reference to the actual drawable, and clones it or even returns the same instance in it's Extending Thanks for reopening the pull request. |
Looks like this is conflicting with commit 132fd08 over different exception messages for handling unregistered module, and you're throwing |
Yep, already merged it locally, thanks. :) |
Hi. Thanks for the great module. Is there a plan to merge the spin functionality into the core module. This is something I really need for a project. Thanks once again... |
Spin functionality is already available using IconTextView. This pull On Tue, Dec 15, 2015 at 7:48 PM, Ayo Adesugba [email protected]
|
Yeah. I need spin for the IconDrawable, since i modify the button left drawable depending on app state from the fragment. Thanks... |
You can copy the IconDrawable implementation from here into your project On Wed, Dec 16, 2015 at 8:25 AM, Ayo Adesugba [email protected]
|
Sorry for the delay, this is the #1 thing in the roadmap of the project, I just can't find time right now. |
I just noticed that an |
Does |
The version in the mainline project doesn’t; the version in my pull request does. You can copy that implementation over in your project if you like. I also have all of my pull requests merged in my fork if you’re interested in them, though I haven’t merged in the changes from the latest releases. It seems that the author doesn’t have the time to go through these and get them merged in the mainline.
|
November 2015.. Damn, time goes so fast, I really need to take care of this. I'm sorry for the delay. |
Note that as I commented in another pull request, all of my pull requests affect each other's scope somewhat, so if you want to merge all of them then it would be better to just merge the develop branch from my fork where I have them merged properly already. If you want to merge them one at a time instead, then let me rebase the other branches after each merge. EDIT: I have the rtl-support branch merged in my develop branch too though, so if you don't want to merge that then you can merge the last commit before it, i.e. 1zaman/android-iconify@47e49d0. |
@1zaman Sorry about the late reply however your solution stops spinning if the fragment/activity goes into paused state to workaround the issue so far seems to be a little bit tedious to the end-user. @JoanZapata Need a helping hand? |
It should keep on spinning as long as the view is being drawn by the layout rendering framework. Can you share the exact steps that reproduce your issue? |
@1zaman Herewith setup, if you call
|
@JoanZapata Any update/plan to merge this PR? |
This pull request adds lots of enhancements and fixes to
IconDrawable
:Note that because we need to be able to generate the drawable without a
Context
from the constant state, the methods that resolved resources (i.e. colors, dimensions) now have to take aContext
as a parameter. This unfortunately makes the change backwards-incompatible.The spin animation logic is also slightly changed in
CustomTypefaceSpan
to initialize the start time upon first draw instead of upon instantiation, since that is the actual start time of the animation. This matches it with the implementation inIconDrawable
.