fix(android): override user interface style

[AbdullahFaqeir]

• A PR fixes #13825

Fixes an unexpected behaviour when trying to change the dark/light mode multiple times which causes the screen to go blank and made the change between both animated.

[m1ga]

The last changes make the app restart correctly now :+1: Which makes me wonder: are the other changes needed at all? Do we need a default windowAnimationStyle ?

At some point we'll need to find the source why it is behaving like this after 4-5 clicks but at least it will show the app again.

Would love to have some community members to test this as there are some overrideUserInterfaceStyle users out there.

[AbdullahFaqeir]

I'll explain why it's happening when I get on my laptop

[AbdullahFaqeir]

@m1ga The whole story begins here https://github.com/tidev/titanium-sdk/blob/abbd387dedeec56ce1841196bf04e92a7622e410/android/titanium/src/java/org/appcelerator/titanium/TiRootActivity.java#L171

At some point while clicking, the activity gets duplicated, which is the root activity of the app, when going inside this condition, we lose the state of the activity.

[AbdullahFaqeir]

@m1ga Here's what's happening.

I assumed that since you keep clicking on the button and internally the activity is being recreated which in turn starts the while life cycle of the activity which is the root activity, at some point, the root activity gets duplicated. which leads us to the following

https://github.com/tidev/titanium-sdk/blob/50d8604ea40b4c2aacb55ee1128a2706f421715c/android/titanium/src/java/org/appcelerator/titanium/TiRootActivity.java#L154-L174

if you go over the comments, you'll see at the very last line before calling activityOnCreate() that is bypassing the TiBaseActivity.onCreate() thus, after jumping to specially the TiBaseActivity.onCreate(), we'll see the real effect of what happened.

See below: https://github.com/tidev/titanium-sdk/blob/50d8604ea40b4c2aacb55ee1128a2706f421715c/android/titanium/src/java/org/appcelerator/titanium/TiBaseActivity.java#L685-L696

By bypassing the TiBaseActivity.onCreate() we are stepping over the bunch of code that actually recreate the window and brings back whatever views the window had in it.

We ara stepping over this as well https://github.com/tidev/titanium-sdk/blob/50d8604ea40b4c2aacb55ee1128a2706f421715c/android/titanium/src/java/org/appcelerator/titanium/TiBaseActivity.java#L722-L725 And I believe many other things that would cause the window/activity to lose it's state.

I don't know if all of this makes since to you!, let me know

[AbdullahFaqeir]

@m1ga any updates on this one?

[m1ga]

@AbdullahFaqeir as mentioned on Slack: I'll take a look at it again at the weekend.

And I think we should just use the one line from the last commit as that was fixing the issue, not the other default parts. But I'll test all at the weekend again and give more feedback

[m1ga]

Ok, I'll tested it again and I think we can reduce this PR to just one line: just adding getTiApp().setRootActivity(null); before: https://github.com/tidev/titanium-sdk/blob/master/android/titanium/src/java/org/appcelerator/titanium/TiBaseActivity.java#L1242

fixes the issue already. That would reduces the possible site effects as we don't change the XML or anything else in the BaseActivity besides the part inside the if condition (which is only triggered when you change the interface style)

[AbdullahFaqeir]

@m1ga I've created a new method to handle the recreation because there's another places where the activity is recreated for the style https://github.com/tidev/titanium-sdk/blob/e1212e9fccd79f5c302a8380157288df356081da/android/titanium/src/java/org/appcelerator/titanium/TiBaseActivity.java#L1259C4-L1259C33

[m1ga]

I saw that but it didn't go into that method so it wasn't needed there in my tests. It was only needed in the one place.

[AbdullahFaqeir]

@m1ga it would in other cases, I'm not only fixing your case, I'm doing something to handle all cases in a proper way.

[m1ga]

I totally understand that :+1: Just want to make sure that the setTheme parts or the default transition will not introduce any change will make a default app behave different with 12.6.0 compared to 12.5.0.GA. E.g. in the onCreate you set setTheme(selectedTheme); which is R.style.Theme_Titanium_Light. But what if the user sets a custom theme in tiapp.xml or js? Will that work correctly? I didn't notice any difference with or without it, so if you have a test case where it is needed I would be very happy to test it.

The updateActivity() part works great

[AbdullahFaqeir]

@m1ga Can we conclude this? like do a final review showing me what you want and don't want

[m1ga]

I think for this issue the two switches from

ActivityCompat.recreate(this);

getTiApp().setRootActivity(null);
ActivityCompat.recreate(this);

should be enough (so your updateActivity() method). This will fix the initial issue and can be tested with the code.

Keep the other layout/style/transition changes for another PR so we can test that with another example. As it doesn't change anything for the issue I'm a bit concerned that it might add changes to existing apps and that is not what we want. I just can't test it to see what it will fix.

[m1ga]

@AbdullahFaqeir just a quick bump to see if you could update the PR as describe in the comment above (just keep the this.updateActivity(); + method in this PR, move the other fixes [theme stuff] into another PR). Then we can finish this one and test it quicker.

It's a big one again (even if it's just a few lines of code :smile: ) and improves the SDK a lot :+1:

[prashantsaini1]

@m1ga @AbdullahFaqeir I doubt that setting the rootActivity to null would break notification/data-intent/shortcut handling as rootActivity is responsible to fire the newintent event for such scenarios via Ti.Android.rootActivity.addEventListener('newintent', () => ());, which is why rootActivity is never set to null.

[prashantsaini1]

@m1ga @AbdullahFaqeir As I mentioned in last comment too, today I found a scenario where it's breaking the app (almost badly). The most common use case is to let an app remain in some fixed light/dark mode irrespective of the system mode. Very common case with many Android apps which allow users to opt among Same as system/ Light / Dark modes.

alloy.js

// set user defined style
Ti.UI.overrideUserInterfaceStyle = Ti.UI.USER_INTERFACE_STYLE_DARK;

// Above code makes `rootActivity` null, thus the below code crash the app.
Ti.Android.rootActivity.addEventListener('newintent', () => {});

// open first window afterwards...

---

However, if we alter the sequence of above code,

alloy.js

// `rootActivity` is not null as of now
Ti.Android.rootActivity.addEventListener('newintent', () => {});

// Set user defined style.
Ti.UI.overrideUserInterfaceStyle = Ti.UI.USER_INTERFACE_STYLE_DARK;

// open first window afterwards...

• 2nd snippet makes rootActivity null again and newintent event will not be fired.
• The only way to get newintent event fired is to add it again but we cannot precisely know when the rootActivity becomes available again.
• Though after changing style, rootActivity becomes available after sometime via setTimeout, but this is not a consolidated approach to run rootActivity related code again.
• I almost found a way to get rootActivity, it's available again in Ti.UI.addEventListener('userinterfacestyle', () => {}); method, but again rootActivity is null if the style is now changed via system (not the user one).

So this PR actually makes rootActivity null which must not be done until the app is meant to be totally restarted. I highly doubt that it would break some other codes/modules which generally rely on rootActivity.

[m1ga]

Hi @prashantsaini1 as you can see in the initial issue: https://github.com/tidev/titanium-sdk/issues/13825#issue-1695892821 the test code had to do with Ti.UI.overrideUserInterfaceStyle already. Changing Ti.UI.overrideUserInterfaceStyle was breaking the apps before already that is why this PR started in the first place. BTW: I don't think you should run that in alloy.js even with 12.5.1 it was restarting the app for people and ended up in a non starting state.

[prashantsaini1]

@m1ga Actually this is not related to alloy.js, but accessing the rootActivity code after we change style via Ti.UI.overrideUserInterfaceStyle as it makes rootActivity null.

Fact is that apps do not break by setting Ti.UI.overrideUserInterfaceStyle, they break by setting it too frequently as in the initial issue. I tested all cases (except setting it too fast) with 12.5.1.GA, and all works fine.

I even figured out a more general use case without even setting Ti.UI.overrideUserInterfaceStyle at all. Just change the system theme in a simple test app and it would make rootActivity null forever.

Run this app and change theme from system settings:

const win = Titanium.UI.createWindow({backgroundColor: "red"});
const btn = Ti.UI.createButton({title: "change"});
win.add(btn);
win.open();

Ti.UI.addEventListener('userinterfacestyle', (e) => {
	// Ti.Android.rootActivity is reported null if system theme is changed.
});

I believe we need to reassess this PR with mentioned use cases that some apps may want to provide user specific light/dark modes, or may be relying on rootActivity.

[m1ga]

I think we should continue this in the old issue (i can reopen it) or create a new issue for it :smile:

I was digging through some old Slack logs:

2023/06/11: After a lot of testing, I found that the following instruction (I was executing it in alloy.js) creates problems on Android when the app is started (not simply resumed) by a notification click: Ti.UI.overrideUserInterfaceStyle = Ti.UI.USER_INTERFACE_STYLE_LIGHT;

2024/05/03: I found another bugs; the app will hang if I click the notification while the app is closed or killed. This line will make the app hang->. Android only. Ti.UI.overrideUserInterfaceStyle = Ti.UI.USER_INTERFACE_STYLE_LIGHT;

and I had some private chats like this one: so i kind of found the cause of it, its caused by Ti.UI.overrideUserInterfaceStyle. commenting it out fixes it

overrideUserInterfaceStyle always broke apps if you set it by hand in some cases. The "quick" switch was the only way I could reproduce it. It will restart the activities in a different order at some point (not sure if it's speed but only doing it more then 4 times).

I'm more than happy if you find a way without resetting the rootActivity and making it work as expected! It looks like more people are using this and not Ti.Android.rootActivity. Didn't see any complaint so far with the new SDK.

[prashantsaini1]

@m1ga Just a quick update - I did lots of testing on weekend to get the proper fixes to following problems:

1. Set theme for entire app upon fresh launch (before any window is opened).
2. Override theme frequently.

Solution for # 1:

• Set the app theme in TiApplication.java class, which is actually the most correct place. Native Android apps also set whole app theme in Application class as well.
• Since alloy.js code cannot be run before rootActivity is created, we can use Ti.App.Properties using a pre-defined key-name to set the user defined theme for entire app on launch, rather than setting Ti.UI.overrideUserInterfaceStyle in alloy.js.
• This avoids the app restart again right after the launch and keeps rootActivity always alive.
• It will solve problems for many people like you mentioned who got stuck on this.
• I have tested this solution and works fine normally with single app start, with notification and intent launchers cases as well.

Notes for # 2:

• Setting rootActivity null is only required if rootActivity itself is the visible screen, which was the case with the initial sample.
• I further found that if user directly changes device theme, rootActivity becomes null due to the same method updateActivity being called there as well.
• We should ideally set rootActivity to null only if the currently visible activity is the rootActivity.
• I am running more tests around this solution.

I'd appreciate other suggestions!

[m1ga]

@prashantsaini1 that sounds great! Thanks for putting so much effort in it.

I know there was some theme setting in this PR in the first commits. As they didn't make any difference in my testes I've requested to removed those but it looks like I was wrong about this.

Looking forward to do some tests here with your solutions

[prashantsaini1]

@m1ga Thanks for the hint on theme settings earlier in this PR. I will review them as well to cover up more scenarios!

[m1ga]

@prashantsaini1 did you make any progress on this one yet? It sounded like you had some code already. Happy to test it if there is something yet

[prashantsaini1]

@m1ga Thanks for reminding on this one. Almost got slipped due to super busy weeks with some frequent sick days. I'll try to add final code at earliest within Feb, so you can give some test runner.