tidev / titanium-sdk

🚀 Native iOS and Android Apps with JavaScript
https://titaniumsdk.com/
Other
2.76k stars 1.21k forks source link

fix(android): override user interface style #14113

Open AbdullahFaqeir opened 2 months ago

AbdullahFaqeir commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

AbdullahFaqeir commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@m1ga any updates on this one?

m1ga commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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

m1ga commented 1 month ago

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 commented 2 weeks ago

@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: