View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0001780 | FSSCP | FRED | public | 2008-09-27 12:27 | 2012-06-25 06:47 |
Reporter | KeldorKatarn | Assigned To | Goober5000 | ||
Priority | normal | Severity | trivial | Reproducibility | always |
Status | closed | Resolution | open | ||
Product Version | 3.6.9 | ||||
Summary | 0001780: FRED error message boxes have no size limit | ||||
Description | In 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. | ||||
Tags | No tags attached. | ||||
duplicate of | 0002658 | resolved | Goober5000 | Error messages too long to fit on the screen |
|
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. |
|
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. |
|
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. |
|
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) |
|
You can't simply add a textbox as MessageBox is an actual MFC class. I'll take a look at truncating the message though. |
|
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. |
|
Well if you fancy coding that in, be my guest. I certainly think that 2 or 3 are the best choices, preferably 2. |
|
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
|
|
|
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. |
|
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); |
|
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. |
|
Was this patch ever committed to trunk? |
|
Doesn't appear to have been. Gonna see if anyone thinks it's gonna hurt something. |
|
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. |
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 |