Fix Spacer bug with mutation
For Spacer with no Modifiers we need to set minimum dimension,
however was recently changed to be done after checking LayoutParams
which can be null in valid case (in mutation, when parent is not
there/doesn't have a diff. For those cases, we should always first
apply minimum dimension (preserving the old behaviour) and then
update LayoutParams if needed.
Additionally, resetting LayoutParams to 0 for LinearDimension before
adding View to its parent so that's correctly propagated and not
have previous value in mutation.
Bug: N/A
Test: Added
Change-Id: Iff36850372a29e11f9049ef3386a3e11a07d7f82
diff --git a/wear/protolayout/protolayout-renderer/src/main/java/androidx/wear/protolayout/renderer/inflater/ProtoLayoutInflater.java b/wear/protolayout/protolayout-renderer/src/main/java/androidx/wear/protolayout/renderer/inflater/ProtoLayoutInflater.java
index 9df108f..993dd38 100644
--- a/wear/protolayout/protolayout-renderer/src/main/java/androidx/wear/protolayout/renderer/inflater/ProtoLayoutInflater.java
+++ b/wear/protolayout/protolayout-renderer/src/main/java/androidx/wear/protolayout/renderer/inflater/ProtoLayoutInflater.java
@@ -2294,6 +2294,16 @@
// Modifiers cannot be applied to android's Space, so use a plain View if this Spacer has
// modifiers.
View view;
+
+ // Init the layout params to 0 in case of linear dimension (so we don't get strange
+ // behaviour before the first data pipeline update).
+ if (spacer.getWidth().hasLinearDimension()) {
+ layoutParams.width = 0;
+ }
+ if (spacer.getHeight().hasLinearDimension()) {
+ layoutParams.height = 0;
+ }
+
if (spacer.hasModifiers()) {
view =
applyModifiers(
@@ -2311,9 +2321,6 @@
parentViewWrapper.maybeAddView(view, layoutParams);
if (spacer.getWidth().hasLinearDimension()) {
- // Init the layout params' width to 0 (so we don't get strange behaviour before the
- // first data pipeline update).
- layoutParams.width = 0;
handleProp(
spacer.getWidth().getLinearDimension(),
width -> {
@@ -2331,9 +2338,6 @@
}
if (spacer.getHeight().hasLinearDimension()) {
- // Init the layout params' width to 0 (so we don't get strange behaviour before the
- // first data pipeline update).
- layoutParams.height = 0;
handleProp(
spacer.getHeight().getLinearDimension(),
height -> {
@@ -2356,6 +2360,11 @@
handleProp(
spacer.getWidth().getLinearDimension(),
width -> {
+ // Update minimum width first, because LayoutParams could be null.
+ // This calls requestLayout.
+ int widthPx = safeDpToPx(width);
+ view.setMinimumWidth(widthPx);
+
// We still need to update layout params in case other dimension is
// expand, so 0 could
// be miss interpreted.
@@ -2365,9 +2374,7 @@
return;
}
- lp.width = safeDpToPx(width);
- // This calls requestLayout.
- view.setMinimumWidth(safeDpToPx(width));
+ lp.width = widthPx;
},
posId,
pipelineMaker);
@@ -2376,6 +2383,11 @@
handleProp(
spacer.getHeight().getLinearDimension(),
height -> {
+ // Update minimum height first, because LayoutParams could be null.
+ // This calls requestLayout.
+ int heightPx = safeDpToPx(height);
+ view.setMinimumHeight(heightPx);
+
// We still need to update layout params in case other dimension is
// expand, so 0 could be miss interpreted.
LayoutParams lp = view.getLayoutParams();
@@ -2384,9 +2396,7 @@
return;
}
- lp.height = safeDpToPx(height);
- // This calls requestLayout.
- view.setMinimumHeight(safeDpToPx(height));
+ lp.height = heightPx;
},
posId,
pipelineMaker);
diff --git a/wear/protolayout/protolayout-renderer/src/test/java/androidx/wear/protolayout/renderer/inflater/ProtoLayoutInflaterTest.java b/wear/protolayout/protolayout-renderer/src/test/java/androidx/wear/protolayout/renderer/inflater/ProtoLayoutInflaterTest.java
index 0a66ff6..4fa4807 100644
--- a/wear/protolayout/protolayout-renderer/src/test/java/androidx/wear/protolayout/renderer/inflater/ProtoLayoutInflaterTest.java
+++ b/wear/protolayout/protolayout-renderer/src/test/java/androidx/wear/protolayout/renderer/inflater/ProtoLayoutInflaterTest.java
@@ -78,6 +78,7 @@
import android.widget.TextView;
import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
import androidx.core.content.ContextCompat;
import androidx.core.view.accessibility.AccessibilityNodeInfoCompat;
import androidx.test.ext.junit.runners.AndroidJUnit4;
@@ -630,14 +631,75 @@
// Dimensions are in DP, but the density is currently 1 in the tests, so this is fine.
expect.that(tv.getMeasuredWidth()).isEqualTo(width);
expect.that(tv.getMeasuredHeight()).isEqualTo(height);
+ }
- // This tests that layoutParams weren't null in the case when there's no modifiers so that
- // minimum dimension is correctly set.
+ @Test
+ public void inflate_spacer_noModifiers_hasMinDim() {
+ int width = 10;
+ int height = 20;
+ LayoutElement root =
+ LayoutElement.newBuilder()
+ .setSpacer(
+ Spacer.newBuilder()
+ .setHeight(
+ SpacerDimension.newBuilder()
+ .setLinearDimension(dp(height)))
+ .setWidth(
+ SpacerDimension.newBuilder()
+ .setLinearDimension(dp(width))))
+ .build();
+
+ FrameLayout rootLayout = renderer(fingerprintedLayout(root)).inflate();
+
+ // Check that there's a single element in the layout...
+ assertThat(rootLayout.getChildCount()).isEqualTo(1);
+ View tv = rootLayout.getChildAt(0);
+
+ // This tests that minimum dimension is correctly set.
+ // Dimensions are in DP, but the density is currently 1 in the tests, so this is fine.
expect.that(tv.getMinimumWidth()).isEqualTo(width);
expect.that(tv.getMeasuredHeight()).isEqualTo(height);
}
@Test
+ public void inflate_spacer_afterMutation_hasMinDim() {
+ // Confirms that all dimensions are correctly set after mutation.
+ int width = 10;
+ int newWidth = width - 5;
+ int height = 20;
+ int newHeight = height - 6;
+
+ Layout layout1 = layoutBoxWithSpacer(width, height);
+ Layout layout2 = layoutBoxWithSpacer(newWidth, newHeight);
+
+ // Add initial layout.
+ Renderer renderer = renderer(layout1);
+ ViewGroup inflatedViewParent = renderer.inflate();
+
+ // Compute the mutation.
+ ViewGroupMutation mutation =
+ renderer.computeMutation(getRenderedMetadata(inflatedViewParent), layout2);
+ assertThat(mutation).isNotNull();
+ assertThat(mutation.isNoOp()).isFalse();
+
+ // Apply the mutation.
+ boolean mutationResult = renderer.applyMutation(inflatedViewParent, mutation);
+ assertThat(mutationResult).isTrue();
+
+ // This contains layout after the mutation.
+ ViewGroup boxAfterMutation = (ViewGroup) inflatedViewParent.getChildAt(0);
+ View spacerAfterMutation = boxAfterMutation.getChildAt(0);
+
+ // Dimensions are in DP, but the density is currently 1 in the tests, so this is fine.
+ expect.that(spacerAfterMutation.getMeasuredWidth()).isEqualTo(0);
+ expect.that(spacerAfterMutation.getMeasuredHeight()).isEqualTo(0);
+
+ // This tests that minimum dimension is correctly set.
+ expect.that(spacerAfterMutation.getMinimumWidth()).isEqualTo(newWidth);
+ expect.that(spacerAfterMutation.getMinimumHeight()).isEqualTo(newHeight);
+ }
+
+ @Test
public void inflate_spacerWithModifiers() {
int width = 10;
int height = 20;
@@ -671,6 +733,42 @@
}
@Test
+ public void inflate_spacer_withModifiers_afterMutation_hasMinDim() {
+ // Confirms that all dimensions are correctly set after mutation.
+ int width = 10;
+ int newWidth = width - 5;
+ int height = 20;
+ int newHeight = height - 6;
+ Modifiers.Builder modifiers =
+ Modifiers.newBuilder().setBorder(Border.newBuilder().setWidth(dp(2)).build());
+
+ Layout layout1 = layoutBoxWithSpacer(width, height, modifiers);
+
+ // Add initial layout.
+ Renderer renderer = renderer(layout1);
+ ViewGroup inflatedViewParent = renderer.inflate();
+
+ Layout layout2 = layoutBoxWithSpacer(newHeight, newWidth, modifiers);
+
+ // Compute the mutation.
+ ViewGroupMutation mutation =
+ renderer.computeMutation(getRenderedMetadata(inflatedViewParent), layout2);
+ assertThat(mutation).isNotNull();
+ assertThat(mutation.isNoOp()).isFalse();
+
+ // Apply the mutation.
+ boolean mutationResult = renderer.applyMutation(inflatedViewParent, mutation);
+ assertThat(mutationResult).isTrue();
+
+ ViewGroup boxAfterMutation = (ViewGroup) inflatedViewParent.getChildAt(0);
+ View spacerAfterMutation = boxAfterMutation.getChildAt(0);
+
+ // Dimensions are in DP, but the density is currently 1 in the tests, so this is fine.
+ expect.that(spacerAfterMutation.getMeasuredWidth()).isEqualTo(0);
+ expect.that(spacerAfterMutation.getMeasuredHeight()).isEqualTo(0);
+ }
+
+ @Test
public void inflate_spacerWithDynamicDimension_andModifiers() {
int width = 100;
int widthForLayout = 112;
@@ -5749,4 +5847,27 @@
.setLayoutWeight(FloatProp.newBuilder().setValue(weight).build())
.build();
}
+
+ /** Builds a wrapper Box that contains Spacer with the given parameters. */
+ private static Layout layoutBoxWithSpacer(int width, int height) {
+ return layoutBoxWithSpacer(width, height, /* modifiers= */ null);
+ }
+
+ /** Builds a wrapper Box that contains Spacer with the given parameters. */
+ private static Layout layoutBoxWithSpacer(
+ int width, int height, @Nullable Modifiers.Builder modifiers) {
+ Spacer.Builder spacer =
+ Spacer.newBuilder()
+ .setWidth(SpacerDimension.newBuilder().setLinearDimension(dp(width)))
+ .setHeight(SpacerDimension.newBuilder().setLinearDimension(dp(height)));
+ if (modifiers != null) {
+ spacer.setModifiers(modifiers);
+ }
+ return fingerprintedLayout(
+ LayoutElement.newBuilder()
+ .setBox(
+ Box.newBuilder()
+ .addContents(LayoutElement.newBuilder().setSpacer(spacer)))
+ .build());
+ }
}
