google / accompanist

A collection of extension libraries for Jetpack Compose
https://google.github.io/accompanist
Apache License 2.0
7.43k stars 598 forks source link

[Adaptive] `gapWidth` ignored on foldables #1496

Closed ashughes closed 1 year ago

ashughes commented 1 year ago

Description Using TwoPane with a gapWidth and a foldAwareConfiguration does not include the gapWidth when laid out across a folding feature. However, the gapWidth is included in the layout when a folding feature is not present.

Steps to reproduce

TwoPane(
    first = {
        Spacer(
            Modifier
                .background(Color.Red)
                .fillMaxSize()
        )
    },
    second = {
        Spacer(
            Modifier
                .background(Color.Blue)
                .fillMaxSize()
        )
    },
    strategy = HorizontalTwoPaneStrategy(
        splitFraction = 0.3f,
        gapWidth = 16.dp,
    ),
    displayFeatures = displayFeatures,
    foldAwareConfiguration = FoldAwareConfiguration.VerticalFoldsOnly,
)

Expected behavior I would expect the 16.dp gap width to be present regardless of whether the TwoPane layout is laid out across a folding feature.

Actual behavior The 16.dp gap width is only present when the TwoPane layout is not laid out across a folding feature.

Screenshot on non-foldable: includes gap ![Screenshot_20230126_150322_Squid Dev](https://user-images.githubusercontent.com/295214/214970748-18da56e5-5d2a-4f38-ae5e-3b1889732bf1.jpg)
Screenshot on fold-in emulator: ignores gap ![Screenshot_20230126_150329](https://user-images.githubusercontent.com/295214/214971155-086f6231-ca1e-4dda-8a02-f31516b2b127.png)
Screenshot on Surface Duo 2 emulator (physical hinge): ignores gap ![Screenshot_1674774218](https://user-images.githubusercontent.com/295214/214970998-3216c710-1370-4c2d-9a6e-2ab191c4bce7.png)
alexvanyo commented 1 year ago

Hi, thank you for the report and the screenshots!

The gapWidth is indeed only used currently when there is no impacting physical fold present. When there is such a physical fold present, the width of the physical fold is used instead to determine the width. In some sense the strategy is the "virtual fold" when no impacting physical fold is present.

Would you expect that 16.dp to be evenly split on the two sides of the fold (8.dp and 8.dp), in addition to the fold's intrinsic width? I'm curious if adding a end padding of 8.dp to your first content and a start padding of 8.dp to your second content would work for your use case as padding that should be applied regardless of a real folding feature being present, and then using a gapWidth of 0.dp in HorizontalTwoPaneStrategy.

ashughes commented 1 year ago

@alexvanyo Thanks for the feedback! What you described using content padding is what I'm currently doing. However, using the gapWidth would simplify things as different components wouldn't need to know how they're laid out and whether or not to include extra end or start padding (this is because we switch between a one and two pane layout based on the width).

On the two devices I've tested (a 7.6 Fold-in with outer display API 33 emulator and Surface Duo 2 emulator), there was no width to the physical fold (as seen in the screenshots above). I do see how incorporating the physical fold width on a device with a single folding screen could be helpful in order to place the two panes around the fold appropriately, but depending on the fold width reported, you still might want some extra padding as well. I haven't used a device that reports the fold width, so I have no experience with what that looks like.

It makes sense to me that a device with a physical hinge separating two screens like the Surface Duo 2 would report a fold width of zero since the fold is essentially outside the screen. However, the result is that there is no padding at all at the end/start of each pane. On a separate note, for devices like this, I would actually prefer to specify a different gapWidth since having a physical hinge between two screens looks visually different than a single screen, but as far as I can tell it's not possible to determine.

I could definitely see splitting the gapWidth evenly between the two sizes of the fold in addition to the fold's intrinsic width. However, this might produce a gap that is too wide if the fold width is generous. Alternatively, the actual gap width could be max(foldWidth, gapWidth). This would provide the same gap between the panes on non-foldables as with foldables that have a fold width < gapWidth.

alexvanyo commented 1 year ago

(this is because we switch between a one and two pane layout based on the width)

Great to hear 😄 . Not sure the exact setup, but would you be able to use a Box or pass in a Modifier to the layouts with the extra padding?

It makes sense to me that a device with a physical hinge separating two screens like the Surface Duo 2 would report a fold width of zero since the fold is essentially outside the screen.

I think the bounds of the fold on the Surface Duo 2 should be non-zero, but visually that gap will be covered up underneath the physical hinge:

https://user-images.githubusercontent.com/4217560/215171246-7aec2936-e0fb-48c4-bd3c-3064a79fca63.mov

On a separate note, for devices like this, I would actually prefer to specify a different gapWidth since having a physical hinge between two screens looks visually different than a single screen, but as far as I can tell it's not possible to determine.

You should be able to determine that based on the [occlusion type](https://developer.android.com/reference/androidx/window/layout/FoldingFeature#occlusionType()) of the folding feature. If it is OcclusionType.FULL, then that would indicate that there is a fold that will cover up UI (like on the Surface Duo 2)

I could definitely see splitting the gapWidth evenly between the two sizes of the fold in addition to the fold's intrinsic width. However, this might produce a gap that is too wide if the fold width is generous. Alternatively, the actual gap width could be max(foldWidth, gapWidth). This would provide the same gap between the panes on non-foldables as with foldables that have a fold width < gapWidth.

Yeah, a max could make sense, although from an API perspective I'm concerned that changing behavior like that would be harder to invert if not desired, then add-in separately.

alexvanyo commented 1 year ago

To expand on my last comment, I think you could wrap the existing TwoPane to add a gapPadding for the horizontal case using something like this:

@Composable
fun HorizontalTwoPane(
    first: @Composable () -> Unit,
    second: @Composable () -> Unit,
    horizontalSplitFraction: Float,
    gapPadding: Dp,
    displayFeatures: List<DisplayFeature>,
    modifier: Modifier = Modifier,
) {
    TwoPane(
        first = {
            Box(
                modifier = Modifier.padding(end = gapPadding / 2)
            ) {
                first()
            }
        },
        second = {
            Box(
                modifier = Modifier.padding(start = gapPadding / 2)
            ) {
                second()
            }
        },
        strategy = HorizontalTwoPaneStrategy(
            splitFraction = horizontalSplitFraction,
        ),
        displayFeatures = displayFeatures,
        foldAwareConfiguration = FoldAwareConfiguration.VerticalFoldsOnly,
        modifier = modifier,
    )
}

I'm not sure if I'd want to add that to the API surface though, as it's a bit less flexible with the strategy and whether gapPadding should be divided by 2 or not.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

alexvanyo commented 1 year ago

Closing as working as intended, the above method allows achieving the same functionality with the current API.