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.

Sign up for GitHub

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): prevent BaseActivity to stay with exitOnClose:false #14159

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

fix(android): prevent BaseActivity to stay with exitOnClose:false #14159

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions android/titanium/src/java/org/appcelerator/titanium/TiApplication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,15 @@ public static void addToActivityStack(Activity activity)

public static void removeFromActivityStack(Activity activity)
{
// check if no windows are left - happens when closing windows quickly with exitOnClose:false
if (TiActivityWindows.getWindowCount() == 0) {
Activity currentActivity = getAppCurrentActivity();
if (currentActivity instanceof TiBaseActivity) {
// close app otherwise it won't restore correctly
Copy link
Contributor

Choose a reason for hiding this comment

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

@m1ga I think this will need some additional condition checks for exitOnClose.

  • For exitOnClose: false window, though it prevents to show splash screen, but it will close entire activity stack too which is not intended.
  • Also, for exitOnClose: true window, it may not dispose JS runtime properly as it's terminating stack without checking whether JS script is running or not.

We need more testing on this change as it can break the behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea here is to close the window in the exitOnClose:false state because we've ended up in a place where we shouldn't be. So I don't respect the false part and close the app so it has to restart next time. This is better then it's being stuck at the splashscreen.
If you close the windows in a normal speed it won't end up in that if case.

exitOnClose: true shouldn't change with the PR as it won't go into that if condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • That's exactly I meant in my 1st point in last comment. exitOnClose: false won't work as intended with this PR.

There are likely two scenarios an app would want to do on back-press:

  1. Do not exit - we set exitOnClose: false on first window and open home-intent on its back button press.
    • This would surely ignore enableOnBackInvokedCallback which we can improve in future releases.
  2. Exit - it's already the default behaviour now since we fixed the exitOnClose default value in this PR.

Or is there any other case I am missing here that you are trying to achieve? 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently with exitOnClose: false it will put the app to the background and not close it when you "close" the last window. This works fine unless you close two windows quickly. Then it is not putting the app to the background but stays at the splashscreen/base actitvity. Check the demo code + videos.

This is the whole issue I'm trying to fix here by "force closing" the app when we end up in that state. So it will be the same as exitOnClose:true for this edge case.

The issue is a due to the system close animations and closing it quicker before the system close animation is done and everything is cleaned up.
Normally it will end up in a state where you have 1 window in the stack and Ti will put it to the background because of exitOnClose:false. But when you do it quickly it will end up with a zero window stack and it doesn't know what to do here.

The video under Different approach: will show you another way where I've blocked the "back" until the animation is done. But for me that looks a bit like the app is not doing what you want it do do.

And to test it you have to have normal window animations enabled on your phone and close the 2nd window + 1st window very quickly 😄 I think it's an edge case but since Ti is that performant that you can close them so quickly we should add the fix for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Taking an excerpt from your last comment:

Currently with exitOnClose: false it will put the app to the background and not close it when you "close" the last window. This works fine unless you close two windows quickly. Then it is not putting the app to the background but stays at the splashscreen/base actitvity.

  • With your PR, will it still put apps to background without closing when back-press happens at normal speed?
    • If yes, then this PR is probably fine.
    • If not, then it introduce a breaking change as users won't know that their app will be closed in an edge case.

Another solution: Rather terminating the app, we launch the home-intent in this case in SDK itself, because then it still keeps the same purpose of setting exitOnClose: false, the app won't be killed.

Copy link
Contributor

@prashantsaini1 prashantsaini1 Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@m1ga Thanks for confirming the androidback.

Your only concern is now that the app didn't close when you called win1.close(), it's because you have to reset its exitOnClose: true again.

Ideally root Ti.Window (win1) won't be closed programmatically just like that. It will be closed if users really want to open another screen, e.g. signing out the app and opening login/signup screen.

  • In general if I want to call win1.close(), either:
    • I want to close the app, so I set exitOnClose: true again on win1 before calling it's close().
win2.addEventListener("close", function(){
   win1.exitOnClose = true; // reset it here and app closes fine just like this PR.
   win1.close();
});
  • Or I want to open another screen, which does not need any further changes.
win2.addEventListener("close", function(){
	win1.close();

	var win3 = Ti.UI.createWindow({
		backgroundColor: "teal",
		exitOnClose: false // `false` will put app to background again, `true` will close the app.
	})
	win3.open();
})

To summarise, you have to take care of adding/removing androidback and setting exitOnClose depending on what you want to achieve.

If you want, we can jump over a Slack call and test things out together to see if there's really an edge case we cannot handle with current SDK features (obviously after normal working hours 😊).

Copy link
Contributor

@prashantsaini1 prashantsaini1 Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@m1ga would love to hear your further updates if there's still any open issue!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

while the JS code works it's just a workaround that I don't want to add in all apps or have other devs find this out and add it.

As a app dev I just want to set exitOnClose:false and no think about switching it or using androidback. So the issue still exists without this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand your use case which is possible when users presses back button too fast.

My whole point is that this PR is changing the meaning of exitOnClose: false in this edge case as apps will exit anyways.

However, if you still want to take this forward, then I would strongly suggest you to verify following scenarios after the app is terminated in this edge case:

  1. Clicking on notification should bring the app to foreground.
  2. Sending data to app via intent-filters should be delegated to app as expected.

These scenarios are very common ways to invoke the app and should work same as with current SDK.

We should rather find a proper way to put the app to background no matter how fast the back button is pressed. I will try to give this issue a proper fix in coming weeks (not very soon though).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for all the infos. Will test those cases 👍

If we find the real lifecycle issue it would be great! No rush here, no user report so far about this edge case. Thank you for taking the time to answer with all the details 👍

currentActivity.finishAffinity();
TiApplication.terminateActivityStack();
}
}
if (activity != null) {
activityStack.remove(activity);
}
Expand Down
Loading