Expose call group chain information #160
Conversation
|
So if I'm following you, the logs should be different if |
|
@rjrjr not quite, the current logs should not change at all, and current users won't need to opt in to UiToolingDataApi::class What is new, is that there is now a property Usage can be seen inside the test https://github.com/square/radiography/pull/160/files#diff-0a37f7ae94183d0cefb9e8ba89d53a9409011f55230e69ad4300cacee413734eR161 If someone does want to use the sourceLocation property they will need to opt in to UiToolingDataApi::class. I considered that we may want to create a wrapper class to avoid this, but I think directly exposing this tooling class is fine because if it does change we would need to change the wrapper anyway in potentially breaking ways, and having the user opt in makes them aware of the source of the data and that it is potentially unstable. |
|
LGTM (and thanks!), but could you put that explanation in the kdoc for |
| .containsExactly("Call3", "Call2", "Call1", "BasicText", "Layout", "ReusableComposeNode") | ||
| assertThat(callGroup.callChain.map { it.location?.sourceFile }) | ||
| .containsExactly("ComposeViewTest.kt", "ComposeViewTest.kt", "ComposeViewTest.kt", "ComposeViewTest.kt", "BasicText.kt", "Layout.kt") | ||
| } |
There was a problem hiding this comment.
This test seems to be testing several things (at least 2) and should be broken up to test one thing per test.
|
Would you mind updating the README with examples of how to get one output vs the other more detailed output? I'm a little confused. |
|
@rjrjr |
Nope, nevermind, I was being an idiot. I could have sworn I saw you introducing it in this PR. |
|
@pyricau I've split the test and updated the readme with explanations and examples |
elihart
force-pushed
the
eli-add_call_chain_info
branch
from
09ee1ea to
c92245b
Compare
|
@rjrjr The instrumented tests for API 30 seem to be failing, but I believe it is unrelated to my changes. It looks like the last commit to main also failed like this 
I ran the tests with an api 30 emulator locally and can't reproduce |
Welcome to our espresso hell. We'll figure it out. |
|
I too can't reproduce this problem locally, on I do notice that the tests are taking a very long time to run, especially including AVD startup; and that |
Experimenting here: #161 |
|
It's very clearly a flake predating this work. I'm going to go ahead and merge it. And then we can play the "how do we release this thing again?" game! |
I completely agree. Good idea! |
|
Hey @elihart, I think I finally fixed our CI. How would you like a release at long last? |
|
A release would be great @rjrjr ! Thanks for following up :) |
Our app relies heavily on lots of decomposition within Compose UI in order to organize UI and promote code reuse. However, this means we have lots of Compose functions which are basically wrapper functions and there can be quite a lot of functional nesting like that.
I was quite confused for a while about why these functions didn't show up in the Radiography output, or would show up sporadically. If we have a ui module that exports a compose "component" we want that component name to identify the usage of the component in the radiography output. However, currently this doesn't reliably happen and instead we mostly see more primitive elements show up in the output scan.
The code comment on
computeLayoutInfosthankfully explains why this happens - only the nodes that emit layouts are represented in the final output, and they inherit the name of the top most call group wrapping them. I can see why this was done, to prevent very verbose chains, but unfortunately it doesn't always result in the best output.This PR tries to expose more granular information about these call chains so that users can optionally get more insight into what function calls were made. I also included the source location of these function calls so that our tooling can make use of that to make more educated guesses about which functions to surface (eg, if we know its a file in our project, include the call name). To acces this, there is now a property
val callChain: List<CallGroupInfo>on ComposeView which can be used to get information about all of the call groups wrapping the layout node.Usage can be seen inside the test: https://github.com/square/radiography/pull/160/files#diff-0a37f7ae94183d0cefb9e8ba89d53a9409011f55230e69ad4300cacee413734eR161
The original behavior in terms of which display names are used is unchanged, and in general there is no impact to existing users. This can be seen in the screenshot of the compose app before and after, and all existing tests pass without changes needed. I did add a test to verify the call chain information is captured correctly.
I hope that this additional capability is welcome, but I am open to suggestions about how to modify the approach to better fit the direction of the library.
Before:
After: (note there are no differences)
