Fix and enhance IconDrawable

[1zaman]

This pull request 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 (resolves #93, resolves #121).
• 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 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.

[1zaman]

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.

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.

[1zaman]

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 LayoutDrawable), so we might want to retain the constant state to work around these issues.

[JoanZapata]

Thanks for this huge work! Unfortunately it seems it breaks IconDrawable when used as a compound drawable.

tv.setText("fa-arrow-left");
tv.setCompoundDrawables(
    new IconDrawable(context, FontAwesomeIcons.fa_arrow_left).actionBarSize(context), 
    null, null, null);

before

after

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.

[1zaman]

The issue with the compound drawable is related to this change:

• Removes the changing of the bounds upon size change, as that should only be done by the view.

Drawables should only define their intrinsic dimensions. The bounds are meant to be set by the controller view. If you use setCompoundDrawablesWithIntrinsicBounds(), then it should work as intended. Note that this behaviour is identical to all the platform drawable implementations, and the previous behaviour was technically the faulty one.

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.

[JoanZapata]

Yeah, you're right, sorry. I should really get some time to read more about Drawable. :-)

As there is no harm in implementing the state

IMHO, there is. More code means harder to maintain and to track bugs.

I was thinking, what if, instead of extending Drawable, we extend BitmapDrawable? All we have to do then is to create the icon Bitmap and provide it to the superclass, which would implement all the methods related to state, bounds, etc... The memory footprint would be higher though, so I'm not sure that's a really good idea. Might be worth the try though. If you have any thoughts I'd be glad to hear about it.

In the meanwhile I'll reopen the PR and take some more time to understand it entirely.

[1zaman]

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 newDrawable() implementation. That's how it was done in #121, but you wanted a proper implementation so I did that here :wink:. It doesn't really matter how you do it because it is never actually going to be used for it's intended purpose, which is to keep a shared state for drawables inflated from the same XML resource definition.

Extending BitmapDrawable does not seem to be a good idea. As you mention, it will introduce a large and unnecessary memory footprint. Also, there is no way to increase the size of it's Bitmap, nor does it have any public API for changing it's Bitmap instance, so we would be forced to assign it a static and fixed size upon initialization. I think the current implementation of a custom drawable is perfectly suitable for the situation.

Thanks for reopening the pull request.

[1zaman]

Looks like this is conflicting with commit 132fd08a28c89dbcbd560089f9c44bbefed12034 over different exception messages for handling unregistered module, and you're throwing IllegalStateException while I'm using IllegalArgumentException. You can resolve this however you like if/when you merge this.

[JoanZapata]

Yep, already merged it locally, thanks. :)

[adesugbaa]

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...

[1zaman]

Spin functionality is already available using IconTextView. This pull request adds it in IconDrawable as well.

On Tue, Dec 15, 2015 at 7:48 PM, Ayo Adesugba [email protected] wrote:

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...

— Reply to this email directly or view it on GitHub https://github.com/JoanZapata/android-iconify/pull/134#issuecomment-164785501 .

[adesugbaa]

Yeah. I need spin for the IconDrawable, since i modify the button left drawable depending on app state from the fragment.

Thanks...

[1zaman]

You can copy the IconDrawable implementation from here into your project and use that until this pull request is merged into a new library update.

On Wed, Dec 16, 2015 at 8:25 AM, Ayo Adesugba [email protected] wrote:

Yeah. I need spin for the IconDrawable, since i modify the button left drawable depending on app state from the fragment.

Thanks...

— Reply to this email directly or view it on GitHub https://github.com/JoanZapata/android-iconify/pull/134#issuecomment-164977790 .

[JoanZapata]

Sorry for the delay, this is the #1 thing in the roadmap of the project, I just can't find time right now.

[squeeish]

I just noticed that an IconDrawable for {fa-bicycle} is cut off due to the incorrect width. Would be awesome if this is fixed!

[ghost]

Does IconDrawable have spin functionality, as far as I can tell, it doesn't

[1zaman]

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.

On Jun 22, 2016, at 1:47 PM, Kevin [email protected] wrote:

Does IconDrawable have spin functionality, as far as I can tell, it doesn't

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/JoanZapata/android-iconify/pull/134#issuecomment-227681170, or mute the thread https://github.com/notifications/unsubscribe/AEDCjc5zbwxiHkLD_Kh-F-BPoZmgXPVHks5qOPaYgaJpZM4GWQfn.

[JoanZapata]

November 2015.. Damn, time goes so fast, I really need to take care of this. I'm sorry for the delay.

[1zaman]

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@47e49d00f32dc11dff06a8d18bd4b26245289b2a.

[ghost]

@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?

[1zaman]

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?

[ghost]

@1zaman Herewith setup, if you call LoadingButton#setLoading(true), then the spinning icon drawable should appear and be in a spinning state

import android.content.Context;
import android.graphics.Color;
import android.util.AttributeSet;
import android.util.TypedValue;
import android.widget.Button;

import com.joanzapata.iconify.IconDrawable;
import com.joanzapata.iconify.fonts.FontAwesomeIcons;

/**
 * @author Kevin Leigh Crain
 **/
public class LoadingButton extends Button {

    private boolean loading;

    public LoadingButton(Context context) {
        super(context);
        init();
    }

    public LoadingButton(Context context, AttributeSet attrs) {
        super(context, attrs);
        init();
    }

    public LoadingButton(Context context, AttributeSet attrs, int defStyle) {
        super(context, attrs, defStyle);
        init();
    }

    private void init() {
        int size = (int) TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, 12,
                getResources().getDisplayMetrics());
        setPadding(size, 0, size, 0);
    }

    public void setLoading(boolean loading) {
        this.loading = loading;
        IconDrawable drawable = loading ? new IconDrawable(getContext(),
                FontAwesomeIcons.fa_spinner.key()) : null;
        if (drawable != null) {
            drawable.color(Color.WHITE);
            drawable.actionBarSize();
            drawable.spin();
        }
        setCompoundDrawables(null, null, drawable, null);
        setEnabled(!loading);
    }

    public boolean isLoading() {
        return loading;
    }
}

[farhan]

@JoanZapata Any update/plan to merge this PR?