[Compose] Fix margin for Chain first and last elements

oscar-ad · oscar-ad · commit 6fdd704d0946 · 2022-05-19T14:07:24.000-07:00
Fixes #572 The margin for constraints that can be declared for chains would not be applied for VerticalChains. Also applies the gone margin, which was ignored for both kinds of Chains.
diff --git a/constraintlayout/core/src/main/java/androidx/constraintlayout/core/state/ConstraintReference.java b/constraintlayout/core/src/main/java/androidx/constraintlayout/core/state/ConstraintReference.java
@@ -71,15 +71,16 @@ public interface ConstraintReferenceFactory {
     protected int mMarginRight = 0;
     protected int mMarginStart = 0;
     protected int mMarginEnd = 0;
-    int mMarginTop = 0;
-    int mMarginBottom = 0;
-
-    int mMarginLeftGone = 0;
-    int mMarginRightGone = 0;
-    int mMarginStartGone = 0;
-    int mMarginEndGone = 0;
-    int mMarginTopGone = 0;
-    int mMarginBottomGone = 0;
+    protected int mMarginTop = 0;
+    protected int mMarginBottom = 0;
+
+    protected int mMarginLeftGone = 0;
+    protected int mMarginRightGone = 0;
+    protected int mMarginStartGone = 0;
+    protected int mMarginEndGone = 0;
+    protected int mMarginTopGone = 0;
+    protected int mMarginBottomGone = 0;
+
     int mMarginBaseline = 0;
     int mMarginBaselineGone = 0;
 
diff --git a/constraintlayout/core/src/main/java/androidx/constraintlayout/core/state/helpers/ChainReference.java b/constraintlayout/core/src/main/java/androidx/constraintlayout/core/state/helpers/ChainReference.java
@@ -38,5 +38,4 @@ public ChainReference style(State.Chain style) {
     public ChainReference bias(float bias) { mBias = bias;
         return this;
     }
-
 }
diff --git a/constraintlayout/core/src/main/java/androidx/constraintlayout/core/state/helpers/HorizontalChainReference.java b/constraintlayout/core/src/main/java/androidx/constraintlayout/core/state/helpers/HorizontalChainReference.java
@@ -16,9 +16,9 @@
 
 package androidx.constraintlayout.core.state.helpers;
 
-import androidx.constraintlayout.core.widgets.ConstraintWidget;
 import androidx.constraintlayout.core.state.ConstraintReference;
 import androidx.constraintlayout.core.state.State;
+import androidx.constraintlayout.core.widgets.ConstraintWidget;
 
 public class HorizontalChainReference extends ChainReference {
 
@@ -39,16 +39,19 @@ public void apply() {
             if (first == null) {
                 first = reference;
                 if (mStartToStart != null) {
-                    first.startToStart(mStartToStart).margin(mMarginStart);
+                    first.startToStart(mStartToStart)
+                            .margin(mMarginStart)
+                            .marginGone(mMarginStartGone);
                 } else if (mStartToEnd != null) {
-                    first.startToEnd(mStartToEnd).margin(mMarginStart);
+                    first.startToEnd(mStartToEnd).margin(mMarginStart).marginGone(mMarginStartGone);
                 } else if (mLeftToLeft != null) {
                     // TODO: Hack until we support RTL properly
-                    first.startToStart(mLeftToLeft).margin(mMarginLeft);
+                    first.startToStart(mLeftToLeft).margin(mMarginLeft).marginGone(mMarginLeftGone);
                 } else if (mLeftToRight != null) {
                     // TODO: Hack until we support RTL properly
-                    first.startToEnd(mLeftToRight).margin(mMarginLeft);
+                    first.startToEnd(mLeftToRight).margin(mMarginLeft).marginGone(mMarginLeftGone);
                 } else {
+                    // No constraint declared, default to Parent.
                     first.startToStart(State.PARENT);
                 }
             }
@@ -61,16 +64,17 @@ public void apply() {
 
         if (previous != null) {
             if (mEndToStart != null) {
-                previous.endToStart(mEndToStart).margin(mMarginEnd);
+                previous.endToStart(mEndToStart).margin(mMarginEnd).marginGone(mMarginEndGone);
             } else if (mEndToEnd != null) {
-                previous.endToEnd(mEndToEnd).margin(mMarginEnd);
+                previous.endToEnd(mEndToEnd).margin(mMarginEnd).marginGone(mMarginEndGone);
             } else if (mRightToLeft != null) {
                 // TODO: Hack until we support RTL properly
-                previous.endToStart(mRightToLeft).margin(mMarginRight);
-            }else if (mRightToRight != null) {
+                previous.endToStart(mRightToLeft).margin(mMarginRight).marginGone(mMarginRightGone);
+            } else if (mRightToRight != null) {
                 // TODO: Hack until we support RTL properly
-                previous.endToEnd(mRightToRight).margin(mMarginRight);
+                previous.endToEnd(mRightToRight).margin(mMarginRight).marginGone(mMarginRightGone);
             } else {
+                // No constraint declared, default to Parent.
                 previous.endToEnd(State.PARENT);
             }
         }
@@ -86,10 +90,12 @@ public void apply() {
         switch (mStyle) {
             case SPREAD: {
                 first.setHorizontalChainStyle(ConstraintWidget.CHAIN_SPREAD);
-            } break;
+            }
+            break;
             case SPREAD_INSIDE: {
                 first.setHorizontalChainStyle(ConstraintWidget.CHAIN_SPREAD_INSIDE);
-            } break;
+            }
+            break;
             case PACKED: {
                 first.setHorizontalChainStyle(ConstraintWidget.CHAIN_PACKED);
             }
diff --git a/constraintlayout/core/src/main/java/androidx/constraintlayout/core/state/helpers/VerticalChainReference.java b/constraintlayout/core/src/main/java/androidx/constraintlayout/core/state/helpers/VerticalChainReference.java
@@ -39,10 +39,11 @@ public void apply() {
             if (first == null) {
                 first = reference;
                 if (mTopToTop != null) {
-                    first.topToTop(mTopToTop);
+                    first.topToTop(mTopToTop).margin(mMarginTop).marginGone(mMarginTopGone);
                 } else if (mTopToBottom != null) {
-                    first.topToBottom(mTopToBottom);
+                    first.topToBottom(mTopToBottom).margin(mMarginTop).marginGone(mMarginTopGone);
                 } else {
+                    // No constraint declared, default to Parent.
                     first.topToTop(State.PARENT);
                 }
             }
@@ -55,10 +56,15 @@ public void apply() {
 
         if (previous != null) {
             if (mBottomToTop != null) {
-                previous.bottomToTop(mBottomToTop);
+                previous.bottomToTop(mBottomToTop)
+                        .margin(mMarginBottom)
+                        .marginGone(mMarginBottomGone);
             } else if (mBottomToBottom != null) {
-                previous.bottomToBottom(mBottomToBottom);
+                previous.bottomToBottom(mBottomToBottom)
+                        .margin(mMarginBottom)
+                        .marginGone(mMarginBottomGone);
             } else {
+                // No constraint declared, default to Parent.
                 previous.bottomToBottom(State.PARENT);
             }
         }
@@ -83,5 +89,4 @@ public void apply() {
             }
         }
     }
-
 }
diff --git a/projects/ComposeConstraintLayout/app/src/main/java/com/example/constraintlayout/verification/dsl/Chains.kt b/projects/ComposeConstraintLayout/app/src/main/java/com/example/constraintlayout/verification/dsl/Chains.kt
@@ -35,6 +35,7 @@ import androidx.constraintlayout.compose.ChainStyle
 import androidx.constraintlayout.compose.ConstraintLayout
 import androidx.constraintlayout.compose.ConstraintSet
 import androidx.constraintlayout.compose.Dimension
+import androidx.constraintlayout.compose.Visibility
 
 @Preview
 @Composable
@@ -114,7 +115,42 @@ fun Test12() { // Constraint an horizontal chain
             start.linkTo(parent.start, 12.dp)
         }
         constrain(chain1) {
-            start.linkTo(box3.end)
+            start.linkTo(box3.end, 10.dp)
+        }
+    }
+    ThreeBoxChainLayout(constraintSet = constraintSet)
+}
+
+@Preview
+@Composable
+fun Test18() { // Constraint an horizontal chain with gone margins
+    val constraintSet = ConstraintSet {
+        val box1 = createRefFor("box1")
+        val box2 = createRefFor("box2")
+        val box3 = createRefFor("box3")
+        val chain1 = createHorizontalChain(box1, box2, chainStyle = ChainStyle.Spread)
+
+        constrain(box1) {
+            width = Dimension.value(20.dp)
+            height = Dimension.value(20.dp)
+            centerVerticallyTo(parent)
+        }
+        constrain(box2) {
+            width = Dimension.value(20.dp)
+            height = Dimension.value(20.dp)
+            centerVerticallyTo(box1)
+        }
+        constrain(box3) {
+            width = Dimension.value(200.dp)
+            height = Dimension.value(20.dp)
+            top.linkTo(parent.top, 12.dp)
+            start.linkTo(parent.start, 12.dp)
+            visibility = Visibility.Gone
+        }
+        constrain(chain1) {
+            start.linkTo(box3.end, 10.dp, 100.dp)
+            // gone margin with parent should not matter
+            end.linkTo(parent.end, 10.dp, 200.dp)
         }
     }
     ThreeBoxChainLayout(constraintSet = constraintSet)
@@ -146,7 +182,43 @@ fun Test13() { // Constrain a vertical chain
             start.linkTo(parent.start, 12.dp)
         }
         constrain(chain1) {
-            top.linkTo(box3.bottom)
+            top.linkTo(box3.bottom, 10.dp)
+        }
+    }
+    ThreeBoxChainLayout(constraintSet = constraintSet)
+}
+
+@Preview
+@Composable
+fun Test17() { // Constrain a vertical chain with gone margins
+    val constraintSet = ConstraintSet {
+        val box1 = createRefFor("box1")
+        val box2 = createRefFor("box2")
+        val box3 = createRefFor("box3")
+        val chain1 = createVerticalChain(box1, box2, chainStyle = ChainStyle.Spread)
+
+        constrain(box1) {
+            width = Dimension.value(20.dp)
+            height = Dimension.value(20.dp)
+            centerHorizontallyTo(parent)
+        }
+        constrain(box2) {
+            width = Dimension.value(20.dp)
+            height = Dimension.value(20.dp)
+            centerHorizontallyTo(box1)
+        }
+        constrain(box3) {
+            width = Dimension.value(20.dp)
+            height = Dimension.value(200.dp)
+            top.linkTo(parent.top, 12.dp)
+            start.linkTo(parent.start, 12.dp)
+            visibility = Visibility.Gone
+        }
+        constrain(chain1) {
+            // With the gone margin it should basically layout the same as if box3 was visible
+            top.linkTo(box3.bottom, 10.dp, 100.dp)
+            // gone margin with parent should not matter
+            bottom.linkTo(parent.bottom, 10.dp, 200.dp)
         }
     }
     ThreeBoxChainLayout(constraintSet = constraintSet)
diff --git a/projects/ComposeConstraintLayout/app/src/main/res/raw/results.txt b/projects/ComposeConstraintLayout/app/src/main/res/raw/results.txt

Original file line number	Diff line number	Diff line change
`@@ -38,5 +38,4 @@ public ChainReference style(State.Chain style) {`
`38`	`38`	`public ChainReference bias(float bias) { mBias = bias;`
`39`	`39`	`return this;`
`40`	`40`	`}`
`41`		`-`
`42`	`41`	`}`
Original file line number	Diff line number	Diff line change
`@@ -39,10 +39,11 @@ public void apply() {`
`39`	`39`	`if (first == null) {`
`40`	`40`	`first = reference;`
`41`	`41`	`if (mTopToTop != null) {`
`42`		`- first.topToTop(mTopToTop);`
	`42`	`+ first.topToTop(mTopToTop).margin(mMarginTop).marginGone(mMarginTopGone);`
`43`	`43`	`} else if (mTopToBottom != null) {`
`44`		`- first.topToBottom(mTopToBottom);`
	`44`	`+ first.topToBottom(mTopToBottom).margin(mMarginTop).marginGone(mMarginTopGone);`
`45`	`45`	`} else {`
	`46`	`+ // No constraint declared, default to Parent.`
`46`	`47`	`first.topToTop(State.PARENT);`
`47`	`48`	`}`
`48`	`49`	`}`
`@@ -55,10 +56,15 @@ public void apply() {`
`55`	`56`
`56`	`57`	`if (previous != null) {`
`57`	`58`	`if (mBottomToTop != null) {`
`58`		`- previous.bottomToTop(mBottomToTop);`
	`59`	`+ previous.bottomToTop(mBottomToTop)`
	`60`	`+ .margin(mMarginBottom)`
	`61`	`+ .marginGone(mMarginBottomGone);`
`59`	`62`	`} else if (mBottomToBottom != null) {`
`60`		`- previous.bottomToBottom(mBottomToBottom);`
	`63`	`+ previous.bottomToBottom(mBottomToBottom)`
	`64`	`+ .margin(mMarginBottom)`
	`65`	`+ .marginGone(mMarginBottomGone);`
`61`	`66`	`} else {`
	`67`	`+ // No constraint declared, default to Parent.`
`62`	`68`	`previous.bottomToBottom(State.PARENT);`
`63`	`69`	`}`
`64`	`70`	`}`
`@@ -83,5 +89,4 @@ public void apply() {`
`83`	`89`	`}`
`84`	`90`	`}`
`85`	`91`	`}`
`86`		`-`
`87`	`92`	`}`