Automattic / stories-android

Loop concept app - WP Stories library
GNU General Public License v2.0
20 stars 6 forks source link

Pan / zoom background images: use FIT_CENTER scaleType #648

Closed mzorz closed 3 years ago

mzorz commented 3 years ago

Follow up to #647

It was noted that using CENTER_CROP made some images that already contained text to get cut (cropped) most of the time, especially when these images would have a different aspect ratio that that of the device's screen where Story frames were being created.

After a conversation held elsewhere, we came to the decision to enable pinch to zoom (see base PR), and also change to a FIT_CENTER scaleType. This way, any image will fit in width / height depending on the side of the image that is closer to the device's side in terms of length, maintaining its aspect ratio.

This is a summary of that conversation:

- On phones taller than 16:9, restrict the editable surface to 16:9 and black out the bottom section of the editor that would have been cropped out on export. (And don’t allow views to be added there/crop views that extend over the cutoff).
- On all phones, allow pinch to zoom, and when either height or width is now larger than the image bounds, show a blur which will be included in the exported image.
- Leave videos alone for now, we’ll iterate on those later

Note 1: this change only applies to pictures sourced from the media picker, but not pictures captured with the Stories library itself, given we use all the available screen. Note 2: this change only applies to Images (not videos) Note 3: the blur indicated in the definition above is not part of this PR ("...show a blur which will be included in the exported image...") Note 4: it was observed on a Samsung J2 that the exported image would have pixelated text when text was added. I don't think it's related to this PR, but leaving the note here since it was during development of this change that I saw it.

To test (use the demo app):

  1. pick a square image or for the case, any image that does not fit the device screen, or is smaller than the device screen
  2. observe it gets scaled with the FIT_CENTER scaleType
  3. repeat with a few other images - also verify the pinch to zoom works, and that when you switch between story frames the zoomed in portion is "remembered" each time.
  4. tap on the checkmark button to save
  5. verify the saved Story frames are all of the same size (exported as 9:16) with a black stripe on each side of the image where it didn't fit entirely on the screen.

Bonus step: when you tap on the "+" icon to add frames to the Story and the media picker shows up, tap BACK once to check that the zoomed in information is kept as well.

peril-automattic[bot] commented 3 years ago

You can test the changes on this Pull Request by downloading the APK here.

aforcier commented 3 years ago

Nice @mzorz !

I haven't dug into the code yet, but from a try with the demo app the center fit and pan/zoom behavior seem to work well! 🎉

I do have a few questions though:

On phones taller than 16:9, restrict the editable surface to 16:9 and black out the bottom section of the editor that would have been cropped out on export. (And don’t allow views to be added there/crop views that extend over the cutoff).

It doesn't look like this part has been implemented in this PR either? On a tall device (Pixel 5), an imported image in the story editor still extends all the way to the bottom of the screen, so that the part that will get cut off on export is not indicated:

tall-screen-export

this change only applies to pictures sourced from the media picker, but not pictures captured with the Stories library itself, given we use all the available screen.

The previous point should apply to pictures captured from the camera - we're using all the available screen but will still export down to 9:16. (That raises a good point that maybe we should cut out the bottom of the live preview for tall devices for accuracy as well. 🤔)

mzorz commented 3 years ago

It doesn't look like this part has been implemented in this PR either?

oops you're right! didn't make it here. Will make the changes and push shortly 👍

mzorz commented 3 years ago

It doesn't look like this part has been implemented in this PR either? On a tall device (Pixel 5), an imported image in the story editor still extends all the way to the bottom of the screen,

Fixed this issue in https://github.com/Automattic/stories-android/pull/649

Also re:

The previous point should apply to pictures captured from the camera - we're using all the available screen but will still export down to 9:16. (That raises a good point that maybe we should cut out the bottom of the live preview for tall devices for accuracy as well. 🤔)

That PR should cover for it too 👍 Ready for another look @aforcier

aforcier commented 3 years ago

:shipit: ! (Given that the comments are being addressed in later PRs.)