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(android): support touch feedback for all backgrounds or no background #11795

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@
import android.graphics.drawable.ColorDrawable;
import android.graphics.drawable.Drawable;
import android.graphics.drawable.RippleDrawable;
import android.graphics.drawable.ShapeDrawable;
import android.os.Build;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.core.view.ViewCompat;
import android.text.TextUtils;
import android.util.Pair;
Expand Down Expand Up @@ -815,8 +815,6 @@ public void propertyChanged(String key, Object oldValue, Object newValue, KrollP
boolean hasColorState = hasColorState(d);
boolean hasBorder = hasBorder(d);
boolean hasGradient = hasGradient(d);
boolean nativeViewNull = (nativeView == null);

boolean requiresCustomBackground = hasImage || hasColorState || hasBorder || hasGradient;

// PROPERTY_BACKGROUND_REPEAT is implicitly passed as false though not used in JS. So check the truth value and proceed.
Expand All @@ -832,16 +830,11 @@ public void propertyChanged(String key, Object oldValue, Object newValue, KrollP
background = null;
}

if (d.containsKeyAndNotNull(TiC.PROPERTY_BACKGROUND_COLOR)) {
Integer bgColor = TiConvert.toColor(d, TiC.PROPERTY_BACKGROUND_COLOR);
if (!nativeViewNull) {
if (canApplyTouchFeedback(d)) {
applyTouchFeedback(bgColor, d.containsKey(TiC.PROPERTY_TOUCH_FEEDBACK_COLOR)
? TiConvert.toColor(d, TiC.PROPERTY_TOUCH_FEEDBACK_COLOR)
: null);
} else {
nativeView.setBackgroundColor(bgColor);
}
if (this.nativeView != null) {
if (d.containsKeyAndNotNull(TiC.PROPERTY_BACKGROUND_COLOR)) {
this.nativeView.setBackgroundColor(TiConvert.toColor(d, TiC.PROPERTY_BACKGROUND_COLOR));
} else {
this.nativeView.setBackground(null);
}
}
} else {
Expand Down Expand Up @@ -898,11 +891,15 @@ public void propertyChanged(String key, Object oldValue, Object newValue, KrollP

applyCustomBackground();
}
if (canApplyTouchFeedback(d)) {
String colorString = TiConvert.toString(d.get(TiC.PROPERTY_TOUCH_FEEDBACK_COLOR));
applyTouchFeedback((colorString != null) ? TiConvert.toColor(colorString) : null);
}
if (key.equals(TiC.PROPERTY_OPACITY)) {
setOpacity(TiConvert.toFloat(newValue, 1f));
}
if (!nativeViewNull) {
nativeView.postInvalidate();
if (this.nativeView != null) {
this.nativeView.postInvalidate();
}
} else if (key.equals(TiC.PROPERTY_SOFT_KEYBOARD_ON_FOCUS)) {
Log.w(TAG,
Expand Down Expand Up @@ -1010,26 +1007,22 @@ public void processProperties(KrollDict d)
handleBackgroundImage(d);

} else if (d.containsKey(TiC.PROPERTY_BACKGROUND_COLOR) && !nativeViewNull) {
// Set the background color on the view directly only if there is no border.
// If border is present, then we must use the TiBackgroundDrawable.
bgColor = TiConvert.toColor(d, TiC.PROPERTY_BACKGROUND_COLOR);

if (canApplyTouchFeedback(d)) {
applyTouchFeedback(bgColor, d.containsKey(TiC.PROPERTY_TOUCH_FEEDBACK_COLOR)
? TiConvert.toColor(d, TiC.PROPERTY_TOUCH_FEEDBACK_COLOR)
: null);
} else {
// Set the background color on the view directly only
// if there is no border. If a border is present we must
// use the TiBackgroundDrawable.
if (hasBorder(d)) {
if (background == null) {
applyCustomBackground(false);
}
background.setBackgroundColor(bgColor);
} else {
nativeView.setBackgroundColor(bgColor);
if (hasBorder(d)) {
if (background == null) {
applyCustomBackground(false);
}
background.setBackgroundColor(bgColor);
} else {
nativeView.setBackgroundColor(bgColor);
}
}
if (canApplyTouchFeedback(d)) {
String colorString = TiConvert.toString(d.get(TiC.PROPERTY_TOUCH_FEEDBACK_COLOR));
applyTouchFeedback((colorString != null) ? TiConvert.toColor(colorString) : null);
}

if (d.containsKey(TiC.PROPERTY_HIDDEN_BEHAVIOR) && !nativeViewNull) {
Object hidden = d.get(TiC.PROPERTY_HIDDEN_BEHAVIOR);
Expand Down Expand Up @@ -1190,23 +1183,51 @@ protected boolean canApplyTouchFeedback(@NonNull KrollDict props)

/**
* Applies touch feedback. Should check canApplyTouchFeedback() before calling this.
* @param backgroundColor The background color of the view.
* @param rippleColor The ripple color.
* @param rippleColor The ripple color to use. Set to null to use system's default ripple color.
*/
private void applyTouchFeedback(@NonNull Integer backgroundColor, @Nullable Integer rippleColor)
private void applyTouchFeedback(Integer rippleColor)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason to drop the @Nullable? I kind of like having it be explicit in our APIs so we can use tools to verify checking for null pointers or not...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I question the value of the @Nullable annotation. All reference types are implicitly nullable. The only benefit it provides is more for IDE intellisense purposes since it reveals the annotations and effectively documents the API which we can already do via JavaDoc comments.

The @NonNull annotation is far more valuable since we can use it to trigger compiler/linter errors. So, if it doesn't have the @NonNull annotation, then it's assumed nullable.

Anyways, that's my 2 cents. Unless you can think of something else that might benefit from this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I get that things are implicitly @Nullable (so long as they're not primitives). I saw it more as a marker of what APIs we've "done" since we've got this large legacy codebase that we didn't use the annotations on. If we don't use @Nullable, it's unclear if we've already reviewed the API and decided they're all implicitly nullable, or if we simply haven't gotten around to adding annotations.

{
// Do not continue if there is no view to modify.
if (this.nativeView == null) {
return;
}

// Fetch default ripple color if given null.
if (rippleColor == null) {
Context context = proxy.getActivity();
TypedValue attribute = new TypedValue();
if (context.getTheme().resolveAttribute(android.R.attr.colorControlHighlight, attribute, true)) {
rippleColor = attribute.data;
} else {
throw new RuntimeException("android.R.attr.colorControlHighlight cannot be resolved into Drawable");
}
if (rippleColor == null) {
Log.e(TAG, "android.R.attr.colorControlHighlight cannot be resolved into Drawable");
return;
}
}
RippleDrawable rippleDrawable =
new RippleDrawable(ColorStateList.valueOf(rippleColor), new ColorDrawable(backgroundColor), null);
nativeView.setBackground(rippleDrawable);

// Fetch the background drawable that we'll be applying the ripple effect to.
Drawable backgroundDrawable = this.background;
if (backgroundDrawable == null) {
backgroundDrawable = this.nativeView.getBackground();
}

// Create a mask if a background doesn't exist or if it's completely transparent.
// Note: Ripple effect won't work unless it has something opaque to draw to. Use mask as a fallback.
ShapeDrawable maskDrawable = null;
boolean isVisible = (backgroundDrawable != null);
if (backgroundDrawable instanceof ColorDrawable) {
int colorValue = ((ColorDrawable) backgroundDrawable).getColor();
if (Color.alpha(colorValue) <= 0) {
isVisible = false;
}
}
if (!isVisible) {
maskDrawable = new ShapeDrawable();
}

// Replace view's existing background with ripple effect wrapping the old drawable.
nativeView.setBackground(
new RippleDrawable(ColorStateList.valueOf(rippleColor), backgroundDrawable, maskDrawable));
}

@Override
Expand Down
7 changes: 5 additions & 2 deletions apidoc/Titanium/UI/View.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1766,8 +1766,11 @@ properties:
- name: touchFeedback
summary: A material design visual construct that provides an instantaneous visual confirmation of touch point.
description: |
This is an opt-in feature available from Android Lollipop.
Touch feedback is applied only if the backgroundColor is a solid color.
Touch feedback is only applied to a view's background. It is never applied to the view's foreground content
such as a <Titanium.UI.ImageView>'s image.

For Titanium versions older than 9.1.0, touch feedback only works if you set the
<Titanium.UI.View.backgroundColor> property to a non-transparent color.
type: Boolean
default: false
platforms: [android]
Expand Down
114 changes: 114 additions & 0 deletions tests/Resources/ti.ui.view.addontest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
/*
* Appcelerator Titanium Mobile
* Copyright (c) 2015-Present by Axway, Inc. All Rights Reserved.
* Licensed under the terms of the Apache Public License
* Please see the LICENSE included with this distribution for details.
*/
/* eslint-env mocha */
/* eslint no-unused-expressions: "off" */
'use strict';

describe('Titanium.UI.View', function () {
let win;

this.timeout(5000);

afterEach((done) => {
if (win) {
let t = setTimeout(function () {
if (win) {
win = null;
done();
}
}, 3000);
win.addEventListener('close', function listener () {
clearTimeout(t);
if (win) {
win.removeEventListener('close', listener);
}
win = null;
done();
});
win.close();
} else {
win = null;
done();
}
});

it.android('touchFeedback', (finish) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the effort in creating a test, but I don't think it's really verifying anything since this is an interactive UI change, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. It doesn't verify anything, but it does exercise the native API to make sure it doesn't crash. So, I guess it's better than nothing? 🤷‍♂️

Ideally, QE should test this by hand via a functional/smoke test since this is a visual feature.

win = Ti.UI.createWindow({ layout: 'horizontal' });
win.add(Ti.UI.createLabel({
text: 'View 1',
touchFeedback: true,
touchFeedbackColor: 'yellow'
}));
win.add(Ti.UI.createLabel({
text: 'View 2',
backgroundColor: 'gray',
touchFeedback: true,
touchFeedbackColor: 'yellow'
}));
win.add(Ti.UI.createLabel({
text: 'View 3',
backgroundImage: '/Logo.png',
touchFeedback: true,
touchFeedbackColor: 'yellow'
}));
win.add(Ti.UI.createLabel({
text: 'View 4',
backgroundGradient: {
type: 'linear',
startPoint: { x: '0%', y: '50%' },
endPoint: { x: '100%', y: '50%' },
colors: [ { color: 'red', offset: 0.0 }, { color: 'blue', offset: 1.0 } ]
},
touchFeedback: true,
touchFeedbackColor: 'yellow'
}));
win.add(Ti.UI.createLabel({
text: 'View 5',
borderRadius: 20,
borderColor: 'red',
borderWidth: '8dp',
touchFeedback: true,
touchFeedbackColor: 'yellow'
}));
win.add(Ti.UI.createLabel({
text: 'View 6',
backgroundColor: 'gray',
borderRadius: 20,
borderColor: 'red',
borderWidth: '8dp',
touchFeedback: true,
touchFeedbackColor: 'yellow'
}));
win.add(Ti.UI.createLabel({
text: 'View 7',
backgroundImage: '/Logo.png',
borderRadius: 20,
borderColor: 'red',
borderWidth: '8dp',
touchFeedback: true,
touchFeedbackColor: 'yellow'
}));
win.add(Ti.UI.createLabel({
text: 'View 8',
backgroundGradient: {
type: 'linear',
startPoint: { x: '0%', y: '50%' },
endPoint: { x: '100%', y: '50%' },
colors: [ { color: 'red', offset: 0.0 }, { color: 'blue', offset: 1.0 } ]
},
borderRadius: 20,
borderColor: 'red',
borderWidth: '8dp',
touchFeedback: true,
touchFeedbackColor: 'yellow'
}));
win.addEventListener('open', () => {
finish();
});
win.open();
});
});