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-24570] Android: Fix screen density breaking changes + multi-window in Android 7.0 #8970

Merged
merged 3 commits into from Jul 17, 2017

Conversation

frankieandone
Copy link
Contributor

@frankieandone frankieandone commented Apr 17, 2017

Test Case:

Add this to tiapp.xml in your Titanium app directory.

<android>
	<manifest xmlns:android="http://schemas.android.com/apk/res/android">
		<uses-sdk android:targetSdkVersion="24"/>
	</manifest>
</android>

In titanium_mobile/build directory, run: node scons.js build && node scons.js package && node scons.js install. In your Titanium app, run and cat build/android/AndroidManifest.xml. Watch out for density flag in configChange attribute. Verify new flags are present. Then put window in split screen mode.

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

Object.keys(finalAndroidManifest.application.activity).forEach(function (name) {
var activity = finalAndroidManifest.application.activity[name];
if (!activity.configChanges) {
activity.configChanges = ['density'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is density the only possible value in configChanges? Otherwise it would break other configs. And we should warn them to put it there if they don't do so far.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there other values that go into configChanges but it is a requirement for consistency across different Android 7 devices for screen magnification. For activity.configChanges = ['density']; I used the same logic used elsewhere just in case a situation arises where configChanges is empty but the fact is screenSize will always be added to configChanges so it is kind of redundant in a way.

Also, I should make users aware that density is added by default?

Copy link
Contributor

Choose a reason for hiding this comment

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

The "density" configChange should always be injected into the AndroidManifest.xml for every activity when targeting Android 7.0 or higher.

If we do not add this configChange setting, then a Titanium activity's UI will disappear if the end-user changes the "Display Size" under Android's main "Settings/Display" screen.

The reason this happens is because apps targeting Android 7.0 (or higher) are expected to handle "Display Size" changes dynamically. If we do not override the "density" configChange, then the activity's onCreate() method will be called, which will wipe the UI we were previously displaying (the same behavior you would see if you were missing the "screenOrientation" configChange and you rotated the screen). Apps targeting an older OS version (ie: API Level) are force-quit and restarted instead when the "Display Size" changes.

@jquick-axway
Copy link
Contributor

@fmerzadyan, can you double check if Titanium's JavaScript APIs correctly report the new "Display Size" set under Android "Settings/Display" please?
Ti.API.info("@@@ Screen Density: " + Ti.Platform.displayCaps.density); Ti.API.info("@@@ Screen DPI: " + Ti.Platform.displayCaps.dpi); Ti.API.info("@@@ Logical Density Factor: " + Ti.Platform.displayCaps.logicalDensityFactor);

Setting the "Display Size" to a larger setting will lower the DPI. For example, "xxxhdpi" to "mdpi" when setting it to the largest setting. We need to make sure that we're not caching the above properties since they can change dynamically now.

@garymathews garymathews changed the title timob-24570 address Android N: breaking screen density breaking changes [TIMOB-24570] Android: Fix screen density breaking changes in Android 7.0 Apr 20, 2017
@jquick-axway
Copy link
Contributor

@cb1kenobi, just to double check, is this the best way to inject this attribute setting to all activities in the AndroidManifest.xml file if the API Level is >= 24?
(The developer should not be able to remove it. It's required.)

@frankieandone frankieandone force-pushed the timob-24570 branch 3 times, most recently from 71cf57f to 63f946e Compare April 26, 2017 17:12
@frankieandone frankieandone changed the title [TIMOB-24570] Android: Fix screen density breaking changes in Android 7.0 [TIMOB-24570] Android: Fix screen density breaking changes + multi-window in Android 7.0 Apr 26, 2017
default :
return "medium";
return "unknown";
Copy link
Contributor

Choose a reason for hiding this comment

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

I gave you bad advise. It should not return "unknown". Developers use this name to load images of an appropriate size based on the "density" name property. So, this code needs to be changed to use >= checks. It should also not return "medium" for unknown densities either.

The code should be changed to something like this...

public String getDensity() {
	synchronized(dm) {
		getDisplay().getMetrics(dm);
		int dpi = dm.densityDpi;
		if (dpi >= 560) {        // DisplayMetrics.DENSITY_560
			return "xxxhigh";
		} else if (dpi >= 400) { // DisplayMetrics.DENSITY_400
			return "xxhigh";
		} else if (dpi >= 280) { // DisplayMetrics.DENSITY_280
			return "xhigh";
		} else if (dpi >= DisplayMetrics.DENSITY_HIGH) {
			return "high";
		} else if (dpi >= DisplayMetrics.DENSITY_TV) {
			return "tvdpi";
		} else if (dpi >= DisplayMetrics.DENSITY_MEDIUM) {
			return "medium";
		}
		return "low";
	}
}

if (this.realTargetSDK >= 13) {
if (this.realTargetSDK >= 24 && !finalAndroidManifest.application.hasOwnProperty('resizeableActivity')) {
finalAndroidManifest.application.resizeableActivity = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this from "_build.js". We don't want to force developers to use "resizeableActivity" in case they have layout issues when sized too small in a split-screen window... in which case they can opt-out by setting this attribute to false in the "tiapp.xml" file.

^ Never mind. Your code is checking for the existence of the property. It's doing it right.

@frankieandone frankieandone force-pushed the timob-24570 branch 2 times, most recently from c65e317 to 7266e2b Compare April 26, 2017 22:25
Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

Approved

@lokeshchdhry
Copy link
Contributor

@fmerzadyan , I am seeing an issue where app built with this fix has an invalid Androidmanifest.xml.

<?xml version="1.0" encoding="UTF-8"?>
<manifest xmlns:android="http://schemas.android.com/apk/res/android" package="com.app.timob24570" android:versionCode="1" android:versionName="1.0">
	<uses-sdk android:minSdkVersion="14" android:targetSdkVersion="24"/>
	<application android:icon="@drawable/appicon" android:label="TIMOB-24570" android:name="Timob24570Application" android:debuggable="false" android:theme="@style/Theme.AppCompat" android:resizeableActivity="true">
		<activity android:name=".Timob24570Activity" android:label="@string/app_name" android:theme="@style/Theme.Titanium" android:configChanges="keyboardHidden|orientation|fontScale|screenSize|smallestScreenSize|screenLayout|density"/>
		<service android:name="com.appcelerator.aps.APSAnalyticsService" android:exported="false"/>
	</application>
	<uses-permission android:name="android.permission.INTERNET"/>
	<uses-permission android:name="android.permission.ACCESS_WIFI_STATE"/>
	<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/>
	<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"/>
</manifest>

This is causing the app to get installed but not launch or show up in the apps list on the device.

Studio Ver: 4.9.0.201705302345
SDK Ver: 6.2.0 local build
OS Ver: 10.12.3
Xcode Ver: Xcode 8.3.3
Appc NPM: 4.2.9
Appc CLI: 6.2.2
Ti CLI Ver: 5.0.14
Alloy Ver: 1.9.11
Node Ver: 6.10.1
Java Ver: 1.8.0_101
Devices: ⇨ google Pixel --- Android 7.1.1
⇨ google Nexus 5 --- Android 6.0.1

@lokeshchdhry
Copy link
Contributor

lokeshchdhry commented Jul 17, 2017

FR Passed.

The app switches as expected to split screen to normal mode & vice versa. Also, changing the display size in the settings appropriately reflects the change in the app but, this change is only seen when the app is restarted & not when the app is resumed from paused state or from background.
Confirmed with @jquick-axway about this behavior as we do not have a event like e.g resize for the app to change its UI/display size on the fly.
Also, the split screen mode works as expected. The app can be put in split screen mode & its screen size can be increased or decreased.

Studio Ver: 4.9.1.201707130742
SDK Ver: 6.2.0.v20170713162837 from PR
OS Ver: 10.12.3
Xcode Ver: Xcode 8.3.3
Appc NPM: 4.2.9
Appc CLI: 6.2.2
Ti CLI Ver: 5.0.14
Alloy Ver: 1.9.11
Node Ver: 6.10.1
Java Ver: 1.8.0_101
Device: Pixel Android 7.0
Emulator: Android 7.0, android 7.1.1

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

4 participants