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: cross-platform light/dark mode API #11457

Merged
merged 37 commits into from May 8, 2020

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Jan 28, 2020

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

Description:
This incorporates the code from @drauggres in his PR #11301

This attempts to make a more comprehensive cross-platform API for dealing with determining the current theme/mode, and consistency in the return value of Ti.UI.fetchSemanticColor.

API

  • Ti.UI.userInterfaceStyle reports the current theme via Ti.UI constants: USER_INTERFACE_STYLE_DARK, USER_INTERFACE_STYLE_LIGHT, and USER_INTERFACE_STYLE_UNSPECIFIED
  • Ti.UI.fetchSemanticColor(String name) returns a Ti.UI.Color proxy for the named color (and you can get the underlying "color" by calling toHex() on it to get a hex string) on iOS 13+, a string (representing the hex value) on Android and iOS < 13. The mismatched return types are necessary to make it easy to just pass the returned value to the UI properties without conversion right now.
  • Ti.UI now has a new userInterfaceStyle event which gets fired when the device theme changes. The object received in the event listener has a value property whose value is the constant for the new USER_INTERFACE_STYLE_* that was set.

Deprecations

  • Ti.App.iOS.userInterfaceStyle -> use Ti.UI.userInterfaceStyle
  • Ti.App.iOS.USER_INTERFACE_STYLE_DARK -> use Ti.UI.USER_INTERFACE_STYLE_DARK
  • Ti.App.iOS.USER_INTERFACE_STYLE_LIGHT -> use Ti.UI.USER_INTERFACE_STYLE_LIGHT
  • Ti.App.iOS.USER_INTERFACE_STYLE_UNSPECIFIED -> use Ti.UI.USER_INTERFACE_STYLE_UNSPECIFIED
  • Undocumented Ti.UI.iOS.fetchSemanticColor() (use Ti.UI.fetchSemanticColor())

Doc updates

  • Document the Ti.UI.Color proxy. This is really an iOS-only proxy. We need a way to pass around the underlying UIColor objects from iOS so that we can set various UI color properties with them and they get "automatically" updated if the user changes the device theme.

Notable parity issues
While passing around Ti.UI.Color proxies on iOS allows for the UI to automatically adjust to theme changes, we don't have the equivalent on Android. Can we hook an equivalent Ti.UI.Color proxy that could auto-update on Android?

To Test
The actual API has some unit tests to verify the new properties/constants exist.

It's difficult to test on the emulator/sim, but to test the new event on theme changes you can run this app:

Resources/semantic.colors.json

{
	"primaryBackground": {
		"light": "#efece3",
		"dark": "#001900"
	}
}

Resources/app.js

// Also try baked-in colors on iOS like 'systemRedColor' - they will change as light/dark mode changes - although very subtly (I had to use the Digital Color Meter to double-check)
const primaryBackground = Ti.UI.fetchSemanticColor('primaryBackground');
const win = Ti.UI.createWindow({
    backgroundColor: primaryBackground
});

const btn = Ti.UI.createButton({
    title: 'Trigger'
});

btn.addEventListener('click', function() {
    Ti.API.info('Hello world!');
});

win.add(btn);
win.open();

Ti.UI.addEventListener('userinterfacestyle', e => {
    console.log(e);
    console.log(primaryBackground.toHex());
});

On iOS Emulator, I had to use the Hardware > Siri menu and then tell Siri to "Tun Dark mode on" (or off). On Android, You need an apilevel 29+ emulator, and you can drag from he top status bar down to get a quick menu. Click the pencil icon to edit the options displayed, drag the "Dark Theme" button up into it, and then you can use that to toggle dark/light themes while the app is running.

@build
Copy link
Contributor

build commented Jan 28, 2020

Messages
📖

💾 Here's the generated SDK zipfile.

📖 ✊ 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 6649 tests are passing.
(There are 709 skipped tests not included in that total)

Generated by 🚫 dangerJS against 5f9711c

const _t = this;
const xmlFileName = 'semantic.colors.xml';
const valuesDirPath = path.join(this.buildAppMainResDir, 'values');
const valuesNightDirPath = path.join(this.buildAppMainResDir, 'values-night');
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't create a "values.xml" file. We risk file collision with the app developer's own "res" files. They need to be prefixed with "ti_" or "ti-". (C-style convention.)

So, we should name the files:

  • "ti_semantic_values.xml" and "ti_semantic_values_night.xml".
  • Or "ti-semantic-values.xml" and "ti-semantic-values-night.xml".

Note: Drawable "res" files can't have dash '-' chars (they're illegal), but XML "res" files can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jquick-axway These aren't creating a values.xml file, they're creating:

  • build/app/src/main/res/values/semantic.colors.xml
  • build/app/src/main/res/values-night/semantic.colors.xml

Is the suggestion we should instead name them:

  • build/app/src/main/res/ti_semantic_colors.xml
  • build/app/src/main/res/ti_semantic_colors_night.xml
    ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, those are folders. Sorry about that.

But we should still prefix the XML files with "ti".
How about "ti.semantic.colors.xml"?

@jquick-axway
Copy link
Contributor

jquick-axway commented Jan 28, 2020

Do we add an event to Ti.UI to fire when theme changes and let developers decide if they want to try and handle it?

On Android, we know when the system changes between dark/light theme on Android Q (and day/night mode on older OS versions) when Java method onConfigurationChanged() gets invoked with "uiMode" set.

Can we hook an equivalent Ti.UI.Color proxy that could auto-update on Android?

Theoretically yes. Currently, colors are applied to views via the Java ColorDrawable class which is assigned a hard-coded color value. We could create our own custom TiColorDrawable class which can dynamically change colors internally. This would involve us setting up a global listener for when "uiMode" changes, and when it does, our custom TiColorDrawable would have to call its invalidateSelf() method, which signals the view to re-render itself with the updated drawable.


The only thing that bugs me here is that we're conflating dark/light theme with day/night mode.


Also, AndroidX offers a Dark/Light AppCompat theme. Perhaps we should evaluate it before the 9.0.0 release... since changing the theme could be a breaking-change (particularly for those who customize an existing theme).

@sgtcoolguy
Copy link
Contributor Author

@jquick-axway Is there really a difference between Day/Night and Light/Dark modes? It seems to me to be a re-branding from Android: https://developer.android.com/guide/topics/ui/look-and-feel/darktheme
I mean in effect, they use both Light/Dark theme and Day/Night more or less interchangeably. The docs there even show that NIGHT_MODE_NO == Light and NIGHT_MODE_YES == Dark.

@jquick-axway
Copy link
Contributor

Right, I've seen Google's docs on this. I don't know how they're differentiating the 2 concepts yet. I haven't had time to play with it.

On Android, if you drag the top status bar down and tap on the "Night Light" button (ie: the "Moon" button), that will flip the Day/Night mode. This triggers the "uiMode" config-change. This is not intended to trigger Dark mode (but I suppose you can set up an app to do this). Changing Dark/Light theme under Display settings is supposed to trigger "uiMode" as well. So, we need to figure out how to differentiate between Day/Night and Dark/Light changes.

@jquick-axway
Copy link
Contributor

jquick-axway commented Jan 29, 2020

I think you have to use the MODE_NIGHT_FOLLOW_SYSTEM constant on Android Q (and above) to have the app honor the system's light/dark theme (not day/night setting). We would also have to update Titanium's app theme to "Theme.AppCompat.DayNight".

I've found more/better information below, which includes a sample app. When using a theme/style with android:isLightTheme="true" appears to support dynamically changing light/dark theme. I still have no idea how to differentiate between in day/night and dark/light theme changes in onConfigurationChanged() yet... and perhaps it isn't possible? (sigh)
https://github.com/android/user-interface-samples/tree/master/DarkTheme

@sgtcoolguy sgtcoolguy marked this pull request as ready for review March 5, 2020 15:58
Copy link
Contributor

@janvennemann janvennemann left a comment

Choose a reason for hiding this comment

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

Just a minor note on the docs. The iOS changes are looking good, nothing to add there.

apidoc/Titanium/UI/UI.yml Outdated Show resolved Hide resolved
const _t = this;
const xmlFileName = 'semantic.colors.xml';
const valuesDirPath = path.join(this.buildAppMainResDir, 'values');
const valuesNightDirPath = path.join(this.buildAppMainResDir, 'values-night');
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, those are folders. Sorry about that.

But we should still prefix the XML files with "ti".
How about "ti.semantic.colors.xml"?

if (Ti.UI.userInterfaceStyle === Ti.UI.USER_INTERFACE_STYLE_DARK) {
return UI.SEMANTIC_COLOR_TYPE_DARK;
}
return UI.SEMANTIC_COLOR_TYPE_LIGHT;
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that "night" mode is supported on API Levels older than 29.

Simplest way to test it is by toggling "Battery Saver" mode...

  1. Unplug the Android device from USB. (It needs to run on battery power.)
  2. Open the Calculator app.
  3. Drag the top status bar down.
  4. Tap on the "Battery Saver" button.

I tested your PR on Android 9 (API Level 28) and it works. The "userinterfacestyle" event was received and I received the correct color.
So, you don't need to add any guards for Android. :)

try {
return colorset[colorName][uiModule.semanticColorType].color || colorset[colorName][uiModule.semanticColorType];
const entry = colorset[colorName][UI.semanticColorType];
const hex = entry.color || entry;
Copy link
Contributor

Choose a reason for hiding this comment

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

Our docs under the "Dark Mode" section says that we support an "alpha" property too.
https://docs.appcelerator.com/platform/latest/#!/api/Titanium.UI

It was missing in the original JS code here before. But I can see "alpha" support is in our Android "_build.js" and in our iOS "TiUtils.m" source code.
@ewanharris, am I right about this?

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 believe @drauggres has an open PR for this at #11315

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes (halfbaked as most of it was), I believe that we handle the alpha property when generating the iOS semantic colors file, but not in the lookup in the JS code

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. I scheduled TIMOB-27519 for 9.1.0. Thanks

@jquick-axway
Copy link
Contributor

jquick-axway commented Mar 28, 2020

I tested this PR with the below code on Android 10, 9, and 4.4 using real devices. This will automatically change the background color when flipping between Light/Dark mode. It also tests the handling of multiple windows.

function createThemedWindow(settings) {
	if (!settings) {
		settings = {};
	}
	settings.backgroundColor = Ti.UI.fetchSemanticColor("primaryBackground");
	var window = Ti.UI.createWindow(settings);
	function onUserInterfaceStyleChanged(e) {
		Ti.API.info("@@@ userinterfacestyle event.value: " + e.value);
		window.backgroundColor = Ti.UI.fetchSemanticColor("primaryBackground");
	}
	window.addEventListener("open", function() {
		Ti.UI.addEventListener("userinterfacestyle", onUserInterfaceStyleChanged);
	});
	window.addEventListener("close", function() {
		Ti.UI.removeEventListener("userinterfacestyle", onUserInterfaceStyleChanged);
	});
	return window;
}

var parentWindow = createThemedWindow({ title: "Parent Window" });
var openButton = Ti.UI.createButton({ title: "Show Child" });
openButton.addEventListener("click", function() {
	var childWindow = createThemedWindow({ title: "Child Window" });
	var closeButton = Ti.UI.createButton({ title: "Close" });
	closeButton.addEventListener("click", function() {
		childWindow.close();
	});
	childWindow.add(closeButton);
	childWindow.open();
});
parentWindow.add(openButton);
parentWindow.open();

@sgtcoolguy
Copy link
Contributor Author

ok, I think I addressed the remaining review feedback here? I'm gonna tag for QE to test now...

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.

CR: Pass

@sgtcoolguy sgtcoolguy force-pushed the semantic-color branch 2 times, most recently from df4ed4c to 22a0351 Compare April 30, 2020 18:42
@ssekhri
Copy link

ssekhri commented May 8, 2020

FR Passed.
The fetchSemanticColor() method and userinterfacestyle event work fine on android and iOS for dark and light mode.
Verified on:
Mac OS: 10.15.4
SDK: 9.1.0.v20200430122302
Appc CLI: 8.0.0
JDK: 11.0.4
Node: 10.17.0
Xcode: 11.4
Device: iPhoneX(v13.4), iOS simulator 13.4, Pixel 3(v10.0), Pixel 2 emulator(v10.0)

@ssekhri ssekhri merged commit 28eba34 into tidev:master May 8, 2020
@sgtcoolguy sgtcoolguy deleted the semantic-color branch May 9, 2020 12:47
@hansemannn
Copy link
Collaborator

A tiny addition to this great step forward: TIMOB-27895

@KamalKant-Tech
Copy link

This is event is not triggering for me when I change the theme mode in android and iOS.

Ti.UI.addEventListener('userinterfacestyle', e => {
    console.log(e);
    console.log(primaryBackground.toHex());
});

@ewanharris
Copy link
Collaborator

@KamalKant-Tech, what SDK are you using? This will ship in SDK 9.1.0, and the event is firing ok for me when I use an SDK from that branch (appc ti sdk install -b 9_1_X). Before commenting on PR's, please make sure to check the JIRA ticket (usually linked in the description) and check the Fix Version/s field there against your SDK version

@KamalKant-Tech
Copy link

@ewanharris Thanks, I got the issue it was not working because of SDK version. I have been using 9.0.3 and then I moved to 9.1.0 now it works.

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

9 participants