LayoutNode.detach() should do more work [150774014]

Fixed

Bug

Status Update

No update yet.

Description

an...@google.com

created issue #1

Mar 4, 2020 05:24PM

Currently in DrawNode.detach() we call `owner?.onInvalidate(this)`
As we moving towards modifiers looks like now LayoutNode.detach() should be calling this as well making sure the parent is invalidated when we remove from the tree LayoutNode which was drawing via DrawModifier as otherwise we can continue displaying this LayoutNode as it was not removed
WDYF?

Comments

mo...@google.com <mo...@google.com> #2Mar 4, 2020 11:39PM

Reassigned to an...@google.com.

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

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

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

an...@google.com <an...@google.com> #3Mar 5, 2020 11:28AM

Reassigned to mo...@google.com.

I think there are could be some corner cases when it is not enough.
when some LayoutNode which draws being removed we want to invalidate the RepaintBoundary containing it.
instead we do parentLayoutNode.onRequestMeasure, which iterates through all the parents it affects and then calls or requestLayout or invalidateRepaintBoundary for this grandParent. In first case requestLayout will cause invalidation of out AndroidComposeView and in the second invalidateRepaintBoundary can be potentially happening on other RepaintBoundary which wraps our grandparent. So the RepaintBoundary wrapping our original LayoutNode can be left not being redrawn.
right?

mo...@google.com <mo...@google.com> Mar 5, 2020 07:19PM

Accepted by mo...@google.com.

ap...@google.com <ap...@google.com> #4Mar 6, 2020 08:18PM

Marked as fixed.

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

commit b0286ae80bb8901cc834a88d5fe66f55f4817de3
Author: George Mount <mount@google.com>
Date: Thu Feb 20 15:14:09 2020

Add support for a LayerModifier.

Fixes: 150774014

This is the first step of changing RepaintBoundaryNode
to use modifiers instead. The next step is to replace
all use of RepaintBoundaryNode.

Relnote: "Added LayerModifier, a modifier that allows
adding a RenderNode for a Layout. It allows setting
clipping, opacity, rotation, scaling, and shadows.
This will replace RepaintBoundary."

Test: ran AndroidLayoutDrawTest

Change-Id: I7100dfe7a795567a48c2d5b3094e3dbd47e0f9c7

M ui/ui-core/api/0.1.0-dev07.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev07.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev07.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/build.gradle
A ui/ui-core/integration-tests/samples/src/main/java/androidx/ui/core/samples/LayerModifierSamples.kt
A ui/ui-core/src/main/java/androidx/ui/core/DrawLayerModifier.kt
M ui/ui-framework/src/androidTest/java/androidx/ui/core/test/AndroidLayoutDrawTest.kt
M ui/ui-geometry/api/0.1.0-dev07.txt
M ui/ui-geometry/api/current.txt
M ui/ui-geometry/api/public_plus_experimental_0.1.0-dev07.txt
M ui/ui-geometry/api/public_plus_experimental_current.txt
M ui/ui-geometry/api/restricted_0.1.0-dev07.txt
M ui/ui-geometry/api/restricted_current.txt
M ui/ui-geometry/src/main/java/androidx/ui/geometry/RRect.kt
M ui/ui-material/src/main/java/androidx/ui/material/Drawer.kt
M ui/ui-platform/api/0.1.0-dev07.txt
M ui/ui-platform/api/current.txt
M ui/ui-platform/api/public_plus_experimental_0.1.0-dev07.txt
M ui/ui-platform/api/public_plus_experimental_current.txt
M ui/ui-platform/api/restricted_0.1.0-dev07.txt
M ui/ui-platform/api/restricted_current.txt
M ui/ui-platform/src/main/java/androidx/ui/core/AndroidOwner.kt
M ui/ui-platform/src/main/java/androidx/ui/core/ComponentNodes.kt
M ui/ui-platform/src/main/java/androidx/ui/core/LayoutNodeWrapper.kt
A ui/ui-platform/src/main/java/androidx/ui/core/OutlineResolver.kt
A ui/ui-platform/src/main/java/androidx/ui/core/OwnedLayer.kt
A ui/ui-platform/src/main/java/androidx/ui/core/RenderNodeLayer.kt
A ui/ui-platform/src/main/java/androidx/ui/core/ViewLayer.kt

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

Issue 150774014

Description

Issue summary

Comments

mo...@google.com <mo...@google.com> #2Mar 4, 2020 11:39PM

an...@google.com <an...@google.com> #3Mar 5, 2020 11:28AM

mo...@google.com <mo...@google.com> Mar 5, 2020 07:19PM

ap...@google.com <ap...@google.com> #4Mar 6, 2020 08:18PM

Add comment

Issue metadata