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

Long lived branch with Overlay refinements #839

Merged
merged 3 commits into from
Jul 28, 2022
Merged

Long lived branch with Overlay refinements #839

merged 3 commits into from
Jul 28, 2022

Conversation

rjrjr
Copy link
Contributor

@rjrjr rjrjr commented Jul 20, 2022

Holding off on merging into main until I have more confidence in the changes, which I'm getting by applying them across Square's flagship Android apps; and until I'm ready to deal with the recent coroutines version update (#817).

Introduces OverlayDialogHolder.onUpdateBounds

OverlayDialogHolder.onUpdateBounds replaces ScreenOverlayDialogFactory.updateBounds.

I finally tried to use ScreenOverlayDialogFactory.updateBounds and was reminded once again why it's bad to create interfaces that build something and then take it back later to be updated. If you want the renderings to be involved in setting the bounds policy, there was no way to smuggle that information along with the Dialog returned from buildDialogWithContent, so that updateBounds could honor it. Also, it was always pretty ugly that the bounds hook was special to the ScreenOverlay world.

The fix is make bounds maintenance part of the job of the OverlayDialogHolder interface. Square reviewers will notice that we're now an even more faithful rip off of Mosaic's DialogRunner -- hi @helios175.

No more hard coded EnvironmentScreen in WorkflowLayout.

WorkflowLayout was wrapping things in EnvironmentScreen for no real reason. No longer.

@rjrjr rjrjr force-pushed the ray/better-bounds branch from 239ec29 to 5374705 Compare July 20, 2022 22:19
@rjrjr rjrjr changed the title OverlayDialogHolder.onUpdateBounds replaces `ScreenOverlayDialogFac… Introduces OverlayDialogHolder.onUpdateBounds Jul 20, 2022
@rjrjr rjrjr force-pushed the ray/better-bounds branch 5 times, most recently from 09fee95 to f6204fe Compare July 20, 2022 23:09
@rjrjr
Copy link
Contributor Author

rjrjr commented Jul 21, 2022

@steve-the-edwards Uh oh:

com.squareup.benchmarks.performance.complex.poetry.RenderPassTest > renderPassCounterFrameTimeoutComplexNoInitializingStateHighFrequencyEvents[test(AVD) - 10] FAILED 
	java.lang.AssertionError: Uh Oh! You have worsened the the number of Render Passes (lower better) by 2 (2.06%) for Runtime: FrameTimeout; the 'Raven navigation (no initializing state) scenario with high frequency events'! The value is now 99 (was 88..97). This likely results in worse performance. You can check with the timing benchmarks if you disagree.
	at org.junit.Assert.fail(Assert.java:89)

I'm surprised by this, and I don't see what I've done to cause it. Can I just bump the expected range, or do we need to dig in?

@rjrjr
Copy link
Contributor Author

rjrjr commented Jul 21, 2022

Re-running that shard to see if this is flaky.

@rjrjr
Copy link
Contributor Author

rjrjr commented Jul 21, 2022

Sure enough, it was a flake. :(

@rjrjr rjrjr marked this pull request as ready for review July 21, 2022 14:42
@rjrjr rjrjr requested review from a team and zach-klippenstein as code owners July 21, 2022 14:42
@rjrjr
Copy link
Contributor Author

rjrjr commented Jul 21, 2022

This is ready for review, although I might not merge it until I get to exercise it a bit harder as a snapshot (1.8.0-rjrjr2beta06-SNAPSHOT).

@steve-the-edwards
Copy link
Contributor

Sure enough, it was a flake. :(

If that was just a flake/within reasonable bounds I am fine with bumping up the expected range.

Copy link

@vgonda vgonda left a comment

Choose a reason for hiding this comment

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

:stamp:

@rjrjr rjrjr force-pushed the ray/better-bounds branch from f6204fe to d8ecc32 Compare July 22, 2022 20:43
@rjrjr
Copy link
Contributor Author

rjrjr commented Jul 26, 2022

I'm going to merge #840 into this branch, and hold off on merging into main until I have a bit more confidence in this series of changes. I'm already working on a refinement of our back button handling in Overlay, and expect to float another PR targeting this branch today.

@rjrjr rjrjr changed the title Introduces OverlayDialogHolder.onUpdateBounds Long lived branch with Overlay refinements Jul 26, 2022
rjrjr added 3 commits July 27, 2022 21:19
`OverlayDialogHolder.onUpdateBounds` replaces `ScreenOverlayDialogFactory.updateBounds`.

I finally tried to use `ScreenOverlayDialogFactory.updateBounds` and was reminded once again why it's bad to create interfaces that build something and then take it back later to be updated. If you want the renderings to be involved in setting the bounds policy, there was no way to smuggle that information along with the Dialog returned from `buildDialogWithContent`, so that `updateBounds` could honor it. Also, it was always pretty ugly that the bounds hook was special to the `ScreenOverlay` world.

The fix is make bounds maintenance part of the job of the `OverlayDialogHolder` interface. Square reviewers will notice that we're now an even more faithful rip off of Mosaic's DialogRunner -- hi @helios175.
WorkflowLayout was wrapping things in EnvironmentScreen for no real reason. No longer.
@rjrjr rjrjr force-pushed the ray/better-bounds branch from f96fb8e to 6d88a6a Compare July 28, 2022 04:19
@rjrjr
Copy link
Contributor Author

rjrjr commented Jul 28, 2022

I'm going to merge #840 into this branch, and hold off on merging into main until I have a bit more confidence in this series of changes. I'm already working on a refinement of our back button handling in Overlay, and expect to float another PR targeting this branch today.

#840 is being weird in CI, though everything passes locally and all our internal tests like it. I'm going to go ahead and merge this branch and target #840 at main, hoping that makes CI sweet again.

@rjrjr rjrjr merged commit 2594db0 into main Jul 28, 2022
@rjrjr rjrjr deleted the ray/better-bounds branch July 28, 2022 15:43
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.

4 participants