Use PanicAlertT/PanicAlert when appropriate #2213

JosJuice · 2015-03-15T16:27:21Z

Read the commit messages for the rationale. I'm not sure if the second commit is good, so if someone wants me to drop it, I'm fine with doing so.

magumagu · 2015-03-15T19:13:33Z

I think we really need a more principled approach to this sort of thing... there are sort of a few categories of places we use PanicAlert():

Something fails, and it's caused by some sort of misconfiguration we expect the user to deal with. (The user specified an invalid IPL dump, the user is trying to use DSPHLE with homebrew, the user is trying to compress an ISO to a drive without enough disk space, etc.). In this case, we want a completely custom message, which describes what appears to be wrong, and how the user might fix it. We never want to suppress this. Let's call this UserErrorAlert(). (This is what we want to use for most calls to PanicAlertT().)
Something failed, and we can deal with it, but it probably means that the game crashed. (Bad FIFO opcodes, bad pointers, etc.) This may or may not be the user's fault (maybe a Dolphin bug, maybe a games naturally crashing, maybe a bad cheat code). We want some sort of generic message indicating this, plus an untranslated technical description which a user can copy-paste, and we want to automatically suppress these after the first. Let's call this EmulationFailureAlert(). (This is what we want to use for most calls to PanicAlert().)
Something failed, and it probably indicates a bug in Dolphin (basically assertion failures). We want some sort of generic "fatal error" message. This is _assert_msg_() (although some things we use _assert_msg_() for probably shouldn't use it).

With better names and implementations, it becomes much more obvious what we want to call where.

Source/Core/Common/MemoryUtil.cpp

@@ -69,7 +69,8 @@ void* AllocateExecutableMemory(size_t size, bool low)
 	{
 		ptr = nullptr;
 #endif
-		PanicAlert("Failed to allocate executable memory. If you are running Dolphin in Valgrind, try '#undef MAP_32BIT'.");
+		PanicAlertT("Failed to allocate executable memory.\n"
+		            "If you are running Dolphin in Valgrind, try '#undef MAP_32BIT'.");


Source/Core/Core/HW/DSPHLE/UCodes/UCodes.cpp

@@ -87,12 +87,12 @@ UCodeInterface* UCodeFactory(u32 crc, DSPHLE* dsphle, bool wii)
 	default:
 		if (wii)
 		{
-			PanicAlert("DSPHLE: Unknown ucode (CRC = %08x) - forcing AXWii.\n\nTry LLE emulator if this is homebrew.", crc);
+			PanicAlertT("DSPHLE: Unknown ucode (CRC = %08x) - forcing AXWii.\n\nTry LLE emulator if this is homebrew.", crc);


JosJuice · 2015-03-15T19:51:50Z

@magumagu Thanks for all the comments. I'm working on fixing the message-specific problems. Your longer comment about a panic alert overhaul was great and made me interested in working on something like that in the future. It would be a bit complicated, with both a UI overhaul and changes in lots of small places, but it would be worth it.

JosJuice · 2015-06-04T10:55:50Z

Since the translation process for 5.0 seems to have started now and this PR adds and changes many translation strings, I don't think it should be merged until after 5.0. We should give the translators as much time as possible.

degasus · 2015-06-04T11:00:32Z

@JosJuice It hasn't started right now, so we're fine to merge this. But you're right, we shouldn't delay this PR much longer.

JosJuice · 2015-06-04T11:07:02Z

Okay, merging this soon sounds good to me then. I will rebase it right away.

Source/Core/VideoCommon/ImageWrite.cpp

@@ -51,23 +51,22 @@ bool TextureToPng(u8* data, int row_stride, const std::string& filename, int wid
 	png_ptr = png_create_write_struct(PNG_LIBPNG_VER_STRING, nullptr, nullptr, nullptr);
 	if (png_ptr == nullptr)
 	{
-		PanicAlert("Screenshot failed: Could not allocate write struct\n");
+		PanicAlertT("Screenshot failed: Could not allocate write struct");


degasus · 2015-06-04T11:21:23Z

lgtm

I tried to change messages that contained instructions for users, while avoiding messages that are so technical that most users wouldn't understand them even if they were in the right language.

It means less work for the translators... But I'm not too sure about this, because most of these have already been translated.

Use PanicAlertT/PanicAlert when appropriate

magumagu reviewed Mar 15, 2015
View reviewed changes

JosJuice force-pushed the panic-alert-t branch from 4e4c64e to 80e7955 Compare March 15, 2015 19:19

magumagu reviewed Mar 15, 2015
View reviewed changes

JosJuice force-pushed the panic-alert-t branch 3 times, most recently from fc7ad99 to 40c61c0 Compare March 15, 2015 20:03

JosJuice force-pushed the panic-alert-t branch from 40c61c0 to acf15a3 Compare April 15, 2015 13:10

JosJuice force-pushed the panic-alert-t branch from acf15a3 to 9e438a7 Compare April 23, 2015 20:07

JosJuice force-pushed the panic-alert-t branch from 9e438a7 to 2f87fb5 Compare June 4, 2015 11:12

degasus reviewed Jun 4, 2015
View reviewed changes

JosJuice added 2 commits June 4, 2015 13:25

Use PanicAlertT instead of PanicAlert when appropriate

95a2abc

I tried to change messages that contained instructions for users, while avoiding messages that are so technical that most users wouldn't understand them even if they were in the right language.

Use PanicAlert instead of PanicAlertT when appropriate

030d467

It means less work for the translators... But I'm not too sure about this, because most of these have already been translated.

JosJuice force-pushed the panic-alert-t branch from 2f87fb5 to 030d467 Compare June 4, 2015 11:25

degasus added a commit that referenced this pull request Jun 4, 2015

Merge pull request #2213 from JosJuice/panic-alert-t

f5b0468

Use PanicAlertT/PanicAlert when appropriate

degasus merged commit f5b0468 into dolphin-emu:master Jun 4, 2015

JosJuice deleted the panic-alert-t branch June 4, 2015 11:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use PanicAlertT/PanicAlert when appropriate #2213

Use PanicAlertT/PanicAlert when appropriate #2213

JosJuice commented Mar 15, 2015

magumagu commented Mar 15, 2015

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

JosJuice commented Mar 15, 2015

JosJuice commented Jun 4, 2015

degasus commented Jun 4, 2015

JosJuice commented Jun 4, 2015

This comment was marked as off-topic.

degasus commented Jun 4, 2015

Use PanicAlertT/PanicAlert when appropriate #2213

Use PanicAlertT/PanicAlert when appropriate #2213

Conversation

JosJuice commented Mar 15, 2015

magumagu commented Mar 15, 2015

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

JosJuice commented Mar 15, 2015

JosJuice commented Jun 4, 2015

degasus commented Jun 4, 2015

JosJuice commented Jun 4, 2015

This comment was marked as off-topic.

degasus commented Jun 4, 2015