paparazzi icon indicating copy to clipboard operation
paparazzi copied to clipboard

Cropping out a small part of the screen results in large image.

Open TWiStErRob opened this issue 3 years ago • 5 comments

Using these two together:

public class CropViewRenderExtension implements RenderExtension {

	@Override
	public @NonNull BufferedImage render(
			@NonNull Snapshot snapshot,
			@NonNull View view,
			@NonNull BufferedImage image
	) {
		return image.getSubimage(0, 0, view.getMeasuredWidth(), view.getMeasuredHeight());
	}
}
	public void snapshotWithSize(@NonNull Paparazzi $this, @NonNull View view, @Dp float width, @Dp float height) {
		ViewGroup parent = new FrameLayout(view.getContext());
		float widthPx = dipToPix(view.getContext().getResources(), width);
		float heightPx = dipToPix(view.getContext().getResources(), height);
		parent.setLayoutParams(new LayoutParams((int)widthPx, (int)heightPx));
		parent.addView(view);
		$this.snapshot(parent);
	}

	private @Px float dipToPix(@NonNull Resources resources, @Dp float value) {
		return TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, value, resources.getDisplayMetrics());
	}

yields the right result, but it's always upscaled to 1000 height. This means that even a 16dp by 16dp icon will be saved as a 1000x1000 PNG file.

This upscaling makes the images look strange (blown up) when looking at the snapshots. And it's not possible to look at individual pixels (if necessary). Based on the code even different display sizes (4k, 1080p, etc) will all yield a 1000 tall image so we lose pixels to compare.

TWiStErRob avatar Nov 08 '21 22:11 TWiStErRob

@geoff-powell the minThumbnailSize is not the minimum, it's exactly that height. It'll up/downscale.

https://github.com/cashapp/paparazzi/compare/gpowell/sample-widget-render-extension?expand=1

TWiStErRob avatar Nov 22 '21 12:11 TWiStErRob

This is still valid in 1.0 albeit in a slightly different way. Consider this extension:

class ConstrainedSizeRenderExtension(
	@Dimension(unit = Dimension.DP)
	private val width: Float,

	@Dimension(unit = Dimension.DP)
	private val height: Float,
) : RenderExtension {

	override fun renderView(contentView: View): View {
		val widthPx = dipToPix(contentView.context.resources, width)
		val heightPx = dipToPix(contentView.context.resources, height)
		return FrameLayout(contentView.context).apply {
			layoutParams = LayoutParams(widthPx.toInt(), heightPx.toInt())
                        addView(contentView)
		}
	}

	companion object {

		@Px
		private fun dipToPix(
			resources: Resources,
			@Dimension(unit = Dimension.DP) value: Float,
		): Float =
			TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, value, resources.displayMetrics)
	}
}

Running a test with ConstrainedSizeRenderExtension(72, 86) results in this image: image

Note a few things:

  • the image is not 72x86dp large
  • the widget rendered inside the image is not 72x86dp large
  • the image is not cropped to the size of the root view returned by renderView
  • the image has a hard-coded height of 1000px
    (which might result in lost pixels when rendering on a large and high density RenderConfig)

TWiStErRob avatar Jun 03 '22 22:06 TWiStErRob

(I tried working around this by setting DeviceConfig, but it failed fast as I can't do it per-test.)

TWiStErRob avatar Jun 03 '22 22:06 TWiStErRob

@TWiStErRob I think it makes sense that this should be configured via DeviceConfig. RenderExtensions work on top of the layout lib generated output and shouldn't alter the device size layout lib renders. If you need to do this for a specific test case I would suggest using a separate test file with the desired DeviceConfig when instantiating the Paparazzi class.

geoff-powell avatar Jun 06 '22 13:06 geoff-powell

Point 4 would still make this practically unusable (the core of this report), as the that tiny cut-out (custom device screen) would be upscaled to 1000px wasting screen space, brain power and disk space.

I would suggest using a separate test file with the desired DeviceConfig

Parameterized tests don't work that way for years now. Every testing library that builds on JUnit 4 and even JUnit 5 itself recommends/enforces method-level parameterization. It's a good idea for a workaround though! (Create a test class that's abstract, and pass in the device size in the subclass constructor.)

TWiStErRob avatar Jun 06 '22 17:06 TWiStErRob

@TWiStErRob please confirm if this is still an issue now that #37 has been fixed by #497 and #655.

jrodbx avatar Dec 15 '22 07:12 jrodbx

Yeah, looks good https://github.com/cashapp/paparazzi/commit/6ac32b5df8d882138197c279326c8a74c2dd3936#diff-8b212f2f8baaecb5c5bd00ebfa2d19cc24cb7816910e3f40ad07820807efd34fR382 fixed it.

The hard-coded 1000 is still a weird thing, but that's a different issue.

TWiStErRob avatar Dec 15 '22 12:12 TWiStErRob

You can put a "Fixed by https://github.com/cashapp/paparazzi/pull/497" on this issue.

TWiStErRob avatar Dec 15 '22 12:12 TWiStErRob