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

[TIMOB-17363] Android/iOS: Added "MaskedImage" support to Android. Moved BLEND_MODE constants to "Ti.UI". #10013

Merged
merged 10 commits into from May 17, 2018

Conversation

jquick-axway
Copy link
Contributor

@jquick-axway jquick-axway commented Apr 26, 2018

JIRA:
https://jira.appcelerator.org/browse/TIMOB-17363

Summary:

  • Moved BLEND_MODE_* constants from Ti.UI.iOS to Ti.UI.
    • Constants are still defined under Ti.UI.iOS for backward compatibility on iOS, but are documented as deprecated.
  • Added Ti.UI.MaskedImage support to Android.
    • Intended as alternate solution to using rounded borders around images, which has hardware acceleration issues on Android.
  • Supports 9-patch images, but only if loaded from APK's "res" directory.
  • Supports base View class' tintColor and opacity properties. Applied to final result.
  • Super optimized masking via GPU shaders on Android for:
    • mask and tint for all blend modes.
    • mask and image for modes SOURCE_IN and DESTINATION_IN.
  • All other masking combinations not in above list resort to less optimized rendering on Android.
  • Android does not support the following blend modes (will log a warning):
    • BLEND_MODE_COLOR
    • BLEND_MODE_COLOR_BURN
    • BLEND_MODE_COLOR_DODGE
    • BLEND_MODE_DIFFERENCE
    • BLEND_MODE_EXCLUSION
    • BLEND_MODE_HARD_LIGHT
    • BLEND_MODE_HUE
    • BLEND_MODE_LUMINOSITY
    • BLEND_MODE_PLUS_DARKER
    • BLEND_MODE_SATURATION
    • BLEND_MODE_SOFT_LIGHT
  • Known issues (to be written up later):
    • Will not auto-rotate JPEGs based on EXIF orientation.
    • toImage() renders bad image on Android 7.0 and higher when masking an image.

Test:
Run the below test procedure on...

  • Android 4.1
  • Android 4.4
  • Android 7.0 or higher

Test procedure...

  1. Create a classic app and add the contents of "MaskedImageTest.zip" to it from TIMOB-17363.
  2. On app startup, verify appc logo is masked within a circle like Screenshot1
  3. Tap the "Size" button and select "Ti.UI.FILL".
  4. Verify image stretches to fit container like Screenshot2.
  5. Rotate device to landscape.
  6. Verify image stretches to fit landscape container like Screenshot3.
  7. Rotate device back to portrait.
  8. Tap the "Mode" button and select "SOURCE_OUT".
  9. Verify image is only displayed outside of circle like Screenshot4.
  10. Tap the "Mask" button and select "MaskCircleLock".
  11. Tap the "Image" button and select "None".
  12. Tap the "Tint" button and select "Red".
  13. Verify lock is red, within a white circle, and outside of circle is red like Screenshot5.
  14. Tap the "Mode" button and select "SOURCE_IN".
  15. Verify lock is white with a red circle like Screenshot6.
  16. Tap the "Size" button and select "300dp".
  17. Verify circle image is no longer stretched like Screenshot7.
  18. Tap the "Image" button and select "MaskVerticalGradient".
  19. Verify circle has a gradient (light red on top; full red on bottom) like Screenshot8.

Copy link
Collaborator

@hansemannn hansemannn left a comment

Choose a reason for hiding this comment

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

Left some review changes, mainly about docs and iOS.

type: Number
permission: read-only
platforms: [iphone, ipad]
since: "7.2.0"
platforms: [android, iphone, ipad]
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it is not supported on Android, I would just leave out the android key. If we would be very exact here, we would leave the iOS only constants on the iOS namespace and put the parity ones here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still define the constant on Android, but log a warning instead. I think it's better than letting the app crash.

It is possible to add the other modes on Android, but we would have to manipulate the bitmap pixels ourselves the slow way. (yuck)

I'll remove the "android" platform keys from the unsupported constants.

@@ -164,6 +164,35 @@ - (TiUIActivityIndicatorStyleProxy *)ActivityIndicatorStyle
MAKE_SYSTEM_PROP(PICKER_TYPE_TIME, UIDatePickerModeTime);
MAKE_SYSTEM_PROP(PICKER_TYPE_COUNT_DOWN_TIMER, UIDatePickerModeCountDownTimer);

MAKE_SYSTEM_PROP(BLEND_MODE_NORMAL, kCGBlendModeNormal);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please deprecate the old ones. We have a macro for this:

// Definition
MAKE_SYSTEM_PROP_DEPRECATED_REPLACED(name, map, api, in, newapi);

// Example
MAKE_SYSTEM_PROP_DEPRECATED_REPLACED(BLEND_MODE_NORMAL, kCGBlendModeNormal, @"UI.iOS.BLEND_MODE_NORMAL", @"7.3.0", @"UI.BLEND_MODE_NORMAL");

Note: The Ti / Titanium is not included into the log as it is replaced by the app name during project generation. For the log, it's manually constructed to avoid being replaced, so we leave it out in every log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

image: '/SplashScreen.png',
mode: Ti.UI.BLEND_MODE_SOURCE_IN,
width: Ti.UI.FILL,
height: Ti.UI.FILL,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should fill by default. How does this behave on iOS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Android currently auto-sizes by default. I'll double check the iOS handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just re-tested this on iOS. You're right. An iOS MaskedImage does a Ti.UI.FILL by default. I'll change Android to make it match.

Also, our iOS MaskedImage doesn't appear to support Ti.UI.SIZE. I get a 0x0 size view when attempting to auto-size it. On Android, I made this work. I auto-size based on the image if available, otherwise I fallback to auto-sizing based on the mask size. (I'll keep this behavior.)

@jquick-axway
Copy link
Contributor Author

@hansemannn, do you know if we need to do anything on the Alloy side after moving the BLEND_MODE constants?

// Android 4.4 has a bug where if bitmap byte width does not fit a 4 byte packing alignment,
// then mask will appear skewed. Resizing bitmap doesn't work. So, give up if this happens.
if ((Build.VERSION.SDK_INT == 19) || (Build.VERSION.SDK_INT == 20)) {
if ((bitmap.getWidth() % 4) != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be moved above the bitmap.extractAlpha() to fail before reaching bitmap.extractAlpha() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, good catch. I was trying to "fix" the resulting grayscaled bitmap, but later gave up.

{
if (this.maskedDrawable != null) {
if (value != null) {
int color = TiConvert.toColor(TiConvert.toString(value));
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe validate if value is a string?

{
if (this.maskedDrawable != null) {
if (value != null) {
int color = TiConvert.toColor(TiConvert.toString(value));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

- Ti.UI.iOS BLEND_MODE constants will now log a warning if used.
- Modified Android's MaskedImage to be Ti.UI.FILL by default like iOS. (Was Ti.UI.SIZE)
- Now logs a warning if a non-string type was given to MaskedImage color properties.
- Modified Android to support mask, image, and tint all at the same time like iOS.
  * Used to not support both tint and image at the same time.
- Fixed dynamic changes of "mask" and "tint" properties on Android. (Would not always update onscreen.)
- Updated docs to indicate which BLEND_MODE constants are not supported by the Android platform.
@jquick-axway
Copy link
Contributor Author

Updated PR:

  • Ti.UI.iOS BLEND_MODE constants will now log a warning if used.
  • Modified Android's MaskedImage to be Ti.UI.FILL by default like iOS. (Was Ti.UI.SIZE.)
  • Now logs a warning if a non-string type was given to tint and tintColor properties.
  • Modified Android to support mask, image, and tint all at the same time like iOS.
    • Used to not support both tint and image at the same time.
  • Fixed dynamic changes of mask, tint, and mode properties on Android. (Would not always update onscreen.)
  • Updated docs to indicate which BLEND_MODE constants are not supported by the Android platform.

@build
Copy link
Contributor

build commented May 17, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@hansemannn hansemannn merged commit a69b61a into tidev:master May 17, 2018
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.

None yet

5 participants