Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow any type of ScannableView #137

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

pyricau
Copy link
Contributor

@pyricau pyricau commented Mar 2, 2021

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 pyricau requested review from jamesmosley55 and a team as code owners March 2, 2021 23:40
@@ -29,7 +29,7 @@ public final class radiography/ScanScopes {
public static final fun singleViewScope (Landroid/view/View;)Lradiography/ScanScope;
}

public abstract class radiography/ScannableView {
public abstract interface class radiography/ScannableView {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also worth noting this break backward compatibility (as you can't switch over ScannableView anymore) but I don't expect any consumer to be impacted.

If we really really want to avoid that we could keep ScannableView as abstract class and make it implement an interface. But then I don't know how to name that interface

Copy link
Collaborator

@zach-klippenstein zach-klippenstein Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's possible to add direct subclasses to a sealed class in any way without technically breaking compatibility. Any consuming bytecode that assumes it has handled all the cases will be wrong.

Technically I think this means we need to bump the major version number.

Copy link
Collaborator

@zach-klippenstein zach-klippenstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -15,17 +11,15 @@ import java.util.concurrent.TimeUnit.SECONDS

/**
* Utility class to scan through a view hierarchy and pretty print it to a [String].
* Call [scan] or [View.scan].
* Call [scan] or [android.view.View.scan].
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the kotlin formatter should keep the import even if it's only used by kdoc.

if (windowTitle != null) {
return "$classSimpleName in $windowTitle window-focus:${view.hasWindowFocus()}"
} else {
return classSimpleName
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not report focus in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's no window title, then this isn't the root view for an attached window. In which case window focus doesn't make sense. Good question though, should add as comment.

@pyricau
Copy link
Contributor Author

pyricau commented Mar 3, 2021

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
Copy link
Contributor Author

pyricau commented Mar 3, 2021

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.

Copy link
Collaborator

@zach-klippenstein zach-klippenstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think I was missing the hint that this was just prep for removing the Android dependency. Thanks for splitting up the commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants