2018-05-25 21:25 EDT


View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0001780FSSCPFREDpublic2012-06-25 02:47
ReporterKeldorKatarn 
Assigned ToGoober5000 
PrioritynormalSeveritytrivialReproducibilityalways
StatusclosedResolutionopen 
Product Version3.6.9 
Target VersionFixed in Version 
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.
Attached Files
  • ? file icon Borked.fs2 (4,688 bytes) 2009-05-01 07:17
  • diff file icon Mantis1780diff.diff (3,050 bytes) 2009-05-08 07:30 -
    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);
    
    diff file icon Mantis1780diff.diff (3,050 bytes) 2009-05-08 07:30 +

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

-Notes

~0010791

karajorma (administrator)

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.

~0010792

KeldorKatarn (reporter)

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.

~0010807

KeldorKatarn (reporter)

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.

~0010809

portej05 (reporter)

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)

~0010841

karajorma (administrator)

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

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

~0010845

portej05 (reporter)

Last edited: 2009-04-25 02: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.

~0010846

karajorma (administrator)

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

~0010863

portej05 (reporter)

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!

~0010864

karajorma (administrator)

Last edited: 2009-05-01 07: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.

~0010866

portej05 (reporter)

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....

~0010869

portej05 (reporter)

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.

~0011606

KeldorKatarn (reporter)

Was this patch ever committed to trunk?

~0012276

chief1983 (administrator)

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

~0013720

Goober5000 (administrator)

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.
+Notes

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