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

New feature, viewOnly, to ignore the device frame #591

Closed
wants to merge 1 commit into from

Conversation

swankjesse
Copy link
Collaborator

@swankjesse swankjesse commented Sep 30, 2022

This works in two ways:

  • Just viewOnly omits the system bars
  • viewOnly with RenderingMode.EXPAND also changes the output size to match the view

Closes: #37

This works in two ways:

 - Just viewOnly omits the system bars
 - viewOnly with RenderingMode.EXPAND also changes
   the output size to match the view
@swankjesse
Copy link
Collaborator Author

If ever we do a 2.x, it’d be nice to remove com.android.ide.common.rendering.api.SessionParams.RenderingMode from Paparazzi’s public API and instead add a new thing that encapsulates both viewsOnly and that. As-is the two features are a bit weird as there’s some combinations that don‘t do much.

@swankjesse
Copy link
Collaborator Author

Some interesting outputs!

This button renders just the pixels we need. It’s still scaled up to be 1000px wide, and that’s bad! I’m hoping to address that in the interceptors follow-up. (#589)
app cash paparazzi_ViewOnlyTest_viewSmallerThanDeviceResolution_full_expand-viewonly

This button word-wraps at the width of the target device. This is using viewsOnly = true with H_SCROLL:
app cash paparazzi_ViewOnlyTest_viewLargerThanDeviceResolution_v_scroll-viewonly

This ultrawide button doesn’t word wrap at all. It’s using viewsOnly = true with FULL_EXPAND.
app cash paparazzi_ViewOnlyTest_viewLargerThanDeviceResolution_full_expand-viewonly

This button renders in a canvas as if there’s a device frame, cause we’re using RenderMode.NORMAL. I don’t think this is a good combination.
app cash paparazzi_ViewOnlyTest_viewSmallerThanDeviceResolution_normal-viewonly

@@ -89,7 +93,8 @@ class Paparazzi @JvmOverloads constructor(
private val maxPercentDifference: Double = 0.1,
private val snapshotHandler: SnapshotHandler = determineHandler(maxPercentDifference),
private val renderExtensions: Set<RenderExtension> = setOf(),
private val supportsRtl: Boolean = false
private val supportsRtl: Boolean = false,
private val viewOnly: Boolean = false
Copy link
Collaborator

@saket saket Sep 30, 2022

Choose a reason for hiding this comment

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

viewOnly is reminding me of file attributes such as read only.

Considering that our usages of the term View is going to only go down as we slowly write everything as composables, can I propose using something agnostic instead? maybe hideDeviceFrame?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call. Let’s workshop a new name.

Two consequences:

  • no frame
  • no device dimensions

Any other suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another suggestion is to match what Android Studio uses for emulators -- enableDeviceFrame

@BenedictP
Copy link

Hey, I also tried to solve this problem in my PR: #550
So some input from my side:

  • As you said it would be better to remove com.android.ide.common.rendering.api.SessionParams.RenderingMode because we don't control this class and there is missing functionality. I tried to solve it with another class instead just a boolean.
  • I think compose layouts are not affected by your change. Paparazzi.kt Line 216 is still using match_parent for height and width.
  • Our main use case is that we want to test Recycler Viewholders. Therefore we want to shrink the y-axis but not the x-axis to check how the view under test behaves on phone/tablet/fablet/foldables.

@rharter
Copy link

rharter commented Oct 13, 2022

This is also fixed by #497 and here are some of my thoughts:

  • I think your usage of RenderingMode is backwards. RenderingMode.SHRINK is documented to "Shrink canvas to the minimum size that is needed to cover the scene", which sounds like your interpretation of EXPAND. I think that KEEP is meant to match the size of the device, EXPAND is meant to allow the view to be rendered larger than the device (to support scrolling), and SHRINK is meant to shrink the canvas to match the view.
  • RenderingMode supports defining EXPAND/KEEP/SHRINK along both horizontal and vertical axes, but you seem to be conflating them here with the updated contentRoot.

@swankjesse
Copy link
Collaborator Author

I’m using EXPAND but setting the initial canvas size to 1x1. That way the snapshot can exceed the bounds of the configured device.

As I see them these are the interesting constraints.

  • Layout constraints. For each x,y dimension we can fix to the device, or do unbounded.
  • System bars on or off. These work best if height and width match the device.

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.

Snapshot without a device frame
5 participants