View Issue Details

IDProjectCategoryView StatusLast Update
0001780FSSCPFREDpublic2012-06-25 06:47
ReporterKeldorKatarn Assigned ToGoober5000  
PrioritynormalSeveritytrivialReproducibilityalways
Status closedResolutionopen 
Product Version3.6.9 
Summary0001780: FRED error message boxes have no size limit
DescriptionIn short, if an error occurs on mission load, saving or checking the mission with FRED's internal checking function, message boxes will describe the various errors, like missing events, unknown even names etc.
Problem is.. FRED tends to put the entire problematic event into this message box and if the even is large, the box doesn't fit to the user's screen and the actual problem description and the OK Cancel buttons are out of the user's reach.
This should be fixed somehow by limiting those boxes' size and adding a vertical scrollbar instead.
TagsNo tags attached.

Relationships

duplicate of 0002658 resolvedGoober5000 Error messages too long to fit on the screen 

Activities

karajorma

2009-04-01 21:39

administrator   ~0010791

I took a look at this but the messagebox class doesn't appear to have the ability to add scrollbars. So either we find a similar MFC class that does or we'd have to roll our own.

KeldorKatarn

2009-04-01 22:08

reporter   ~0010792

What about a simple standard form opened as a dialogue instead?
This error stuff is simply too complex for a standard message box. It's disigned for very short messages only.

A standard dialogue can be written which basially looks exactly like the message box but can have a maximum size, e.g.

This is simply a very annoying thing right now because if you have a very large SEXP node which is displayed as an error in such a box, you effectively have no chance to get that message box off the screen.

KeldorKatarn

2009-04-11 11:30

reporter   ~0010807

Or even easier.. clamp the error message to a maximum size. I mean the FREDer can go and look into the SEXP node once made aware of a problem. The message box doesn't need to display the entire gigantic SEXP node.
This would be easier to code and it would solve this issue right away.

Just clamp the thing to a size that makes sense, add a "too long to be displayed entirely..." at the end and be done with it.

portej05

2009-04-15 13:35

reporter   ~0010809

What about adding a text box to the message box
This would have the added advantage of copy and paste
I haven't actually looked at the code yet (SVN is being VERY slow atm)

karajorma

2009-04-24 17:56

administrator   ~0010841

You can't simply add a textbox as MessageBox is an actual MFC class.

I'll take a look at truncating the message though.

portej05

2009-04-25 06:04

reporter   ~0010845

Last edited: 2009-04-25 06:04

yeah - I wasn't sure if it was the windows message box or a custom one.
I've had a quick look through the code. There're a lot of message boxes that only have a short, but fixed message in them, and I'm sure there're a few which can have a (much!) longer message.
There're three ways I can think of of doing this:
1) Truncate the message - this may actually chop off important details, but I'm not sure
2) Define a LongMessageBox class (and dialog) and make the constructor similar to the call for MessageBox - I'm not sure if you can call DoModal in the constructor though
3) Define a LongMessageBox class globally and overload operator() - I know you can call DoModal there

The disadvantage is that we'd only be able to replace the MessageBox calls when someone reported the bug.

This is very similar to Karajormas first comment.

karajorma

2009-04-25 14:29

administrator   ~0010846

Well if you fancy coding that in, be my guest. I certainly think that 2 or 3 are the best choices, preferably 2.

portej05

2009-05-01 11:09

reporter   ~0010863

hmm, found a bug in the call to campaign_tree_wnd::error - buf is char[2048] while the passed buf3 argument from campaign_tree_wnd::fred_check_sexp is char[4096] - could ruin someones day at some point - but maybe this hasn't yet been hit.

Can you give a case of how to cause this oversize messagebox?
Never having actually seen it, I'm not sure what I'm looking for.

I'll replace the overlength ones with a dialog that has a scrollable textbox in it - just need to figure out how to cause it/them!

2009-05-01 11:17

 

Borked.fs2 (4,688 bytes)

karajorma

2009-05-01 11:19

administrator   ~0010864

Last edited: 2009-05-01 11:19

Uploaded a file. All the instances of Ship 1 are merely to bulk out the size of the event, they don't actually do anything. The real problem is simply that I deleted the "Alpha 1" from the is-destroyed-delay SEXP.

portej05

2009-05-01 17:31

reporter   ~0010866

Update:

This will be a fair bit harder than it looks :P
The function Warning (windebug.cpp) is called from all over the place, including in the game code, which doesn't have MFC initialised (and hence can't be used).
Changes here (the same changes need to be made to Error (windebug.cpp)) will affect the entire game.
I'm hesitant to use a define in stdafx.h for FRED because I don't know what it will stuff up.
Thinking....

2009-05-08 11:30

 

Mantis1780diff.diff (3,050 bytes)   
Index: code/fred2/fredview.cpp
===================================================================
--- code/fred2/fredview.cpp	(revision 5199)
+++ code/fred2/fredview.cpp	(working copy)
@@ -3850,9 +3850,11 @@
 	sprintf(buf2, "%s\n\nThis is an internal error.  Please let Jason\n"
 		"know about this so he can fix it.  Click cancel to debug.", buf);
 
-	if (MessageBox(buf2, "Internal Error", MB_OKCANCEL | MB_ICONEXCLAMATION) == IDCANCEL)
-		Int3();  // drop to debugger so the problem can be analyzed.
+	Error( LOCATION, buf2 );
 
+	//if (MessageBox(buf2, "Internal Error", MB_OKCANCEL | MB_ICONEXCLAMATION) == IDCANCEL)
+	//	Int3();  // drop to debugger so the problem can be analyzed.
+
 #else
 	MessageBox(buf, "Error", MB_OK | MB_ICONEXCLAMATION);
 #endif
Index: code/globalincs/windebug.cpp
===================================================================
--- code/globalincs/windebug.cpp	(revision 5199)
+++ code/globalincs/windebug.cpp	(working copy)
@@ -286,6 +286,7 @@
 
 int Global_warning_count = 0;
 int Global_error_count = 0;
+static const int maxMessageNewLines = 20;
 
 #ifdef _MSC_VER
 #include <crtdbg.h>
@@ -1201,7 +1202,28 @@
 	va_end(args);
 
 	filename = strrchr(filename, '\\')+1;
-	sprintf(AssertText2, "Error: %s\r\nFile: %s\r\nLine: %d\r\n", AssertText1, filename, line);
+
+	int newLineCount = 0;
+	/* On the occurrence of the maxMessageNewLines (th) newline, replace it with NULL */
+	int slen = strlen(AssertText1);
+	for ( int i = 0; i < slen; i++ )
+	{
+		if ( AssertText1[ i ] == '\n' )
+			newLineCount++;
+		
+		if ( newLineCount > maxMessageNewLines )
+		{
+			AssertText1[ i ] = NULL;
+			break;
+		}
+	}
+
+	if ( newLineCount > maxMessageNewLines )
+		sprintf(AssertText2, "Warning: %s\r\nFile: %s\r\nLine: %d\r\n[ Output has been truncated. See clipboard for full information ]", AssertText1, filename, line );
+	else
+		sprintf(AssertText2, "Warning: %s\r\nFile: %s\r\nLine: %d\r\n", AssertText1, filename, line );
+
+	//sprintf(AssertText2, "Error: %s\r\nFile: %s\r\nLine: %d\r\n", AssertText1, filename, line);
 	mprintf(("ERROR: %s\r\nFile: %s\r\nLine: %d\r\n", AssertText1, filename, line));
 
 	Messagebox_active = true;
@@ -1291,8 +1313,27 @@
 	}
 
 	filename = strrchr(filename, '\\')+1;
-	sprintf(AssertText2, "Warning: %s\r\nFile: %s\r\nLine: %d\r\n", AssertText1, filename, line );
 
+	int newLineCount = 0;
+	/* On the occurrence of the maxMessageNewLines (th) newline, replace it with NULL */
+	slen = strlen(AssertText1);
+	for ( int i = 0; i < slen; i++ )
+	{
+		if ( AssertText1[ i ] == '\n' )
+			newLineCount++;
+		
+		if ( newLineCount > maxMessageNewLines )
+		{
+			AssertText1[ i ] = NULL;
+			break;
+		}
+	}
+
+	if ( newLineCount > maxMessageNewLines )
+		sprintf(AssertText2, "Warning: %s\r\nFile: %s\r\nLine: %d\r\n[ Output has been truncated. See clipboard for full information ]", AssertText1, filename, line );
+	else
+		sprintf(AssertText2, "Warning: %s\r\nFile: %s\r\nLine: %d\r\n", AssertText1, filename, line );
+
 	Messagebox_active = true;
 
 	gr_activate(0);
Mantis1780diff.diff (3,050 bytes)   

portej05

2009-05-08 11:37

reporter   ~0010869

I've attached a patch for this.
It simply trims off newlines after the 20th and adds a 'truncated' message.
Because the code for this is in the /code directory, and is shared between the engine, MFC can't be used because MFC hasn't been initialised. There is also no resources file to put dialog resources into (for that library).
The call to CFREDView::internal_error is forwarded to 'Error' so that the same benefit is extended to (some of) the FRED stuff.
There are wayyy too many error message handlers spread around the place.

KeldorKatarn

2010-01-30 21:51

reporter   ~0011606

Was this patch ever committed to trunk?

chief1983

2010-07-28 03:00

administrator   ~0012276

Doesn't appear to have been. Gonna see if anyone thinks it's gonna hurt something.

Goober5000

2012-06-25 06:47

administrator   ~0013720

Derp... looks like this was a duplicate ticket. >.<

Oh well... my fix didn't use the same technique, but it has substantially the same effect. As a bonus, it fixes all handlers, not just the ones affected by portej05's patch.

Closing as duplicate.

Issue History

Date Modified Username Field Change
2008-09-27 12:27 KeldorKatarn New Issue
2009-04-01 21:39 karajorma Note Added: 0010791
2009-04-01 22:08 KeldorKatarn Note Added: 0010792
2009-04-11 11:30 KeldorKatarn Note Added: 0010807
2009-04-15 13:35 portej05 Note Added: 0010809
2009-04-24 17:56 karajorma Note Added: 0010841
2009-04-25 06:04 portej05 Note Added: 0010845
2009-04-25 06:04 portej05 Note Edited: 0010845
2009-04-25 14:29 karajorma Note Added: 0010846
2009-05-01 11:09 portej05 Note Added: 0010863
2009-05-01 11:17 karajorma File Added: Borked.fs2
2009-05-01 11:19 karajorma Note Added: 0010864
2009-05-01 11:19 karajorma Note Edited: 0010864
2009-05-01 17:31 portej05 Note Added: 0010866
2009-05-08 11:30 portej05 File Added: Mantis1780diff.diff
2009-05-08 11:37 portej05 Note Added: 0010869
2010-01-30 21:51 KeldorKatarn Note Added: 0011606
2010-07-28 03:00 chief1983 Note Added: 0012276
2012-06-25 06:44 Goober5000 Relationship added duplicate of 0002658
2012-06-25 06:47 Goober5000 Note Added: 0013720
2012-06-25 06:47 Goober5000 Assigned To => Goober5000
2012-06-25 06:47 Goober5000 Status new => closed