Skip to content

[MaterialButton] Added methods to apply horizontal insets #1790

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[MaterialButton] Added methods to apply horizontal insets #1790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions lib/java/com/google/android/material/button/MaterialButton.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1036,6 +1036,47 @@ public int getInsetTop() {
return materialButtonHelper.getInsetTop();
}

/**
* Sets the button left inset
*
* @attr ref com.google.android.material.R.styleable#MaterialButton_android_insetLeft
* @see #getInsetLeft()
*/
public void setInsetLeft(@Dimension int insetLeft) {
materialButtonHelper.setInsetLeft(insetLeft);
}

/**
* Gets the left inset for this button
*
* @attr ref com.google.android.material.R.styleable#MaterialButton_android_insetLeft
* @see #setInsetLeft(int)
*/
@Dimension
public int getInsetLeft() {
return materialButtonHelper.getInsetLeft();
}
/**
* Sets the button right inset
*
* @attr ref com.google.android.material.R.styleable#MaterialButton_android_insetRight
* @see #getInsetRight()
*/
public void setInsetRight(@Dimension int insetRight) {
materialButtonHelper.setInsetRight(insetRight);
}

/**
* Gets the right inset for this button
*
* @attr ref com.google.android.material.R.styleable#MaterialButton_android_insetRight
* @see #setInsetRight(int)
*/
@Dimension
public int getInsetRight() {
return materialButtonHelper.getInsetRight();
}

@Override
protected int[] onCreateDrawableState(int extraSpace) {
final int[] drawableState = super.onCreateDrawableState(extraSpace + 2);
Expand Down
39 changes: 39 additions & 0 deletions lib/java/com/google/android/material/button/MaterialButtonHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,4 +432,43 @@ public int getInsetTop() {
return insetTop;
}


public void setInsetLeft(@Dimension int newInsetLeft) {
setHorizontalInsets(newInsetLeft, insetRight);
}

public int getInsetLeft() {
return insetLeft;
}

public void setInsetRight(@Dimension int newInsetRight) {
setHorizontalInsets(insetLeft, newInsetRight);
}

private void setHorizontalInsets(@Dimension int newInsetLeft, @Dimension int newInsetRight) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about merging this method with setVerticalInsets into something like setInsets(int, int, int, int) or updateInsets(int, int, int, int)? They're almost exactly the same, it might be nice to consolidate logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I will merge the 2 methods.

// Store padding before setting background, since background overwrites padding values
int paddingStart = ViewCompat.getPaddingStart(materialButton);
int paddingTop = materialButton.getPaddingTop();
int paddingEnd = ViewCompat.getPaddingEnd(materialButton);
int paddingBottom = materialButton.getPaddingBottom();
int oldInsetLeft = insetLeft;
int oldInsetRight = insetRight;
insetLeft = newInsetLeft;
insetRight = newInsetRight;
if (!backgroundOverwritten) {
updateBackground();
}
// Set the stored padding values
ViewCompat.setPaddingRelative(
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're using setPaddingRelative here, don't we want to expose setInsetStart and setInsetEnd since internally setPaddingRelative will swap the left/right values depending on the layout direction?

If we want to support setting left and right values, we'd need to change this to ViewCompat.setPadding(), if I understand correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hunterstich In the current code in the loadFromAttributes you are using setPaddingRelative adding paddingStart + insetLeft. Let me know what you prefer. Maybe the best solution is to use insetLeft, paddingLeft and ViewCompat.setPadding() everywhere.

material-components-android/lib/java/com/google/android/material/button/MaterialButtonHelper.java

Lines 128 to 133 in 5e3ccdc

ViewCompat.setPaddingRelative(
materialButton,
paddingStart + insetLeft,
paddingTop + insetTop,
paddingEnd + insetRight,
paddingBottom + insetBottom);

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh good catch

I agree, it would be nice to correct/avoid the mix of paddingStart|End and left|right. Using paddingLeft|Right + insetLeft|Right and ViewCompat.setPadding everywhere sounds like a good solution to me. Off the top of my head, I can't think of any behavior that this might break, but once you update MaterialButtonHelper and I pull it in and run all our test, we can double check.

In the future (or now if you want...), we can add in support for insetStart|End and ViewCompat.setPaddingRelative.

materialButton,
paddingStart + newInsetLeft - oldInsetLeft,
paddingTop ,
paddingEnd + newInsetRight - oldInsetRight,
paddingBottom);
}

public int getInsetRight() {
return insetRight;
}

}