RUM-9298: Implement Compose actions tracking strategy#2586
Conversation
ae0908d to
486e6b7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/actions-tracking #2586 +/- ##
============================================================
+ Coverage 70.15% 70.16% +0.01%
============================================================
Files 809 810 +1
Lines 30245 30332 +87
Branches 5053 5076 +23
============================================================
+ Hits 21218 21282 +64
- Misses 7622 7629 +7
- Partials 1405 1421 +16
🚀 New features to boost your workflow:
|
486e6b7 to
b8e0d7c
Compare
b8e0d7c to
942c5b1
Compare
0xnm
left a comment
0xnm
left a comment
There was a problem hiding this comment.
nice work! I left some questions, bit overall lgtm.
| } catch (@Suppress("TooGenericExceptionCaught") e: Exception) { | ||
| // All exceptions should be caught and return in case of crash. | ||
| logAddQueueException(e) | ||
| // The iteration of the layout nodes tree has failed, return null. | ||
| return null | ||
| } |
There was a problem hiding this comment.
in which case we can end up here? like owner.root call can throw?
There was a problem hiding this comment.
mostly because queue.add can throw exception, also owner.root is an internal call, it can also throw runtime exception if there is any binary compatibility issue.
| } catch (@Suppress("TooGenericExceptionCaught") e: Exception) { | ||
| // All exceptions should be caught and return in case of crash. | ||
| logAddQueueException(e) | ||
| // The iteration of the layout nodes tree has failed, return null. |
There was a problem hiding this comment.
is it owner.root call who does the iteration under the hood?
There was a problem hiding this comment.
No, owner is ususally AndroidComposeView, which is the root container of the whole compose view, so owner.root just simply returns the root layout node of compose, and we do the iteration from this node with while loop just below.
| } | ||
|
|
||
| private fun logAddQueueException(e: Exception) { | ||
| (Datadog.getInstance() as? FeatureSdkCore)?.internalLogger?.log( |
There was a problem hiding this comment.
We should use the instance for the feature. Or is it like that because of the initialization sequence?
There was a problem hiding this comment.
we have instance per feature? how to get it from here?
There was a problem hiding this comment.
yes, you normally have and instance of FeatureSdkCore passed during the RumFeature initialization, and this is the instance which should be used.
Is there a chance to get it here? But I understand, that it may be not available when we just create a configuration object.
There was a problem hiding this comment.
yes we create it when building the configuration, so we don't have the instance of FeatureSdkCore here
There was a problem hiding this comment.
maybe it is worth then also implement com.datadog.android.rum.tracking.TrackingStrategy interface? There you can get SdkCore instance, you can check how ActivityLifecycleTrackingStrategy is using it, for example.
There was a problem hiding this comment.
updated, now I make ActionTrackingStrategy extend TrackingStrategy and the registration of sdkCore happens in GesturesListener
942c5b1 to
06cd701
Compare
06cd701 to
b997aa7
Compare
| composeActionTrackingStrategy.register(sdkCore, it) | ||
| androidActionTrackingStrategy.register(sdkCore, it) |
There was a problem hiding this comment.
normally we need to unregister as well, but I guess if SDK core is stopped, then it won't be referenced from user code and even though it will have a reference from action tracking strategies, it will be still deallocated.
| * 3(c) 4 | ||
| */ | ||
| @Test | ||
| fun `M return lowest level clickable node W findTargetForTap`(forge: Forge) { |
There was a problem hiding this comment.
minor: let's use bottom-most instead of lowest level, seems like it is the opposite of the quite common top-most.
| * 3(c) 4 | ||
| */ | ||
| @Test | ||
| fun `M return most inisde scrollable node W findTargetForScroll`(forge: Forge) { |
There was a problem hiding this comment.
| fun `M return most inisde scrollable node W findTargetForScroll`(forge: Forge) { | |
| fun `M return bottom-most scrollable node W findTargetForScroll`(forge: Forge) { |
b997aa7 to
9543ce6
Compare
3 participants
What does this PR do?
Implement
ComposeActionsTrackingStrategyto find the target of action in Jetpack Compose.The main idea of the whole process:
GestureListenerreceives the motion event first and start to iterate the view tree from the decor view of the window.AndroidActionTrackingStrategyto simply test the view is clickable/scrollable and hit by the action.ComposeActionTrackingStrategyto continue the iteration if the view is aOwnerof compose layout node.ComposeActionTrackingStrategy, there is no more view tree, instead we can still iterate the layout node tree to find out which node is clickable/scrollable and hit by the motion event coordinates.Motivation
RUM-9298
Additional Notes
Just in case of confusion, there are some back and forth from previous PR, the logic of iterating the view tree was moved out from
GesturesListenertoAndroidActionTrackingStrategy, now it is moved back toGestureListener, because before finding the compose view, we still need to iterate fromDecorViewtoAndroidComposeView, so this part should always be the common logic between Android view and Jetpack Compose.Review checklist (to be filled by reviewers)