[Adaptive] Add accompanist/adaptive library with TwoPane implementation. #1281

alexvanyo · 2022-08-05T01:36:21Z

Sets up an adaptive library, with the intention of adding a collection of adaptive layout utilities to help optimize UI for smaller and larger screens.

The first utility added is TwoPane, which allows placing two slots of content with their placement controlled by a specific strategy. These strategies can be composed, and the built-in entry point supports positioning the slots around folding features automatically. TwoPane itself does not support showing one slot or the other: if that behavior is desired (because there isn't enough room to show both slots), then that decision should be made a level above the TwoPane.

Some of those more complicated layouts (list/detail, supporting panel) would then be using TwoPane as an implementation detail, and configuring the TwoPane strategy and what is passed to the slots appropriately.

This PR is using ~~a snapshot~~ the latest version of Compose for now to have access to Placeable.coordinates, since folding features are given in window coordinates, so TwoPane needs to know it's location relative to the window in order to know where to position its slots.

To provide information about the folding features, I've also set up a WindowGeometry object which contains information about the window. Right now, this includes the list of display features, as well as the window size class. The window size class isn't used anywhere right now, but I included it to have a single object containing the "top-level" window state that can be passed around. a calculation for the list of display features.

device-2022-08-04-182836.mp4

matvei-g

Thanks Alex. Did a high level API pass, left a few comment.

Great work!

matvei-g · 2022-08-05T17:48:52Z

adaptive/src/main/java/com/google/accompanist/adaptive/TwoPane.kt

+    fallbackStrategy: TwoPaneStrategy,
+    windowGeometry: WindowGeometry,
+): TwoPaneStrategy = TwoPaneStrategy { density, layoutDirection, layoutCoordinates ->
+    val verticalFold = windowGeometry.displayFeatures.find {


Here and everywhere else in the TwoPane we only use DisplayFeatures, and window size class from window geometry is used for a completely separate type of decision making.

Given the above, does that make sense for display features and WSC to be combined into the same interface? Splitting them would make the contracts easier to track in all places

Yep that's true and makes sense, I was trying to keep them together as similarly-scoped pieces of information. I think this relates to a larger CompositionLocal question:

If we keep them separate, then that's asking for more window-layout information to be passed around everywhere in the app through different layers. But we keep explicitness, and it's easy to replace in previews and tests.

If we instead provide them in a CompositionLocal, then we lose the explicitness,and previews and testing is more difficult (needing to override a CompositionLocal instead of passing it manually). But, we avoid having to pass them through everywhere, and we can also make it easier to have the strategies be fold-aware by default.

Regarding the split, I think splitting them we don't necessarily ask for more movements, but it might feel like because we operate with two entities instead of one. With that in mind, we might notice that we don't propagate WSC where it is not needed, so in that matter we actually have better-SRP-ed API that asks only for what it needs.

CompositionLocal, as you mentioned, adds some implicit-y and we can always fall back to this option if we want to. Do you think it would make sense to try without it? If developers want, they can do it themselves easy-peasy. Also if you want to read this composition local, you will have to deal with the @composables everywhere here in the API, which I'm not sure is a good idea for such a flexible API surface as we have right now.

Could you try to split them and we can see how it goes? Maybe then it would warrant for a composition local for the display features or the object that contains them? And we can keep it as a parameter on the two pane so that we can default to the composition local, and allow to override for previews and tests.

Updated to split up WindowGeometry, and depend just on the list of DisplayFeatures!

adaptive/src/main/java/com/google/accompanist/adaptive/TwoPane.kt

matvei-g

Thanks, we are getting close! A few more commentarinos

adaptive/src/main/java/com/google/accompanist/adaptive/TwoPane.kt

matvei-g · 2022-08-09T13:23:15Z

adaptive/src/main/java/com/google/accompanist/adaptive/TwoPane.kt

+ */
+public fun TwoPaneStrategy(
+    windowGeometry: WindowGeometry,
+    defaultStrategy: TwoPaneStrategy,


I like the name default more. What I was wondering if we need this wrapping at all? Having to create a strategy and provide a default one for the non-fold seems like an unnecessary step for developers. They might (should) use the other ones, but this function take the best name in the space.

What do you think if we have just remove this from public api?

If we remove this one from the public API, then I think we would need to expose how to create the fold strategies to allow for the same behavior, which would also mean exposing the idea of a strategy that may not apply. Not sure if we'll be able to completely avoid having a default.

In the latest commit, I was able to keep current functionality intact while removing the direct fallback support from the public API. Let me know what you think!

matvei-g · 2022-08-09T13:24:15Z

adaptive/src/main/java/com/google/accompanist/adaptive/TwoPane.kt

+ * Otherwise, the gap will be placed at the given [splitFraction] from start, with the given
+ * [gapWidth].
+ */
+public fun HorizontalTwoPaneStrategy(


You can probably half the number of APIs if you provide an orientation: Orientation parameter.

I think I'd prefer to keep it is as-is. Having the name split means the parameter names can be orientation specific (gapWidth vs gapHeight) and clearer behavior when it comes to LTR and RTL.

ack, let's roll with it!

matvei-g · 2022-08-09T13:28:24Z

adaptive/src/main/java/com/google/accompanist/adaptive/TwoPane.kt

+public fun HorizontalTwoPaneStrategy(
+    windowGeometry: WindowGeometry,
+    splitOffset: Dp,
+    offsetFromStart: Boolean,


Seems like this might have a reasonable default, perhaps, true?

Also, I cannot understand why, but having this boolean makes me want to split the functions into FixedStartTwoPaneStrategy(orientation:Orientation, ...) and FixedEndTwoPaneStrategy.

Updated to have default of true!

matvei-g · 2022-08-09T13:36:36Z

adaptive/src/main/java/com/google/accompanist/adaptive/TwoPane.kt

+    fallbackStrategy: TwoPaneStrategy,
+    windowGeometry: WindowGeometry,
+): TwoPaneStrategy = TwoPaneStrategy { density, layoutDirection, layoutCoordinates ->
+    val verticalFold = windowGeometry.displayFeatures.find {


Regarding the split, I think splitting them we don't necessarily ask for more movements, but it might feel like because we operate with two entities instead of one. With that in mind, we might notice that we don't propagate WSC where it is not needed, so in that matter we actually have better-SRP-ed API that asks only for what it needs.

CompositionLocal, as you mentioned, adds some implicit-y and we can always fall back to this option if we want to. Do you think it would make sense to try without it? If developers want, they can do it themselves easy-peasy. Also if you want to read this composition local, you will have to deal with the @composables everywhere here in the API, which I'm not sure is a good idea for such a flexible API surface as we have right now.

Could you try to split them and we can see how it goes? Maybe then it would warrant for a composition local for the display features or the object that contains them? And we can keep it as a parameter on the two pane so that we can default to the composition local, and allow to override for previews and tests.

matvei-g

Thanks Alex, LGTM given the comments are addressed, I think this is a good API to try out!

I took a look at the layouting logic impl-wise, seems pretty straighforward. If you want me to take a look at some other implementation bits - let me know

matvei-g · 2022-08-10T13:09:46Z

adaptive/src/main/java/com/google/accompanist/adaptive/TwoPane.kt

+ * Otherwise, the gap will be placed at the given [splitFraction] from start, with the given
+ * [gapWidth].
+ */
+public fun HorizontalTwoPaneStrategy(


ack, let's roll with it!

matvei-g · 2022-08-10T13:12:30Z

adaptive/src/main/java/com/google/accompanist/adaptive/TwoPane.kt

+    strategy: TwoPaneStrategy,
+    displayFeatures: List<DisplayFeature>,
+    modifier: Modifier = Modifier,
+    foldAwareConfiguration: FoldAwareConfiguration = FoldAwareConfiguration.AllFolds,


Curious why this is not on the public TwoPaneStrategies? Do you think it would be a better API?

If this was on each of the TwoPaneStrategy s, it would be a bit more awkward to define a custom TwoPaneStrategy since it would have to be fold aware. Placing it on the single TwoPane entry point means that at least one type of fold has to be handled, and the rest of the strategies don't need to directly care about folds.

bentrengrove

Nice work!

alexvanyo added 3 commits August 4, 2022 18:35

Add adaptive project shell

27da5f5

Add WindowGeometry and TwoPane

f9a9619

Add basic TwoPane sample

1cb1c31

matvei-g reviewed Aug 5, 2022

View reviewed changes

alexvanyo added 3 commits August 5, 2022 14:31

Add more documentation to SplitResult

a7ce62a

Remove DelegateTwoPaneStrategy

a2f952c

split = verb, gap = noun (splitting creates a gap)

26d1a55

alexvanyo force-pushed the av/adaptive-two-pane branch from eda9b9c to 26d1a55 Compare August 5, 2022 21:59

Expose fold-aware entry points only

98139d1

alexvanyo force-pushed the av/adaptive-two-pane branch from dfd4bc3 to 98139d1 Compare August 5, 2022 23:20

matvei-g reviewed Aug 9, 2022

View reviewed changes

alexvanyo added 2 commits August 9, 2022 14:03

Remove WindowGeometry and consolidate folding support

266226b

Remove material3 library

5e01d34

matvei-g approved these changes Aug 10, 2022

View reviewed changes

Update to latest Compose dependencies

11fee5e

alexvanyo requested a review from bentrengrove August 10, 2022 18:11

Merge remote-tracking branch 'upstream/main' into av/adaptive-two-pane

fe020d8

bentrengrove approved these changes Aug 11, 2022

View reviewed changes

alexvanyo changed the title ~~[Adaptive] Add accompanist/adaptive library with WindowGeometry and TwoPane implementation.~~ [Adaptive] Add accompanist/adaptive library with TwoPane implementation. Aug 11, 2022

alexvanyo merged commit 3e99592 into main Aug 11, 2022

alexvanyo deleted the av/adaptive-two-pane branch August 11, 2022 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Adaptive] Add accompanist/adaptive library with TwoPane implementation. #1281

[Adaptive] Add accompanist/adaptive library with TwoPane implementation. #1281

alexvanyo commented Aug 5, 2022 •

edited

matvei-g left a comment

matvei-g Aug 5, 2022

alexvanyo Aug 5, 2022

matvei-g Aug 9, 2022

alexvanyo Aug 9, 2022

matvei-g left a comment

matvei-g Aug 9, 2022

alexvanyo Aug 9, 2022

alexvanyo Aug 9, 2022

matvei-g Aug 9, 2022

alexvanyo Aug 9, 2022

matvei-g Aug 10, 2022

matvei-g Aug 9, 2022

alexvanyo Aug 9, 2022

matvei-g Aug 9, 2022

matvei-g left a comment

matvei-g Aug 10, 2022

matvei-g Aug 10, 2022

alexvanyo Aug 10, 2022

bentrengrove left a comment

[Adaptive] Add accompanist/adaptive library with TwoPane implementation. #1281

[Adaptive] Add accompanist/adaptive library with TwoPane implementation. #1281

Conversation

alexvanyo commented Aug 5, 2022 • edited

matvei-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matvei-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matvei-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bentrengrove left a comment

Choose a reason for hiding this comment

alexvanyo commented Aug 5, 2022 •

edited