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

Bug: Don't chain constructors with this(). #30

Closed
reubenscratton opened this issue Apr 30, 2016 · 8 comments
Closed

Bug: Don't chain constructors with this(). #30

reubenscratton opened this issue Apr 30, 2016 · 8 comments

Comments

@reubenscratton
Copy link

The problems caused by this() are most evident when changing the base class from TextView to it's close relative EditText. The EditText behaves rather oddly because it's never gets set up with some important default internal styles (see Android source for EditText(Context, AttributeSet)).

Your current code has the two simpler constructors calling the 3-argument constructor via 'this(context, attrs, 0)'. The derived constructors should be calling super(), not this() and also calling a common init() method which contains the autofit init code.

@AndroidDeveloperLB
Copy link
Owner

AndroidDeveloperLB commented Apr 30, 2016

Actually, calling "this" for the CTOR is very common, and it's also how Google made its own custom views. Here's the example of AppCompatTextView:

    public AppCompatTextView(Context context) {
        this(context, null);
    }

    public AppCompatTextView(Context context, AttributeSet attrs) {
        this(context, attrs, android.R.attr.textViewStyle);
    }

    public AppCompatTextView(Context context, AttributeSet attrs, int defStyleAttr) {
        super(TintContextWrapper.wrap(context), attrs, defStyleAttr);

        mDrawableManager = AppCompatDrawableManager.get();
        mBackgroundTintHelper = new AppCompatBackgroundHelper(this, mDrawableManager);
        mBackgroundTintHelper.loadFromAttributes(attrs, defStyleAttr);

        mTextHelper = AppCompatTextHelper.create(this);
        mTextHelper.loadFromAttributes(attrs, defStyleAttr);
        mTextHelper.applyCompoundDrawablesTints();
    }

This also (optionally) allows "final" fields, which depend on the CTOR.

@reubenscratton
Copy link
Author

Sure, but bypassing default styling can still be very problematic (as it was for me in this case). If you'll permit an Appeal to Authority here's the relevant part from the Android SDK: http://developer.android.com/guide/topics/ui/custom-components.html#modifying

2. Class Initialization

As always, the super is called first. Furthermore, this is not a default constructor, but a parameterized one. The EditText is created with these parameters when it is inflated from an XML layout file, thus, our constructor needs to both take them and pass them to the superclass constructor as well.

While I'm here, very many thanks for creating this component... it saved me a ton of work!

@AndroidDeveloperLB
Copy link
Owner

Both ways are ok and work fine. Using "this" has its own benefits.

@AndroidDeveloperLB
Copy link
Owner

Here're the CTORs of TextView, btw:

  public TextView(Context context) {
        this(context, null);
    }

    public TextView(Context context, @Nullable AttributeSet attrs) {
        this(context, attrs, com.android.internal.R.attr.textViewStyle);
    }

    public TextView(Context context, @Nullable AttributeSet attrs, int defStyleAttr) {
        this(context, attrs, defStyleAttr, 0);
    }

    @SuppressWarnings("deprecation")
    public TextView(
            Context context, @Nullable AttributeSet attrs, int defStyleAttr, int defStyleRes) {
        super(context, attrs, defStyleAttr, defStyleRes);
...

@reubenscratton
Copy link
Author

Why do you think the SDK even mentions it?

If you subclass TextView, as your lib does, how do you propose to apply com.android.internal.R.attr.textViewStyle as per TextView's constructors?

Obviously neither approach is intrinsically wrong - there could be perfectly valid reasons for avoiding particular versions of super() - but this isn't one of them. As you'll find out if you ever extend your lib to provide an autofitting EditText.

@AndroidDeveloperLB
Copy link
Owner

AndroidDeveloperLB commented Apr 30, 2016

I've updated the code. Now "this" should work fine.

@reubenscratton
Copy link
Author

Looks good to me, I didn't realise attr.textViewStyle was accessible... the one in TextView/EditText source code refers to an internal package but it seems probable they're identical.

@AndroidDeveloperLB
Copy link
Owner

well if it's accessible for compat, it should be accessible for me too. :)

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

No branches or pull requests

2 participants