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

Fix background crash focus #11789

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

Thompson3142
Copy link
Contributor

@Thompson3142 Thompson3142 commented Dec 12, 2024

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

This PR addresses the issue of NewPipe stealing focus to report an error even while it is in the background.
To check if the app has been in the background a monitoring class (that could also be used for different purposes) and a shared preference have been introduced to ensure that the state is correctly remembered and can then handle the reporting accordingly e.g. only show a notification if the crash happened while in foreground and show a not.
This was tested via delayed test crashes:

new Handler(Looper.getMainLooper()).postDelayed(() -> {
            // Deliberately throw a RuntimeException
            throw new RuntimeException("Simulated crash for testing purposes.");
        }, 20000);

Occasionally i have seen this misbehave seemingly when another crash occurred before with a different foreground/background state. Unfortunately i have not found a fix for this, if somebody wants to take a look or can reproduce it I would be grateful. I am also open to completely different approaches since this approach is of course not optimal.

Fixes the following issue(s)

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.

Due diligence

@github-actions github-actions bot added the size/medium PRs with less than 250 changed lines label Dec 12, 2024
@ShareASmile ShareASmile added the bug Issue is related to a bug label Dec 15, 2024
Comment on lines 24 to 32
override fun onStart(owner: LifecycleOwner) {
editor.putBoolean(KEY_IS_IN_BACKGROUND, false).commit()
Log.d(TAG, "App moved to foreground")
}

override fun onPause(owner: LifecycleOwner) {
editor.putBoolean(KEY_IS_IN_BACKGROUND, true).commit()
Log.d(TAG, "App moved to background")
}
Copy link
Member

Choose a reason for hiding this comment

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

Can't you override these functions directly in MainActivity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can, i just missed that AppCompatActivity has these methods to overwrite. Unfortunately it does not seem like it fixes the occasionally wrong behavior i mentioned, probably something related to ACRA but I am unsure how to fix it.

@github-actions github-actions bot added size/small PRs with less than 50 changed lines and removed size/medium PRs with less than 250 changed lines labels Dec 17, 2024
@Profpatsch
Copy link
Contributor

Can we add a button to the Settings -> Debug menu that does a delayed crash, so you can simulate the problem here? I don’t know how to reproduce it.

@Thompson3142
Copy link
Contributor Author

I don't know how necessary it is to have a dedicated button for that, but if you want to reproduce it, you can do it very easily using something like:

new Handler(Looper.getMainLooper()).postDelayed(() -> {
            // Deliberately throw a RuntimeException
            throw new RuntimeException("Simulated crash for testing purposes.");
 }, 20000);

You could create a new button like the crashTheAppPreference in DebugSettingsFragment with this snippet, what i did was simply calling it when the player is opened but thats up to you.

@Profpatsch
Copy link
Contributor

I couldn’t reproduce it by either inserting that into MainActivity nor Player; do you have a commit with the exact position where you added that?

@Thompson3142
Copy link
Contributor Author

I never commited it because I don't wanted to mess with the git history, maybe I am missunderstanding though what you can not reproduce?

@Profpatsch
Copy link
Contributor

what you can not reproduce?

I cannot reproduce the error screen popping up and taking focus when NewPipe is in the background.

If you want, you can commit the example on a fresh branch and just link the branch or the commit directly.

@Thompson3142
Copy link
Contributor Author

Maybe it's something device specific? I can provide a commit that I used to test but it really just boils down to adding the code I mentioned into the first line after handleIntent in the Player class. Also can you not reproduce it in general or just with my attempt to fix it?

Comment on lines +199 to +211
@Override
protected void onStart() {
super.onStart();
sharedPrefEditor.putBoolean(KEY_IS_IN_BACKGROUND, false).apply();
Log.d(TAG, "App moved to foreground");
}

@Override
protected void onStop() {
super.onStop();
sharedPrefEditor.putBoolean(KEY_IS_IN_BACKGROUND, true).apply();
Log.d(TAG, "App moved to background");
}
Copy link
Member

Choose a reason for hiding this comment

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

In an app of mine I used a static field instead of the preferences, since there is only going to be one MainActivity alive at a time anyway: https://github.com/Stypox/dicio-android/blob/master/app/src/main/kotlin/org/stypox/dicio/MainActivity.kt#L178
Maybe it would be better if you switched to a static field now that I think of it, so you can make the code simpler and not rely on shared preferences (which do disk I/O every time).
Also, put the Log.d behind an if (DEBUG) { ... }

@Stypox
Copy link
Member

Stypox commented Jan 25, 2025

Maybe it's something device specific?

Yes I think from Android 13 the background intents are blocked anyway

@Profpatsch
Copy link
Contributor

Ah, so I need to test on an older Android version, I see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug size/small PRs with less than 50 changed lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not steal focus to report a guru meditation
4 participants