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

feat: add "scalingMode" property to Ti.UI.ImageView #12753

Merged
merged 8 commits into from
Aug 24, 2021

Conversation

jquick-axway
Copy link
Contributor

@jquick-axway jquick-axway commented Apr 28, 2021

JIRA:

Summary:

  • TIMOB-24313 Added new "scalingMode" property to Ti.UI.ImageView.
    • Allows you to crop or letterbox/pillarbox scale the image.
    • Correctly handles JPEG EXIF orientation/mirroring.
  • Added new constants:
    • Ti.Media.IMAGE_SCALING_AUTO // Legacy behavior
    • Ti.Media.IMAGE_SCALING_NONE // No scaling
    • Ti.Media.IMAGE_SCALING_FILL // Stretches disproportionally
    • Ti.Media.IMAGE_SCALING_ASPECT_FILL // Crops proportionally
    • Ti.Media.IMAGE_SCALING_ASPECT_FIT // Letterbox/Pillarbox scales proportionally
  • TIMOB-28432 On Android, changed "autorotate" property to default to true like iOS.
  • On Android, updated ImageView to correctly handle EXIF mirroring if "autorotate" property is true.

Image Scaling Test:

  1. Build and run ImageScalingChangeTest.js attached to TIMOB-24313 on Android.
  2. Tap on "Scale" button in top toolbar and select "Fill".
  3. Tap on "Resize" button in top toolbar and select "Ti.UI.FILL".
  4. Rotate portrait/landscape and verify image stretches to fill the window.
  5. Tap on "Resize" button in top toolbar and select "Ti.UI.SIZE".
  6. Rotate portrait/landscape and verify image appears letterboxed/pillarboxed. (Not stretched.)
  7. Tap on "Resize" button in top toolbar and select "240x240".
  8. Verify image is stretched within a square view. Appears same in portrait/landscape.
  9. Tap on "Scale" button in top toolbar and select "Aspect Fill". (Enables crop scaling.)
  10. Verify image is cropped within a square view and only top/bottom of image is clipped.
  11. Tap on "Resize" button in top toolbar and select "Ti.UI.FILL".
  12. Verify image is cropped. It's clipped on left/right in portrait and clipped top/bottom in landscape.
  13. Tap on "Scale" button in top toolbar and select "Aspect Fit". (Enables letterbox/pillarbox scaling.)
  14. Verify image is letterboxed in portrait and pillarboxed in landscape.
  15. Tap on "Resize" button in top toolbar and select "240x240".
  16. Verify image is scaled down and not clipped. (It's actually pillarboxed.)
  17. Tap on "Scale" button in top toolbar and select "None". (Disables scaling.)
  18. Verify image is clipped on all sides within the 240x240 sized view, because image is bigger than view.
  19. Repeat steps 2-18 on iOS.

Android EXIF Test:

  1. Build and run ImageExifTest.js attached to TIMOB-28432 on Android.
  2. Make sure the device has Internet access. (It will download images.)
  3. Tap on each row in list and make sure an image is displayed "upright" and is not "mirrored".
  4. Uncomment the autorotate: false line in the JS file. (This will ignored EXIF orientation.)
  5. Tap on each row and verify images are either rotated or mirrored. (This is good.)

@build
Copy link
Contributor

build commented Apr 28, 2021

Messages
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖

✅ All tests are passing
Nice one! All 14588 tests are passing.
(There are 938 skipped tests not included in that total)

📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS against e719131

@hansemannn
Copy link
Collaborator

This is huge. Basically makes the 3rd-party dependency to av.imageview obsolete (although the caching was better there a while ago - not sure if that changed in Titanium)

Copy link
Contributor

@garymathews garymathews left a comment

Choose a reason for hiding this comment

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

CR: PASS

TEST CASE
const window = Ti.UI.createWindow({
	layout: 'vertical',
	backgroundColor: 'white'
});
const picker = Ti.UI.createPicker({ top: 10 });
const image = Ti.UI.createImageView({
	top: 10,
	image: 'https://garymathews.com/droid.gif',
	width: 256,
	height: 144,
	borderWidth: 1,
	borderColor: 'gray',
	autorotate: true
});
const gallery_button = Ti.UI.createButton({ title: 'Select Photo' });

picker.add(Ti.UI.createPickerRow({ title: 'IMAGE_SCALING_AUTO' }));
picker.add(Ti.UI.createPickerRow({ title: 'IMAGE_SCALING_NONE' }));
picker.add(Ti.UI.createPickerRow({ title: 'IMAGE_SCALING_FILL' }));
picker.add(Ti.UI.createPickerRow({ title: 'IMAGE_SCALING_ASPECT_FILL' }));
picker.add(Ti.UI.createPickerRow({ title: 'IMAGE_SCALING_ASPECT_FIT' }));

picker.addEventListener('change', e => {
	switch (e.rowIndex) {
		case 0:
			image.scalingMode = Ti.Media.IMAGE_SCALING_AUTO;
			break;
		case 1:
			image.scalingMode = Ti.Media.IMAGE_SCALING_NONE;
			break;
		case 2:
			image.scalingMode = Ti.Media.IMAGE_SCALING_FILL;
			break;
		case 3:
			image.scalingMode = Ti.Media.IMAGE_SCALING_ASPECT_FILL;
			break;
		case 4:
			image.scalingMode = Ti.Media.IMAGE_SCALING_ASPECT_FIT;
			break;
	}
});

gallery_button.addEventListener('click', _ => {
	Ti.Media.openPhotoGallery({
		mediaTypes: Ti.Media.MEDIA_TYPE_PHOTO,
		success: e => image.image = e.media
	});
});

window.add([ picker, image, gallery_button ]);
window.open();

@jquick-axway
Copy link
Contributor Author

@hansemannn, @m1ga, @garymathews,
I'm thinking we should change the autorotate property to default to true instead of false. That way it will automatically show JPEGs upright and reverse the mirroring if defined in the EXIF. Odds are this is what everyone wants. What do you think?

@m1ga
Copy link
Contributor

m1ga commented Apr 29, 2021

In my opinion it should be true by default (if it works correctly 😉 )

@garymathews
Copy link
Contributor

@jquick-axway I think it should be true by default like it is on iOS

@jquick-axway jquick-axway removed the request for review from vijaysingh-axway May 1, 2021 00:38
@lokeshchdhry
Copy link
Contributor

@jquick-axway , I am seeing an issue on IOS, when Scale is None & Resize is 240x240 the image is not contained inside 240x240 but the whole window like below:
Simulator Screen Shot - iPhone 11 Pro - 2021-08-12 at 14 35 16

@lokeshchdhry
Copy link
Contributor

Fix for TIMOB-28432 looks good.

@hansemannn
Copy link
Collaborator

Would love to see this merged for 10.1.0! It will reduce the number of external required modules (in this case av.imageview for us)

@jquick-axway
Copy link
Contributor Author

@lokeshchdhry , what we're seeing is a clipping issue on iOS. By default an ImageView does not clip its content. That is, the view is correctly being sized at 240x240, but since it doesn't clip by default, it is allowed to show the image outside of the view's bounds. If we set the image view's clipMode property to Ti.UI.iOS.CLIP_MODE_ENABLED, then it does what we want.

So, it's not really a bug, but maybe we should change the default behavior to clip the image by default unless the clipMode property is set to disable it.

@jquick-axway
Copy link
Contributor Author

Updated PR to always clip the image within the ImageView, because it was always scaled within the view's bounds before.

@lokeshchdhry
Copy link
Contributor

FR Passed.

@hansemannn
Copy link
Collaborator

@ewanharris Build passing, go go go! 😄

@ewanharris ewanharris merged commit cce763a into tidev:master Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants