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

Jump to bottom

Android: Use DialogFragment for AlertMessage #8833

Merged
merged 2 commits into from Oct 10, 2020
Merged

Android: Use DialogFragment for AlertMessage #8833

merged 2 commits into from Oct 10, 2020

Conversation

Ebola16
Copy link
Member

@Ebola16 Ebola16 commented May 28, 2020 •
edited

Fixes https://bugs.dolphin-emu.org/issues/7988
Fixes https://bugs.dolphin-emu.org/issues/10770

As far as I can tell, this works (even with multiple panic alerts in quick succession) but there are some things that I'm not sure I've done properly:

  • Did I properly handle AlertMessages while booting the core?
  • Since NativeLibrary.displayAlertMsg has a lock object, pausing emulation while waiting on the lock would freeze AlertMessages after rotating the screen. As far as I can tell, pausing emulation here seems unnecessary but correct me if I'm wrong.

Test case was https://bugs.dolphin-emu.org/issues/11424

@Ebola16 Ebola16 force-pushed the Panic branch 4 times, most recently from 4ca8db0 to 8fadc68 Compare May 28, 2020 03:22
@JosJuice
Copy link
Member

Since NativeLibrary.displayAlertMsg has a lock object, pausing emulation while waiting on the lock would freeze AlertMessages after rotating the screen. As far as I can tell, pausing emulation here seems unnecessary but correct me if I'm wrong.

Yes, not pausing emulation is probably the best workaround. This is the same issue as with PR #8819.

JosJuice
JosJuice reviewed May 28, 2020
View reviewed changes
Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/NativeLibrary.java Outdated Show resolved Hide resolved
Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/fragments/EmulationFragment.java Outdated
{
if (NativeLibrary.GetAlertMessageText() != null)
{
throw new IllegalStateException(NativeLibrary.GetAlertMessageText());
Copy link
Member

Choose a reason for hiding this comment

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

Won't this just make it crash instead of deadlock? Or is there something I'm missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'm not sure the best way to handle this. The choices I thought of were deadlocking for an unknown reason or crashing for a known reason. I also assume panic alerts during core booting indicate major emulation problems anyway. This does fix AlertMessages outside of core booting so I think it's a worthwhile change that isn't worse than what we currently have. I'm open to other suggestions though.

Copy link
Member

Choose a reason for hiding this comment

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

I guess the other option would be to dismiss the dialog if you try to rotate the screen while showing an alert message while booting. While it does avoid crashing, it is a bit odd, so I'm not sure if it's actually better...

I guess either a crash or what I mentioned would be okay, but you should leave a comment about it in either case (either about this crash being intentional, or about why we dismiss the dialog).

Copy link
Member

Choose a reason for hiding this comment

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

Also, if you're going to have it crash, it probably makes more sense to always include "Attempted to destroy surface while booting the core" in the exception message (perhaps with the alert message text appended), since the error that causes the alert message to be shown is not in itself the reason why we are crashing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried another idea. Instead of deadlocking or crashing the emulator, we can simply end emulation. The AlertMessage has been replaced with a log error and toast. Hopefully this won't cause unforeseen problems.

Copy link
Member

Choose a reason for hiding this comment

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

If WaitUntilDoneBooting is called instead of stopping emulation, we'll get the deadlock.

Yes, unless we release the lock first. My suggestion is that we dismiss the dialog and release the lock, and then show a toast.

Copy link
Member

Choose a reason for hiding this comment

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

I just realized that regardless of what we do about this case, we're still going to get a deadlock in the case where we get an alert message after WaitUntilDoneBooting already has been called... I wonder if we maybe just should show all alert messages that are triggered when booting as toasts without even attempting to use a proper dialog.

Copy link
Member Author

@Ebola16 Ebola16 May 28, 2020 •
edited

Choose a reason for hiding this comment

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

I could use the new NativeLibrary.IsBooting check to handle alert messages differently during core boot. Just to clarify, this PR doesn't deadlock with alert messages after core boot because it skips calling WaitUntilDoneBooting for AlertMessages, which only seems to be a problem when the core is booting.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, there is no problem after we're done booting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moving the implementation of IsBooting and making alert messages toasts while the core is booting seems to be a good solution here. Hopefully this version does the trick!

Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/dialogs/AlertMessage.java Outdated Show resolved Hide resolved
Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/NativeLibrary.java Outdated Show resolved Hide resolved
JosJuice
JosJuice reviewed May 28, 2020
View reviewed changes
Source/Core/Core/Core.cpp Outdated Show resolved Hide resolved
@Ebola16 Ebola16 force-pushed the Panic branch 2 times, most recently from 8d4bd09 to 16a2143 Compare May 28, 2020 15:40
JosJuice
JosJuice approved these changes May 28, 2020
View reviewed changes
Copy link
Member

@JosJuice JosJuice left a comment

Choose a reason for hiding this comment

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

The code seems okay now, though I would like someone else to look through this before merging.

@Ebola16
Copy link
Member Author

Ebola16 commented May 28, 2020

Thanks for the help! This PR went in directions I didn't expect.

@Ebola16 Ebola16 changed the title [Feedback Wanted] Android: Use DialogFragment for AlertMessage Android: Use DialogFragment for AlertMessage May 28, 2020
@Ebola16 Ebola16 mentioned this pull request Jun 8, 2020
Merged
1 task
@Ebola16 Ebola16 mentioned this pull request Jun 16, 2020
Merged
@Ebola16
Copy link
Member Author

Ebola16 commented Jun 23, 2020

Minor typo fix. I accidentally called the "AlertMessage" a "Panic Alert" in NativeLibrary.

@JosJuice
Copy link
Member

Well, it seems like nobody else is going to review this...

@JosJuice JosJuice merged commit 5a939cc into dolphin-emu:master Oct 10, 2020
@Ebola16 Ebola16 deleted the Panic branch October 10, 2020 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JosJuice JosJuice JosJuice approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
2 participants
@Ebola16 @JosJuice