I’ve spotted a problem that the trunk has with loading scenarios from
the Paths of Glory module: There are some Stacks which have their Map
set to “Junk”, which doesn’t exist. This causes Stack.setState() to
throw an IllegalStateException (lines 462-8). This didn’t show up as
a problem before because the call to Stack.setState() was wrapped in
a try/catch block in Command.execute() which caught Throwable.
For contrast, BasicPiece.setState() just logs a warning and returns
when the Map can’t be found.
I thought that Command.execute() still tried to catch all kinds of Throwables, but only changed the way they were logged. We definitely should not throw runtime exceptions all the way up the stack on data errors like this.
It does catch all Throwables, but it rethrows Errors which are not
OutOfMemoryErrors, because those really are bugs. Everything else
is handled there by showing the BugDialog.
There are going to be lots of exceptions like this: anything that can be caused by loading a saved game into a modified module. Any error in which we fail to find something by name (such as a map or an image or a prototype) is a data error, and if we put up a bug dialog for every one of these, we’re going to be deluged.
What we should do is define a ModuleDataException class and throw it whenever we hit one of these cases. I’m sure there are lots of them. Then Command can catch ModuleDataException and handle it in a friendlier way. Probably just log it and continue. Maaaaybe a warning in the chat window, but definitely not a dialog that sends a bug to SourceForge.
Another thing: over the weekend, I hit a couple of bugs that popped up a continuous stream of bug dialog windows. I’d get a new one as soon as I cancelled the first one. It’s particularly bad when the error happens deep in the ImageOp classes during a paint() operation. The dialog definitely needs a “Don’t show this dialog again” checkbox.
I agree that we should distinguish between exceptions caused by bad data
and exceptions caused by other problems. (Do we want to call it
ModuleDataException? or just DataException? This kind of problem is more
likely to occur when loading a save or log than a module.)
One case was running out of memory. The OutOfMemoryError was handled fine, but because of where the error was thrown, some fields were left set to null, and I got a continuous stream of NullPointerExceptions in ImageOp.apply()
Of course we want to fix any problems that throw up a Bug dialog. But the chances of a case like this happening are greater than zero, and there’s no sense in annoying users.
Many of the places which are still marked ‘FIXME: review error message’
should probalby throw a DataException.
So should we save everything to an emergency file(s) and exit, after showing
a bug dialog, then? That’s probalby the only way to safely handle most bugs.
Not sure what you’re getting at. I just mean we should plan for the possibility of a bug happening multiple times in a session and let the user decide he doesn’t want to have the bug dialog pop up any more.
In more than half of the reports I get from the current bug dialog, the players say that they can close the dialog and continue with no ill effect. We can let the user decide whether an error is recoverable.
What I’m wondering here is if the benefits of letting the user continue
after a bug outweight the costs. The way we have it right now, VASSAL
will continue to fight on as best it can despite being (mortally?) wounded.
That’s nice from the point of view of a very advanced user (possibly only
for developers and some module designers), since if you know what’s going
on internally, you can judge the seriousness of the problem and have a
fair idea of how messed up things will be if you continue to use VASSAL
after a bug. I’m less sure that this will be judged as a feature by
regular users. I’m afraid that the average user will not be so careful
about distinguishing pre-bug and post-bug behavior, and that we’ll end
up with a reputation for having way more bugs than we actualy have due
to users seeing post-bug problems. It’s not clear to me at all that the
benefits outweight the costs here.
I don’t know whether this was a conscious choice on the part of developers,
but I can’t think of another application which lets you keep using it after
a potentially fatal error. So we don’t have a lot of company in making this
choice.
It puts me in mind of A.S. Johnston at Shiloh:
Dialog box: “You appear to be wounded. Would you like to have a
doctor look at that?”
Gen. Jonhston clicks “No” and continues to direct the battle… Go look
up what happend to A.S. Johnston if you don’t know how that worked out
for him.
Regarding the granularity of silencing errors, and the bug dialog in
particular: Whenever I see “Don’t show me this dialog again” as a choice,
I’m always apprehensive because I have no idea what it means. “This” and
“again” are terribly ambiguous. Is this dialog the dialog itself? Is it
this kind of message? Is it this particular message? Does again mean
during this session? ever? It’s annoying to me, since disambiugating this
would seem to burden the user with too much information or too many choices,
while leaving it without comment dramatically underspecifies what selecting
“Don’t show me this again” actually does.
Let’s take a concrete example: A trigger is set to increment the “hits” property by the value of the “strength” property, which turns out to have the value “foo” instead of a number. The choices are:
A) leave the property unchanged
B) abort the command
C) shut down the program
As a user, which would you prefer? Certainly not C). I even wonder whether B) is a good idea. You’re more likely to get into an inconsistent state if you execute only part of a Command. At least A) leaves you in a game state that’s known to be valid.
Think of the “Paths of Glory” example that you started this discussion off with. Were people complaining about bugs in those savefiles before? If we throw up a bug dialog for those games now, the perception is going to be that the new VASSAL version has a bug. It’s not improving the user’s experience nor is it boosting VASSAL’s reputation.
I think I’m convincing myself that we should trap these data errors as silently as possible. Even a dismissable dialog may be too obtrusive. I’d rather see something like web browsers have: a little “alert” icon that appears when there’s a problem loading a page, and the user can click on it for more details if they want.
We’re not talking about the same thing here. I’m talking about unexpected
exceptions which could leave us in an inconsistent state, not exceptions
which result from data parsing, from which we should be able to recover.
Cool. I’m going to take one more pass over the error-handling changes I made last night, then. I’ll see if I can fill in some make some comments around the FIXME’s I delete too
To be clear: I agree with you about problems caused by bad data. I’d still
like to have your thoughts on whether we should (save &) exit in the face
of other problems, and also what you think about “don’t show again”
granularity.
ErrorDialog already keeps a set containing keys to be ignored, and it’s
possible to call ErrorDialog.warning() and ErrorDialog.error() with
such keys. The keys are Objects, so can be as general as the class of
the exception or as specific as you like. In this case, what “don’t show
again” means is don’t show another dialog for anything matching this key
during this session.
For coding bugs like NullPointerExceptions, I guess you’re more likely to be left in a bad state. Still, as a user I consider a bug that makes the program shut down to be the worst kind. Worse than getting annoying dialogs, worse than buttons that do nothing and worse than things not behaving as I expect. The only thing shutting down is better than is corrupting my data, so I wouldn’t close the program automatically unless there’s a risk of ruining the player’s saved games or modules. I don’t think that this is something we generally have to worry about, though, since we don’t do any automatic overwriting.
I like having finer granularity around ignoring errors. I saw that code, but also saw that it wasn’t being used, so I wasn’t sure what state it was in. I’ll put some granularity around the data error handling, then.
I think we just don’t agree on this point. I don’t trust the user to be
discerning enough not to blame us if VASSAL eats their children when
they go blithely ahead after being shown the “Error: VASSAL might eat
your children” dialog.
No, it’s being used. The single most common (in terms of number of
occurences in the code) exception is an IOException thrown while
reading some file. I tried to show a ReadErrorDialog in all of those
cases. ReadErrorDialog is a wrapper around ErrorDialog, and provides
the option not to be warned about that particular file again.
Joel: Argues by analogy (Shiloh, “eat your children”)
Rodney: Argues by example
In the spirit of the above, let me offer another example:
VASSAL throws a NullPointer. Now a piece is missing from the Map. The player doesn’t notice. Of course we need to tell the player that something fishy happened and things might not be copacetic. But if we exit and save, what have we gained? We’ve saved the game with the piece missing! Exiting without saving is out of the question, and yet after we exit, the player’s just going to restart and load up the saved game, which puts him right where we were if we hadn’t exited in the first place. Or maybe he decides to revert to an earlier saved game, but he can do that regardless of whether we choose to shut down the program.
What we’ve gained is that the user hasn’t spent a bunch of time trying to
use VASSAL when it’s in an indeterminate state. Users will not distinguish
VASSAL acting badly after an exception (which might come from custom code
and not be our fault) from VASSAL being generally shoddy.
If the user reloads the emergency save, he at least has a chance of working
from a state which isn’t a known bad state. It’s not like continuing on
after an error like this is going to result the in the missing piece coming
back. Unless a miracle occurs, the problem will not get better either way;
but restarting with the emergency save won’t make it worse, while it could
get a lot worse without restarting VASSAL.
