radiography icon indicating copy to clipboard operation
radiography copied to clipboard

Allow any type of ScannableView

Open pyricau opened this issue 4 years ago • 2 comments

Note: there's more work to make this not Android dependent. Notably refs to the WindowManager which likely belong to displayName for Views instead (?)

pyricau avatar Mar 02 '21 23:03 pyricau

I'm not sure I understand why unsealing the class has to be done together with removing thread checks.

And removing the threading guarantees seems like it could break stuff - remind me why we don't care?

Great questions!

So I started with the unsealing but then realized that for any of this to make sense, I really wanted to decouple Radiography from requiring a dependency on Android. Hence the follow up commits that remove dependencies on Looper and then window manager.

I can totally break those out in separate PRs, I was being lazy.

That being said, I believe both changes are generally good + fixing tech debt remnants, here's why:

  • Before adding curtains, we were scanning the root views on the calling thread and then using the looper thread of each root view to scan the hierarchy. That's wrong for two reasons: a) we can't access the WindowManagerGlobal lock so we should stick access to the main thread as that's what all android code uses to add / remove from the root views. b) it doesn't make sense to have a distinct looper for each root view, or post N times (once per view) if we're on a background thread. Also compose doesn't necessarily have the same requirements. So I would rather have the ScanScopes implement their own threading policies (e.g. curtains throwing if calling from non main thread) and the calling code be responsible for posting to the main thread to gather the entire response.
  • Printing the window title and window focus is useful when the root view is a window root view. If it's a composable, or a non window root view, that doesn't really make sense. Notice how the compose tests have this duplicated string at the top of the print.

pyricau avatar Mar 03 '21 14:03 pyricau

And so of course a follow up change prior to releasing would be to make the Android dependency optional and prove that it works. I'm thinking we might not actually need a separate module, which would be annoying as our default params in Radiogaphy.scan are Android centric. I believe the main use case is still mostly Android so I don't want to make that harder. But if I provide all the params then I'm hoping there will be no attempt to load the android specific classes and this can run on a JVM. Need to confirm though.

Another option might be to make a new entry point but I'm not sure how it'd be different. Unless we go with same name an API and you have to choose your artifact, and the only difference is in the defaults?

Or we could get rid of the defaults (major version bump) but that seems like it'd make the core use case harder.

pyricau avatar Mar 03 '21 14:03 pyricau