willowtreeapps / Hyperion-Android

App Debugging & Inspection Tool for Android
MIT License
1.95k stars 144 forks source link

A better way to inject the hyperion views into the layout hierarchy. #157

Closed evant closed 5 years ago

evant commented 5 years ago

Many issues we've run into with accessibility, focus, etc. are due to hyperion injecting views into the layout hierarchy even when it's not shown. To fix this, no views are added until hyperion is shown. And when it is, the additional views are added to the window's content view keeping the app content in place.

Behavior changes: 1) A plugin's view is not always guaranteed to be attached to the layout, as it's attached when the menu is shown and detached when the menu is hidden. This can happen multiple times between when a plugin is created and destroyed. 2) MeasurementHelper#getScreenLocation coordinates may not line up with with the content view when used in an overlay. This is because the overlay is now placed at the same level as the content view so it might be moved down a bit to make room for the status bar etc. You can use MeasurementHelper#getContentRootLocation` instead which will get you coordinates relative to the content root (and the overlay).

Known issues: There a some graphical glitches in the sample app that needs to be investigated.

ToxicBakery commented 5 years ago

I believe this branch is a good example of why it would be great to get snapshots rolling on hyperion such that this branch could simply be manually pushed into the repo and a snapshot used for testing the changes. Granted, this would be easier if the project was configured through Circle as well. I'm here to help if you want to consider a route like that, I have a lot of experience with my public libraries.

Kritarie commented 5 years ago

@ToxicBakery I like that idea. I don't have the bandwidth at the moment to get that setup, but I'm open to suggestions. Would it be as simple as editing the circle .yml to automatically publish on master builds? For the moment I'm thinking we should go ahead and merge this.

ToxicBakery commented 5 years ago

Effectively that simple, yes. As always though it's a few key steps.

1) The build needs to properly version the artifacts such that they are labeled snapshot if they are not being built on the master branch (typical configuration) 2) You'll need to configure circle with a context that includes OSS credentials for publishing the snapshot. This is pretty trivial 3) Add publish to the gradlew task list in the yml file

Because of the way circle handles security, you'll want the script to only attempt publishing on builds triggered by this repo. For example: https://github.com/ToxicBakery/Arbor/blob/master/.circleci/config.yml#L23

You'll also note I'm writing the signing information out as I let circle publish my release artifacts for me to OSS but that's optional. You can just as easily skip signing and mark all CI builds as snapshots. Snapshots on OSS do not require signing however I sign them anyways in my projects.

Next, subprojects is an easy way to hook into setting the build version. Again, most of this is optional but the -SNAPSHOT bit is the :key: https://github.com/ToxicBakery/Arbor/blob/master/build.gradle#L27

Lastly, you want to configure the repositories inside the publishing DSL. This is where the OSS credentials come in to play and additionally the usage of the -SNAPSHOT identifier. Technically you could just define both the release repository and snapshot repository separately and then use the corresponding publish task for the snapshot repo but this was again just a personal preference on my part to always simply call publish and publishToMavenLocal and never bother remembering anything else than those two. https://github.com/ToxicBakery/Arbor/blob/master/common/build.gradle#L290

Kritarie commented 5 years ago

@ToxicBakery Thanks for the info! I think a few of us here are going to look into setting this up in the near future for a couple of our OSS projects including Hyperion.

ToxicBakery commented 5 years ago

That's great news. I look forward to it!

Also, nice work on this PR @evant.