Replace Modifier + with 'then' [161529964]

Fixed

Feature Request

Status Update

No update yet.

Description

ma...@google.com

created issue #1

Jul 17, 2020 02:16PM

No description yet.

Comments

ma...@google.com <ma...@google.com> #2Jul 17, 2020 02:17PM

With chaining being the most common way to combine several modifiers together, chainig also has few other parameters:

Chaining starts with . (dot)
Chaining looks structured and implies order

Both this points are missing in Modifier.background.padding + another case.

Therefore, the proposal is to substitute plus with then to encourage people to type Modifier.background().padding().then(another) for better readability and consistency. This was also possible with padding.plus(another), but it appeared to be not enough.

infix stays in then version as well to make it easier to write fun Modifier.myModifier() = this then MyModifierImpl()

mo...@google.com <mo...@google.com> #3Jul 20, 2020 08:16PM

I think we did not agree that this was better in API council. Have we gotten external feedback on this? I'd like to see data on this before making this change.

ma...@google.com <ma...@google.com> #4Jul 21, 2020 11:02AM

I decided to took an action on this as I want to make it good before alpha release to provide consistent API with proper guidance. I chatted with Adam before hard and we agreed on this, sorry I didn't involve you. I believe that without the action we will postpone this for too long and will just settle with smth that we could have improved.

Some input points for this decision:

I'd like to avoid + at all. I think this look terrible when you chain modifiers, e.g

Box(Modifier
    .background(Color.Red)
    .padding(10.dp) 
    + MyModifierFromAbove
    .background(Color.Blue)

It reads poorly and "breaks" the chain visually. Also this usage discourage people to inline if-else as you need to wrap it with () or smth. This is the personal developers choice maybe, but I feel that it's good for us to be opinionated about it and guide (not force) developers to the happy, readable path.

Adam wants to preserve infix notion to make creation of Modifiers easy, e.g

fun Modifier.myModifier() = this concatOperator MyModifier()

where concatOperator is anything, like + or plus or then.

Make it clear that order matters, e.g. then is more strong of a signal than plus

We were able to find good tradeoff between these 3 requirements, as follows:

make plus to be then, to avoid + on the call side
leave infix in then, to make modifiers creators happy.

With this, it's still possible to write

Box(Modifier
    .background(Color.Red)
    .padding(10.dp) 
    then MyModifierFromAbove
    .background(Color.Blue)

So we're leaving this option to people who prefer this, but encouraging to concat with ., like so:

Box(Modifier
    .background(Color.Red)
    .padding(10.dp) 
    .then(MyModifierFromAbove)
    .background(Color.Blue)

This also makes it easy to write if-else modifiers

Box(Modifier
    .background(Color.Red)
    .then(if(cond) Modifier.border(Color.Red, 4.dp) else Modifier.padding(4.dp))

Which is a very nice bonus imo.

Now, to the feedback we're received:

Recently people in kotlin slack asked for guidance on what's the best way to chain modifiers. The answer from Adam and myself was to "chain with . or .plus". I expect with this change it will be easier to figure out this guidance naturally.
We keep continuously receiving feedback that it's unclear that order matters (no links unfortunatelly), so this will be a stronger signal, which I like.

With all that in mind I made this change, as I believe this is step forward in almost every aspect aside from "having to change the codebase".

Happy to hear thoughts and suggestions!

mo...@google.com <mo...@google.com> #5Jul 21, 2020 05:28PM

While I still prefer the '+' to 'then', I understand the reasoning behind the change. I only have "personal preference" arguments, so nothing substantial.

ap...@google.com <ap...@google.com> #6Jul 23, 2020 02:23PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit 1eec3ee4f92235cea9f2181fd5227acf40381690
Author: Matvei Malkov <malkov@google.com>
Date: Fri Jul 17 15:09:02 2020

Replace Modifier::plus with Modifier::then.

With chaining being the most common way to combine several modifiers together, chainig also has few other parameters:
1. Chaining starts with . (dot)
2. Chaining looks structured and implies order

Both this points are missing in `Modifier.background.padding + another` case.

Therefore, the proposal is to substitute plus with then to encourage people to type `Modifier.background().padding().then(another)` for better readability and consistency. This was also possible with `padding.plus(another)`, but it appeared to be not enough.

infix stays in then version as well to make it easier to write `fun Modifier.myModifier() = this then MyModifierImpl()`

Relnote: "Modifier.plus has been deprecated, use Modifier.then instead. 'Then' has a stronger signal of ordering, while also prohibits to type `Modifier.padding().background() + anotherModifier`, which breaks the chain and harder to read"

Test: n/a
Bug: 161529964
Change-Id: Iedd587edbed0ba964ef203a66b98be7297147bd7

M compose/compose-runtime/compose-runtime-benchmark/src/androidTest/java/androidx/compose/benchmark/deeptree/DeepTree.kt
M compose/compose-runtime/compose-runtime-benchmark/src/androidTest/java/androidx/compose/benchmark/siblings/SiblingManagement.kt
M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/core/OnPositionedBenchmark.kt
M ui/integration-tests/demos/src/main/java/androidx/ui/demos/DemoFilter.kt
M ui/ui-android-view/integration-tests/android-view-demos/src/main/java/androidx/ui/androidview/demos/PointerInputInteropComposeInAndroid.kt
M ui/ui-animation/src/androidAndroidTest/kotlin/androidx/compose/animation/AnimationModifierTest.kt
M ui/ui-animation/src/commonMain/kotlin/androidx/compose/animation/AnimationModifier.kt
M ui/ui-core/api/0.1.0-dev16.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev16.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev16.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/integration-tests/ui-core-demos/src/main/java/androidx/ui/core/demos/gestures/DragScaleGestureDetectorDemo.kt
M ui/ui-core/integration-tests/ui-core-demos/src/main/java/androidx/ui/core/demos/gestures/HorizontalScrollersInVerticalScrollerDemo.kt
M ui/ui-core/integration-tests/ui-core-demos/src/main/java/androidx/ui/core/demos/gestures/NestedPressDemo.kt
M ui/ui-core/integration-tests/ui-core-demos/src/main/java/androidx/ui/core/demos/gestures/NestedScrollingDemo.kt
M ui/ui-core/integration-tests/ui-core-demos/src/main/java/androidx/ui/core/demos/gestures/VerticalScrollerInDrawerLayoutDemo.kt
M ui/ui-core/samples/src/main/java/androidx/ui/core/samples/ModifierSamples.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/core/AndroidPointerInputTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/core/PainterModifierTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/core/focus/FindFocusableChildrenTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/core/focus/FindParentFocusNodeTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/core/keyinput/FindParentKeyInputNodeTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/core/pointerinput/PointerInputEventProcessorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/core/test/AndroidLayoutDrawTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/core/test/ClipDrawTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/core/test/ClipPointerInputTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/core/test/DrawLayerTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/core/test/DrawReorderingTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/core/test/DrawShadowTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/core/test/LayerTouchTransformTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/core/test/OnPositionedTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/core/test/OpacityTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/core/test/RtlLayoutTest.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/node/PointerInteropFilter.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/node/ViewInterop.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/ComposedModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/DrawLayerModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/DrawModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/LayoutId.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Modifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/OnPositionedModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/PainterModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/ZIndexModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/focus/FocusModifier2.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/gesture/LongPressDragGestureFilter.kt
M ui/ui-core/src/test/kotlin/androidx/ui/core/ComposedModifierTest.kt
M ui/ui-core/src/test/kotlin/androidx/ui/core/LayoutNodeTest.kt
M ui/ui-core/src/test/kotlin/androidx/ui/core/ModifierTest.kt
M ui/ui-foundation/samples/src/main/java/androidx/compose/foundation/samples/ClickableTextSample.kt
M ui/ui-foundation/samples/src/main/java/androidx/compose/foundation/samples/InteractionStateSample.kt
M ui/ui-foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/DraggableTest.kt
M ui/ui-foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/IndicationTest.kt
M ui/ui-foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/ScrollableTest.kt
M ui/ui-foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/TextFieldFocusTest.kt
M ui/ui-foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/TextFieldTest.kt
M ui/ui-foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/ZoomableTest.kt
M ui/ui-foundation/src/commonMain/kotlin/androidx/compose/foundation/Background.kt
M ui/ui-foundation/src/commonMain/kotlin/androidx/compose/foundation/Box.kt
M ui/ui-foundation/src/commonMain/kotlin/androidx/compose/foundation/Clickable.kt
M ui/ui-foundation/src/commonMain/kotlin/androidx/compose/foundation/ClickableText.kt
M ui/ui-foundation/src/commonMain/kotlin/androidx/compose/foundation/Icon.kt
M ui/ui-foundation/src/commonMain/kotlin/androidx/compose/foundation/Scroll.kt
M ui/ui-foundation/src/commonMain/kotlin/androidx/compose/foundation/lazy/LazyItems.kt
M ui/ui-foundation/src/commonMain/kotlin/androidx/compose/foundation/selection/Toggleable.kt
M ui/ui-layout/src/androidAndroidTest/kotlin/androidx/compose/foundation/layout/ContainerTest.kt
M ui/ui-layout/src/androidAndroidTest/kotlin/androidx/compose/foundation/layout/LayoutAspectRatioTest.kt
M ui/ui-layout/src/androidAndroidTest/kotlin/androidx/compose/foundation/layout/LayoutSizeTest.kt
M ui/ui-layout/src/androidAndroidTest/kotlin/androidx/compose/foundation/layout/OnPositionedTest.kt
M ui/ui-layout/src/androidAndroidTest/kotlin/androidx/compose/foundation/layout/RowColumnTest.kt
M ui/ui-layout/src/androidAndroidTest/kotlin/androidx/compose/foundation/layout/StackTest.kt
M ui/ui-layout/src/androidMain/kotlin/androidx/compose/foundation/layout/ConstraintLayout.kt
M ui/ui-layout/src/commonMain/kotlin/androidx/compose/foundation/layout/AlignmentLine.kt
M ui/ui-layout/src/commonMain/kotlin/androidx/compose/foundation/layout/Column.kt
M ui/ui-layout/src/commonMain/kotlin/androidx/compose/foundation/layout/Intrinsic.kt
M ui/ui-layout/src/commonMain/kotlin/androidx/compose/foundation/layout/LayoutAspectRatio.kt
M ui/ui-layout/src/commonMain/kotlin/androidx/compose/foundation/layout/LayoutDirections.kt
M ui/ui-layout/src/commonMain/kotlin/androidx/compose/foundation/layout/LayoutOffset.kt
M ui/ui-layout/src/commonMain/kotlin/androidx/compose/foundation/layout/LayoutPadding.kt
M ui/ui-layout/src/commonMain/kotlin/androidx/compose/foundation/layout/LayoutSize.kt
M ui/ui-layout/src/commonMain/kotlin/androidx/compose/foundation/layout/Row.kt
M ui/ui-layout/src/commonMain/kotlin/androidx/compose/foundation/layout/Stack.kt
M ui/ui-material/integration-tests/material-studies/src/main/java/androidx/ui/material/studies/rally/Icons.kt
M ui/ui-material/src/commonMain/kotlin/androidx/ui/material/Divider.kt
M ui/ui-material/src/commonMain/kotlin/androidx/ui/material/Drawer.kt
M ui/ui-material/src/commonMain/kotlin/androidx/ui/material/IconButton.kt
M ui/ui-material/src/commonMain/kotlin/androidx/ui/material/Slider.kt
M ui/ui-material/src/commonMain/kotlin/androidx/ui/material/Surface.kt
M ui/ui-material/src/commonMain/kotlin/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/IsDisplayedTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/MultipleActivitiesClickTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/gesturescope/SendDoubleClickTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/gesturescope/SendLongClickTest.kt
M ui/ui-text/src/commonMain/kotlin/androidx/compose/foundation/text/CoreTextField.kt

https://android-review.googlesource.com/1365867

ma...@google.com <ma...@google.com> Jul 23, 2020 02:30PM

Marked as fixed.

Issue 161529964