nextcloud / android

📱 Nextcloud Android app
https://play.google.com/store/apps/details?id=com.nextcloud.client
GNU General Public License v2.0
4.3k stars 1.77k forks source link

Pressing back on the home screen does not exit app when in the first run activity #11392

Open smanilov opened 1 year ago

smanilov commented 1 year ago

⚠️ Before posting ⚠️

Steps to reproduce

  1. Open the app without logging in
  2. Press the system back button

Expected behaviour

The app should exit. This would be normal Android app behavior.

Actual behaviour

The home screen is presented again.

Android version

13

Device brand and model

Samsung Galaxy S22 Ultra

Stock or custom OS?

Stock

Nextcloud android app version

stable-3.24.0, gplayDebug

Nextcloud server version

Irrelevant

Using a reverse proxy?

I don't know

Android logs

bug.log

Server error logs

Irrelevant.

Additional information

Server info is irrelevant, because the bug appears before connecting.

AlvaroBrey commented 1 year ago

This is not a bug, just a behaviour that is not implemented. Right now the app just delegates to the system default back pressed handler, which differs depending on Android version; in most modern versions this just moves the app to Recents and shows the launcher.

For the record, I don't think we should change this. But I'm leaving this open for discussion.

smanilov commented 1 year ago

I wasn't sure whether to call it a bug, but thought that felt more accurate. I see how it could be seen as a feature request too.

You said: "in most modern versions this just moves the app to Recents and shows the launcher.". But in my case, this is not what I'm observing, while my phone and OS are fairly recent (less than a year old model; updated to Nov 22 Android).

What I'm observing is that the screen goes away, just to reappear immediately. That's why I think it's a bug.

smanilov commented 1 year ago

The app is running in this code, which seems wrong to me:

    public void onBackPressed() {
        onFinish();

        if (getIntent().getBooleanExtra(EXTRA_ALLOW_CLOSE, false)) {
            Log.d("stanm", "onBackPressed: EXTRA_ALLOW_CLOSE == false");
            super.onBackPressed();
        } else {
            Log.d("stanm", "onBackPressed: EXTRA_ALLOW_CLOSE == true");
            Intent intent = new Intent(getApplicationContext(), AuthenticatorActivity.class);
            intent.setFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP);
            intent.putExtra(EXTRA_EXIT, true);
            startActivity(intent);
            finish();
        }
    }

This is from app/src/main/java/com/nextcloud/client/onboarding/FirstRunActivity.java.

Specifically, ~I get EXTRA_ALLOW_CLOSE == true~, (EDIT: I don't; see https://github.com/nextcloud/android/issues/11392#issuecomment-1433086867) but the code seems to invoke the AuthenticatorActivity, instead of actually closing the app. I wonder why is that.

smanilov commented 1 year ago

I found a commit that attempts to do just this (according to the commit message): "back press on first run activity closes app"

https://github.com/nextcloud/android/commit/b7089df901bd4d3e09043628579f1dc76137f09a

It's the same commit that introduced the lines that I quoted in my previous comment.

So it seems that it is a desirable behavior.

cc'ing the author of the commit: @tobiasKaminsky

AlvaroBrey commented 1 year ago

I found a commit that attempts to do just this (according to the commit message): "back press on first run activity closes app"

b7089df

It's the same commit that introduced the lines that I quoted in my previous comment.

So it seems that it is a desirable behavior.

cc'ing the author of the commit: @tobiasKaminsky

Ah, I see, and I can reproduce this, but only in the first run activity. I incorrectly read your issue, my apologies. This is indeed a bug, most likely.

AlvaroBrey commented 1 year ago

Looks like the original intention was to fix https://github.com/nextcloud/android/issues/2476, for reference

smanilov commented 1 year ago

Thanks for looking into this further.

Re: my comment https://github.com/nextcloud/android/issues/11392#issuecomment-1433033282

I mixed up the logic, i.e. the debug prints. They should be flipped, so EXTRA_ALLOW_CLOSE is actually false.

I wonder whether this is intended.

smanilov commented 1 year ago

So I narrowed it down to the following sequence of operations:

FirstRunActivity:onBackPressed()

AuthenticatorActivity:onNewIntent()

BaseActivity:onRestart()

It is unclear to my why onRestart() is called. From the [documentation](https://developer.android.com/reference/android/app/Activity#onRestart()), it should only happen when "the user has navigated back to it". But it is triggered automatically for some reason.

AuthenticatorActivity is first started from UserAccountManagerImpl:startAccountCreation via context.startActivity().

Any ideas?

Could it be, that an override of onStop() inside the main activity (FileDisplayActivity) is necessary? (An override that would stop all services, which are started in onStart(), or something of the sort.)

smanilov commented 1 year ago

I managed to build versions 3.13, 3.15 and 3.18 and the bug can be reproduced.

Unfortunately, I cannot figure out how to build older versions (the build fails, probably because I don't have the right toolchain).

AlvaroBrey commented 1 year ago

I managed to build versions 3.13, 3.15 and 3.18 and the bug can be reproduced.

Unfortunately, I cannot figure out how to build older versions (the build fails, probably because I don't have the right toolchain).

No need to do this, the above change was introduced in 3.6.0, so all of the following versions should have the bug.

smanilov commented 1 year ago

Good point. I actually wanted to bisect further back, but, again, I couldn't build versions older than 3.13.

I'm thinking that the change introduced in 3.6.0 might not be the problem.

joshtrichards commented 2 months ago

Kinda related: #4993