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

fix(android): fix views with border and transparency #11224

Merged
merged 2 commits into from Sep 25, 2019

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Sep 17, 2019

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

Description:
Rework handling the disabling of the HW acceleration for views with transparency and borders. isHardwareAccelerated() returns whether the view is attached to a Window with enabled HW acceleration:
https://android.googlesource.com/platform/frameworks/base.git/+/refs/heads/master/core/java/android/view/View.java#20915
So it depends on the view being attached to a Window. Our call of disableHWAcceleration() in TiUIView.java happens before the native view is actually attached to a window, so it would require an additional check in order to determine when to disable acceleration for the TiBorderWrapperView. Currently borderView.isHardwareAccelerated() will always return false, resulting in skipping our method.
We had a change in 6.3.0.GA for this code in this PR:
#9410
Ran the test cases from it and it works fine both on Android 19 and Android 29.
This PR also fixes the case described in this JIRA ticket - https://jira.appcelerator.org/browse/TIMOB-26291
During FR I would recommend trying the test cases from all three PRs to avoid regressions.
Note: No unit test, since this is a visual fix.

Test case:
app.js

var win = Ti.UI.createWindow({
     backgroundColor: '#fff'
});
 
var v1 = Titanium.UI.createView({
     backgroundColor: 'rgba(0,0,0,0.5)',
     height: 100,
     width: Ti.UI.FILL,
     borderRadius: 20
});
win.add(v1);
win.open();

Properly handle disabling the HW acceleration for views with transparency and borders
@ypbnv ypbnv added this to the 8.3.0 milestone Sep 17, 2019
@build build requested a review from a team September 17, 2019 12:11
@build
Copy link
Contributor

build commented Sep 17, 2019

Messages
📖

💾 Here's the generated SDK zipfile.

📖

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

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

Generated by 🚫 dangerJS against 30ff261

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

@ssjsamir ssjsamir self-requested a review September 24, 2019 16:08
Copy link
Contributor

@ssjsamir ssjsamir left a comment

Choose a reason for hiding this comment

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

FR Passed: Views with borderRadius now show up semi-transparent. Tested with the test case in the description and additional tests from the mentioned PRs above were done.

Test Environment

MacOS Catalina 10.15 Beta
Node.js ^8.11.1
Android Pixel XL 7.1 Emulator 
"NPM":"4.2.15","CLI":"7.1.1"
Java Version 1.8.0_131
Android NDK:  12.1.2977051

@ssjsamir ssjsamir merged commit 95fed44 into tidev:master Sep 25, 2019
@hansemannn
Copy link
Collaborator

Either this one or #10637 is causing a regression with Ti.Map which now displays an empty map, after updating to latest master. You can try with our internal project, I think both @garymathews and @jquick-axway have access to it.

@jquick-axway
Copy link
Contributor

@hansemannn, we'll check it out. Thanks.
Question: Do you have a border applied to your map?

@jquick-axway
Copy link
Contributor

jquick-axway commented Oct 3, 2019

Okay, I've confirmed that we have a regression on "master". If you apply a border to a VideoPlayer or MapView (something that requires hardware acceleration), then it won't render. I can understand this happening if "borderRadius" was involved (that's buggy on Google's end), but this is happening to rectangular borders now.

var window = Ti.UI.createWindow({ fullscreen: true });
var videoView = Ti.Media.createVideoPlayer({
	url: "http://assets.appcelerator.com.s3.amazonaws.com/video/media.m4v",
	autoplay: true,
	borderWidth: "4dp",
	borderColor: "red",
	width: Ti.UI.FILL,
	height: Ti.UI.FILL,
});
window.add(videoView);
window.open();

I've written up a private ticket for it here: TIMOB-27444

@jquick-axway
Copy link
Contributor

jquick-axway commented Oct 4, 2019

The fix for the regression can be found here:
#11262

It turns out the old code (before this PR) was wrongly "trying" to disabling hardware acceleration when it didn't need to, but failing due to what this PR was fixing. So, this PR merely brought the pre-existing bug to the surface.

@hansemannn
Copy link
Collaborator

Thanks Josh, that was fast!

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

6 participants