Skip to content

Conversation

@Will5
Copy link

@Will5 Will5 commented Jul 14, 2016

Took KonradJanica's code (#43) and reformatted for easier review by grantland.

*/
private static void autofit(TextView view, TextPaint paint, float minTextSize, float maxTextSize,
int maxLines, float precision) {
int maxLines, float precision) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: revert

@grantland
Copy link
Owner

This is much cleaner!

The new API method names seem a bit odd to me. Instead of #isHeightFitting(), #setHeightFitting(boolean), etc. do you think #isAutofitWidthEnabled(), setAutofitHeightEnabled(boolean) sounds more natural? Additionally, this would allow us to eventually deprecate #isEnabled(), #getEnabled(boolean) and add #isAutofitWidthEnabled() and #setAutofitWidthEnabled(boolean).

@Will5
Copy link
Author

Will5 commented Jul 21, 2016

The proposed naming convention of #isAutofitHeightEnabled() and #setAutofitHeightEnabled(boolean), along with #isAutofitWidthEnabled() and #setAutofitWidthEnabled(boolean) sound much cleaner to me!

@Will5
Copy link
Author

Will5 commented Jul 23, 2016

I pushed a commit with the API refactoring. Let me know if it sounds clearer.


private boolean mEnabled;
private boolean mIsAutofitWidthEnabled;
private boolean mIsAutofitting;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why AutofitHelper::mIsAutofitHeightEnabled is a static variable?

If I use two different AutofitTextView in my app, one with AutofitHeight enabled, and a second with AutofitHeight disabled, then the first will get AutofitHeight disabled too. Is this the expected behaviour?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the field is static so that it can be accessed via the private static void autofit(TextView view, TextPaint paint, float minTextSize, float maxTextSize, int maxLines, float precision) method. I'm not sure that the method needs to be static though. @grantland?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could leave the method autofit(...) static, but then pass mIsAutofitHeightEnabled as a parameter to it. This way, each AutofitTextView can have its own mIsAutofitHeightEnabled setting instead of sharing a global one.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I pushed the appropriate change

@AllanHasegawa
Copy link

Awesome, looks good ^^

Thank you for your commits.

Just a small nitpick. The variable named mIsAutofitHeightEnabled is using "Hungarian Notation" to denote it is a "member variable". See Google's code style: link.

When you use it inside a static function (like the autofit(...)), I think the proper naming convention would be to remove the "m" before the variable name, as it is no longer a "member variable".

@Will5
Copy link
Author

Will5 commented Aug 9, 2016

Thank you @AranHase!

@ejohansson
Copy link

Any updates on this. Have been checking in on this pull request for a while now?

@johnnylambada
Copy link

I made a quick fork of this with @Will5 's changes because I need to use it in my project. It's available from jitpack.io here: https://github.com/flipagram/android-autofittextview -- When this is all merged in I'll remove my fork.

@slvrfn
Copy link

slvrfn commented Jun 16, 2017

why has this still not been merged?

@AndreasBoehm
Copy link

Please merge this PR, we do need this fix for our project as well. Thanks!

konifar added a commit to konifar/android-autofittextview that referenced this pull request Apr 30, 2018
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

Successfully merging this pull request may close these issues.

7 participants