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.

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

Jump to bottom

Fix a bug with Modifier.flex() nested in a row and column #2362

Merged
merged 2 commits into from
Oct 4, 2024
Merged

Fix a bug with Modifier.flex() nested in a row and column #2362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff77ec1
Fix a bug with Modifier.flex() nested in a row and column
squarejesse Oct 3, 2024
703fe99
New snapshots for the test.
squarejesse Oct 3, 2024
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
6 changes: 6 additions & 0 deletions ...rc/androidUnitTest/kotlin/app/cash/redwood/layout/composeui/ComposeUiFlexContainerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import app.cash.paparazzi.Paparazzi
import app.cash.redwood.layout.AbstractFlexContainerTest
import app.cash.redwood.layout.TestFlexContainer
import app.cash.redwood.layout.widget.FlexContainer
import app.cash.redwood.layout.widget.Spacer
import app.cash.redwood.snapshot.testing.ComposeSnapshotter
import app.cash.redwood.snapshot.testing.ComposeUiTestWidgetFactory
import app.cash.redwood.ui.Px
Expand Down Expand Up @@ -62,6 +63,11 @@ class ComposeUiFlexContainerTest(

override fun column() = flexContainer(FlexDirection.Column)

override fun spacer(backgroundColor: Int): Spacer<@Composable () -> Unit> {
// TODO: honor backgroundColor.
return ComposeUiRedwoodLayoutWidgetFactory().Spacer()
}

override fun snapshotter(widget: @Composable () -> Unit) = ComposeSnapshotter(paparazzi, widget)

class ComposeTestFlexContainer private constructor(
Expand Down
3 changes: 3 additions & 0 deletions ...poseUiFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[LTR].png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions ...poseUiFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[RTL].png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
42 changes: 42 additions & 0 deletions ...ut-shared-test/src/commonMain/kotlin/app/cash/redwood/layout/AbstractFlexContainerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import app.cash.redwood.layout.api.MainAxisAlignment
import app.cash.redwood.layout.api.Overflow
import app.cash.redwood.layout.widget.Column
import app.cash.redwood.layout.widget.Row
import app.cash.redwood.layout.widget.Spacer
import app.cash.redwood.snapshot.testing.Blue
import app.cash.redwood.snapshot.testing.Green
import app.cash.redwood.snapshot.testing.Red
Expand Down Expand Up @@ -53,6 +54,10 @@ abstract class AbstractFlexContainerTest<T : Any> {
/** Returns a non-lazy flex container column, even if the test is for a LazyList. */
abstract fun column(): Column<T>

abstract fun spacer(
backgroundColor: Int = argb(17, 0, 0, 0),
): Spacer<T>

abstract fun snapshotter(widget: T): Snapshotter

@Test fun testEmptyLayout_Column() {
Expand Down Expand Up @@ -829,6 +834,43 @@ abstract class AbstractFlexContainerTest<T : Any> {

snapshotter(fullWidthParent.value).snapshot()
}

/**
* We were incorrectly collapsing the dimensions of the widget.
* https://github.com/cashapp/redwood/issues/2018
*/
@Test fun testWidgetWithFlexModifierNestedInRowAndColumn() {
val root = flexContainer(FlexDirection.Column).apply {
width(Constraint.Fill)
height(Constraint.Fill)
margin(Margin(top = 24.dp))
modifier = Modifier
}

val rootChild0 = row().apply {
width(Constraint.Fill)
horizontalAlignment(MainAxisAlignment.Center)
root.children.insert(0, this)
}

val rootChild0Child0 = column().apply {
width(Constraint.Fill)
height(Constraint.Fill)
horizontalAlignment(CrossAxisAlignment.Stretch)
modifier = Modifier
.then(MarginImpl(Margin(start = 24.dp, end = 24.dp)))
.then(FlexImpl(1.0))
rootChild0.children.insert(0, this)
}

val rootChild0Child0Child0 = spacer().apply {
width(48.dp)
height(48.dp)
rootChild0Child0.children.insert(0, this)
}

snapshotter(root.value).snapshot()
}
}

interface TestFlexContainer<T : Any> :
Expand Down
3 changes: 3 additions & 0 deletions ...IViewFlexContainerTestHost/testWidgetWithFlexModifierNestedInRowAndColumn.1.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
8 changes: 8 additions & 0 deletions ...ut-uiview/src/commonTest/kotlin/app/cash/redwood/layout/uiview/UIViewFlexContainerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import app.cash.redwood.layout.TestFlexContainer
import app.cash.redwood.layout.api.Constraint
import app.cash.redwood.layout.api.CrossAxisAlignment
import app.cash.redwood.layout.widget.FlexContainer
import app.cash.redwood.layout.widget.Spacer
import app.cash.redwood.snapshot.testing.UIViewSnapshotCallback
import app.cash.redwood.snapshot.testing.UIViewSnapshotter
import app.cash.redwood.snapshot.testing.UIViewTestWidgetFactory
Expand Down Expand Up @@ -68,6 +69,13 @@ class UIViewFlexContainerTest(

override fun column() = flexContainer(FlexDirection.Column)

override fun spacer(backgroundColor: Int): Spacer<UIView> {
return UIViewRedwoodLayoutWidgetFactory().Spacer()
.apply {
value.backgroundColor = backgroundColor.toUIColor()
}
}

class UIViewTestFlexContainer internal constructor(
private val delegate: UIViewFlexContainer,
) : TestFlexContainer<UIView>,
Expand Down
2 changes: 2 additions & 0 deletions redwood-layout-view/src/main/kotlin/app/cash/redwood/layout/view/YogaLayout.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,13 @@ internal class YogaLayout(context: Context) : ViewGroup(context) {
calculateLayout(
requestedWidth = when {
widthMode == MeasureSpec.EXACTLY -> widthSize.toFloat()
widthMode == MeasureSpec.UNSPECIFIED -> Size.UNDEFINED
widthConstraint == Constraint.Fill -> widthSize.toFloat()
else -> Size.UNDEFINED
},
requestedHeight = when {
heightMode == MeasureSpec.EXACTLY -> heightSize.toFloat()
heightMode == MeasureSpec.UNSPECIFIED -> Size.UNDEFINED
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without the fix this nested spacer is asked to size itself using (mode=AT_MOST, size=0) and it returns a height of 0.

heightConstraint == Constraint.Fill -> heightSize.toFloat()
else -> Size.UNDEFINED
},
Expand Down
8 changes: 8 additions & 0 deletions redwood-layout-view/src/test/kotlin/app/cash/redwood/layout/view/ViewFlexContainerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import app.cash.paparazzi.Paparazzi
import app.cash.redwood.layout.AbstractFlexContainerTest
import app.cash.redwood.layout.TestFlexContainer
import app.cash.redwood.layout.widget.FlexContainer
import app.cash.redwood.layout.widget.Spacer
import app.cash.redwood.snapshot.testing.ViewSnapshotter
import app.cash.redwood.snapshot.testing.ViewTestWidgetFactory
import app.cash.redwood.ui.Px
Expand Down Expand Up @@ -63,6 +64,13 @@ class ViewFlexContainerTest(

override fun column() = flexContainer(FlexDirection.Column)

override fun spacer(backgroundColor: Int): Spacer<View> {
return ViewSpacer(paparazzi.context)
.apply {
setBackgroundColor(backgroundColor)
}
}

override fun snapshotter(widget: View) = ViewSnapshotter(paparazzi, widget)

class ViewTestFlexContainer internal constructor(
Expand Down
3 changes: 3 additions & 0 deletions ...w_ViewFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[LTR].png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions ...w_ViewFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[RTL].png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
6 changes: 6 additions & 0 deletions ...eui/src/androidUnitTest/kotlin/app/cash/redwood/layout/composeui/ComposeUiLazyListTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import app.cash.redwood.layout.api.MainAxisAlignment
import app.cash.redwood.layout.api.Overflow
import app.cash.redwood.layout.widget.Column
import app.cash.redwood.layout.widget.Row
import app.cash.redwood.layout.widget.Spacer
import app.cash.redwood.lazylayout.composeui.ComposeUiLazyList
import app.cash.redwood.lazylayout.widget.LazyList
import app.cash.redwood.snapshot.testing.ComposeSnapshotter
Expand Down Expand Up @@ -70,6 +71,11 @@ class ComposeUiLazyListTest(
return ComposeUiRedwoodLayoutWidgetFactory().Column()
}

override fun spacer(backgroundColor: Int): Spacer<@Composable () -> Unit> {
// TODO: honor backgroundColor.
return ComposeUiRedwoodLayoutWidgetFactory().Spacer()
}

override fun snapshotter(widget: @Composable () -> Unit) = ComposeSnapshotter(paparazzi, widget)

class ComposeTestFlexContainer private constructor(
Expand Down
3 changes: 3 additions & 0 deletions ...i_ComposeUiLazyListTest_testWidgetWithFlexModifierNestedInRowAndColumn[LTR].png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions ...i_ComposeUiLazyListTest_testWidgetWithFlexModifierNestedInRowAndColumn[RTL].png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions ...istAsFlexContainerTestHost/testWidgetWithFlexModifierNestedInRowAndColumn.1.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
8 changes: 8 additions & 0 deletions ...commonTest/kotlin/app/cash/redwood/lazylayout/uiview/UIViewLazyListAsFlexContainerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import app.cash.redwood.layout.TestFlexContainer
import app.cash.redwood.layout.api.MainAxisAlignment
import app.cash.redwood.layout.api.Overflow
import app.cash.redwood.layout.uiview.UIViewRedwoodLayoutWidgetFactory
import app.cash.redwood.layout.widget.Spacer
import app.cash.redwood.lazylayout.toUIColor
import app.cash.redwood.lazylayout.widget.LazyList
import app.cash.redwood.snapshot.testing.UIViewSnapshotCallback
Expand Down Expand Up @@ -47,6 +48,13 @@ class UIViewLazyListAsFlexContainerTest(

override fun column() = UIViewRedwoodLayoutWidgetFactory().Column()

override fun spacer(backgroundColor: Int): Spacer<UIView> {
return UIViewRedwoodLayoutWidgetFactory().Spacer()
.apply {
value.backgroundColor = backgroundColor.toUIColor()
}
}

override fun snapshotter(widget: UIView) = UIViewSnapshotter.framed(callback, widget)

class ViewTestFlexContainer private constructor(
Expand Down
8 changes: 8 additions & 0 deletions ...-view/src/test/kotlin/app/cash/redwood/lazylayout/view/ViewLazyListAsFlexContainerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import app.cash.redwood.layout.api.Overflow
import app.cash.redwood.layout.view.ViewRedwoodLayoutWidgetFactory
import app.cash.redwood.layout.widget.Column
import app.cash.redwood.layout.widget.Row
import app.cash.redwood.layout.widget.Spacer
import app.cash.redwood.lazylayout.widget.LazyList
import app.cash.redwood.snapshot.testing.ViewSnapshotter
import app.cash.redwood.snapshot.testing.ViewTestWidgetFactory
Expand Down Expand Up @@ -69,6 +70,13 @@ class ViewLazyListAsFlexContainerTest(
return ViewRedwoodLayoutWidgetFactory(paparazzi.context).Column()
}

override fun spacer(backgroundColor: Int): Spacer<View> {
return ViewRedwoodLayoutWidgetFactory(paparazzi.context).Spacer()
.apply {
value.setBackgroundColor(backgroundColor)
}
}

override fun snapshotter(widget: View) = ViewSnapshotter(paparazzi, widget)

class ViewTestFlexContainer private constructor(
Expand Down
3 changes: 3 additions & 0 deletions ...ListAsFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[LTR].png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions ...ListAsFlexContainerTest_testWidgetWithFlexModifierNestedInRowAndColumn[RTL].png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.