Status Update
No update yet.
Comments
jo...@google.com
se...@google.com
Branch: androidx-main
commit a330c0d3bcdd41326f37968a60e6084ad4a2e32c
Author: Chet Haase <chet@google.com>
Date: Wed Jul 05 07:26:46 2023
Convert APIs using PointF to use Float instead
PointF is a convenient mechanism for passing around x.y values
representing 2D points. But there are downsides, including:
- Converting to PointF: You may not have the data in PointF form
to begin with, so using an API which takes PointF requires converting
the data to that form (including allocating a PointF object every time)
- Mutability: Point structures can be mutated internally, causing
unpredictability in what that mutation means. Should the library
react to those changes? Ignore them? Do defensive copies (requiring
even more allocations)? Using primitive types like Float make the
behavior more obvious (by making the data inherently immutable).
- Allocations: Whenever we use object types, there are necessarily
allocations on the Java heap for them. This puts pressure on the GC
at both allocation and collection time. Given the amount of points
being passed around (especially at morph creation time, when curves
are being split and created), this causes a lot of PointF objects to
be allocated (even temporarily). Using Float avoids that problem.
Also fixed bug with unclosed paths causing discontinuity at the
start/end point.
Bug: 276466399
Bug: 290254314
Test: integration and unit tests pass
Relnote: PointF parameters changed to Float pairs
Change-Id: Id4705d27c7be31b26ade8186b99fffe2e2f8450e
M graphics/graphics-shapes/api/current.txt
M graphics/graphics-shapes/api/restricted_current.txt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/CubicShapeTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/CubicTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/PolygonMeasureTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/PolygonTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/RoundedPolygonTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/ShapesTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/TestUtils.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Cubic.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/CubicShape.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/FeatureMapping.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/FloatMapping.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Morph.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/PolygonMeasure.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/RoundedPolygon.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Shapes.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Utils.kt
M graphics/integration-tests/testapp-compose/src/main/java/androidx/graphics/shapes/testcompose/DebugDraw.kt
M graphics/integration-tests/testapp-compose/src/main/java/androidx/graphics/shapes/testcompose/ShapeEditor.kt
M graphics/integration-tests/testapp/src/main/java/androidx/graphics/shapes/test/MaterialShapes.kt
ga...@gmail.com
se...@google.com
se...@google.com
The following release(s) address this bug.It is possible this bug has only been partially addressed:
androidx.graphics:graphics-shapes:1.0.0-alpha04
ha...@gmail.com
We decided to create a custom bottom sheet, because of this issue with maxWidth and some other issues.
But assuming I'm understanding what you're proposing to be this:
fun ModalBottomSheetLayout(
maxWidth: Dp = 640.dp,
...
)
then I'm not sure what value I would override it with. Do I just put a large value that will always be larger than the screen width, say 2000.dp (I'm not sure how that would affect sheetShape, or sheetContent if the content relies on the width), or do I manually have to find the width of my composable and use that (which would be equivalant to using fillMaxWidth())? It seems a bit cumbersome to have to do that everywhere I want to use a bottom sheet.
For our use case a way of telling the bottom sheet to use fillMaxWidth() would be ideal. Maybe maxWidth = Dp.Infinity (or Dp.Unspecified?) could be translated to fillMaxWidth()? I'm not sure if this would be considered misuse of those values or not
se...@google.com
I understand your concern here. From our perspective, we want to have a strong opinion on larger screen behavior and ensure the sheet by default has some maximum width on larger screens, not endlessly extending based on its internal content. We can provide some flexibility here two ways, either the way you are describing, which was my idea as well, allowing users to change the maximum value if they have a different opinion on what the maximum value should be. Or another option is having a potential boolean like providesMaxWidth which the user could then disable. However, while the latter is better for your use case, I don't think users who want to change the maxWidth having to do: ModalBottomSheetLayout(modifier.widthIn(max=800.dp), providesMaxWidth=false) is a great experience. A third option would be not providing maximum width at all, but we would prefer the sheet not endlessly span larger screens by default as that is against the spec we are trying to provide.
ha...@gmail.com
I agree that ModalBottomSheetLayout(modifier.widthIn(max=800.dp), providesMaxWidth=false) isn't a great experience, and could probably lead to bugs if you forget to specify both.
Another option could be to model the width with a sealed interface. This at least keeps it confined to one parameter, and I think it provides a simple enough API
sealed interface ModalBottomSheetWidth {
object Default : ModalBottomSheetWidth
object Fill : ModalBottomSheetWidth
data class Custom(val width: Dp) : ModalBottomSheetWidth // Not sure if this has a use-case or not
}
@Composable
fun ModalBottomSheetLayout(
sheetWidth: ModalBottomSheetWidth = ModalBottomSheetWidth.Default
...
) {
val widthModifier = when (sheetWidth) {
ModalBottomSheetWidth.Default -> Modifier.widthIn(max = 640.dp)
ModalBottomSheetWidth.Fill -> Modifier.fillMaxWidth()
is ModalBottomSheetWidth.Custom -> Modifier.width(sheetWidth.width)
}
}
se...@google.com
se...@google.com
ke...@google.com
ap...@google.com
Branch: androidx-main
commit 392df4fcb263770b411399a8342cb3afeeaa1d6d
Author: Kevin Truong <kevinctruong@google.com>
Date: Thu Sep 14 18:55:33 2023
[BottomSheet] Can override the maximum width of ModalBottomSheet and BottomSheetScaffold
Adding a new parameter, sheetMaxWidth, that can be used to specify the maximum width that the sheet would take up.
Dp.Unspecified can be passed in for sheetMaxWidth if a sheet that spans the entire width of the screen is desired.
Test: Adding a tests for ModalBottomSheet and BottomSheetScaffold that tests when Dp.Unspecified is passed in for sheetMaxWidth to ensure that the sheet spans the entire width, as well as different sheetMaxWidth values.
Bug: 266697696
Relnote: Adding a new sheetMaxWidth parameter that developers can set to specify a maximum width that the sheet will span. Dp.Unspecified can be passed in for the parameter if a sheet that spans the entire screen width is desired.
Change-Id: Ifb7c9ee4d0066e86787e8fcbf0d156b9f92e5cfb
M compose/material3/material3/api/current.txt
M compose/material3/material3/api/restricted_current.txt
M compose/material3/material3/src/androidInstrumentedTest/kotlin/androidx/compose/material3/BottomSheetScaffoldTest.kt
M compose/material3/material3/src/androidInstrumentedTest/kotlin/androidx/compose/material3/ModalBottomSheetTest.kt
M compose/material3/material3/src/androidMain/kotlin/androidx/compose/material3/ModalBottomSheet.android.kt
M compose/material3/material3/src/commonMain/kotlin/androidx/compose/material3/BottomSheetScaffold.kt
M compose/material3/material3/src/commonMain/kotlin/androidx/compose/material3/SheetDefaults.kt
jo...@google.com
Kevin, your commit is for M3, right? This issue is for M2
ke...@google.com
Oops, yes sorry this was implemented in M3 but not in M2. Will repoen this ticket and mark the other ticket for M3 as fixed. Thank you for catching this Jossi!
Description
Compose 1.4.0-alpha04 introduced a max width constraint on
ModalBottomSheetLayout. However, theFor larger screens, bottom sheets have a default max-width to prevent undesired layouts and awkward spacing. However, this can be overridden if needed.
I have designs that expect, and will take advantage of, the entire screen width in a bottom sheet, and would like to have this behaviour without creating my own implementation.
As noted in the
I'm not sure what the intended behaviour here is as there seems to be a difference between Material 2 and 3. Since
ModalBottomSheetLayoutis incompose.materialand notcompose.material3I assume it isn't based on Material 3. If that is the case, is there a plan to add a corresponding component incompose.material3with a modal bottom sheet that can override the max width constraint?