#9160 closed patch
GSoC: Added unit test and unified error message display
| Reported by: | SF/sud03r | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | GUI |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
for unit test testing Common::StringTokenizer Class: Added test/common/tokenizer.h
unified display of error messages to users:
Modified common/error.h (Added definition of required helper functions) Added common/error.cpp (Implemented the error table and required helper functions) Modified common/module.mk (to accomodate error.o) Modified base/main.cpp (replaced the switch-cases by the written helper function, modified error handling)
Please, let me know of ay modifications that may be required,
Thanks
Ticket imported from: #2982224. Ticket imported from: patches/1265.
Attachments (4)
Change History (14)
by , 14 years ago
| Attachment: | gsoc_scummvm.patch added |
|---|
comment:1 by , 14 years ago
Hi,
first of all thank you very much for your patch. Just had time to give the patch a quick look, some thoughts:
the Tokenizer tests seems fine to me. You might want to use our ARRAYSIZE from common/util.h instead of the custom "sizeof(tokenArray) / sizeof(Common::String)" use though. (You might also want to replace that in your displayErrorMessage implementation)
About the error message unification:
I noticed that the pointer definitions do not match our conventions (that's all I noticed via a quick look, there might be more). You might want to read up on that topic over here: http://wiki.scummvm.org/index.php/Code_Formatting_Conventions
Next the doxygen comments shouldn't use a ":" in the @param description for example. You might want to also see how our doxygen comments are formatted by checking common/system.h. (Also there is an @return annotation missing for the "errToString" comment).
While "errToString" might be a fine name, I would suggest to use "errorToString".
I also do not get why you want to have an default argument for "displayErrorDialog"? Actually it seems to me that this should only be used along with an error code and that rather the extra description is optional, thus you might want to remove (or justify) the default parameter and swap the parameters.
Also if the "extra" string does not end with a trailing space or the like, it seems to me that the two strings are combined without any separation. You might want to fix that.
Next the kCreatingFileFailed description should probably read "Can not create file" instead of "can't", that looks like rather bad style for written English.
Also I am not sure why you add an displayErrorMessage call right beneath the comment, which talks about that there is no output initialized, thus displaying a dialog box would not work.
comment:2 by , 14 years ago
hi lordHoto!
thanks for your review. I have updated the patch as required. Hope it is okay now :).
comment:3 by , 14 years ago
The following are all minor points, but I thought I'd point them out nevertheless:
* Please use "Cannot create file" instead of "Can not create file" -- a very subtle difference in the language, actually
* In common/module.mk, please insert error.o in the alphabetical correct position
* in base/main.cpp you added a comment at the top which asks whether a separate dialog is still needed here. That's fine, but I'd mark it with a "TODO: " or "FIXME:" prefix. Also, right above that, there is a comment saying "TODO: Show an error dialog or so?" -- it seems to me your patch resolve this and hence it should be removed, no?
Good work so far!
by , 14 years ago
| Attachment: | scummvm_final.patch added |
|---|
final updated file with changes suggested by fingolfin
comment:4 by , 14 years ago
Hi Max,
thanks for your review! I have updated the patch to reflect the suggested changes.
Hope this one makes its way to the trunk :).
comment:5 by , 14 years ago
Much better. I committed the tokenizer test case from this patch (and also some tweaks atop of it)
As for the error stuff, some more remarks * added missing GPL header to common/error.cpp * removed spaces / tabs at line ends * the displayErrorDialog in common/error.cpp mean that common/ now suddenly depends on gui/. This is bad for various reasons. For example, it means that we can't write test cases using the error stuff because it might pull in the whole GUI code as a dependency. Also, the common/ code is meant to not rely on anything except for stuff in backends/ (and even that is to be avoided as much as possible). I suggest this workaround: Keep errorToString and its table in common/error.cpp. But move displayErrorDialog to a different dialog. I propose gui/error.h and gui/error.cpp. This file might later grow to contain more advanced error handling dialogs, too.
Sorry for not noticing the latter point earlier, but I didn't look at the patch closely enough / didn't think enough about it before, it seems.
by , 14 years ago
| Attachment: | gui_changes.patch added |
|---|
Separated error dialog functions to gui/error.h, added scummvm header,removed spaces/tabs.
comment:6 by , 14 years ago
Hi Max!,
thanks for the review.
yeah, I too was thinking of the same. As the error dialogs were basically related to GUI, so they belong more to the GUI module. I have updated the patch to reflect the changes. added the GPL header, but i think the fields $URL and $ID in the header, would be added automatically when the code be committed, so didn't added those.
Thanks!
comment:7 by , 14 years ago
Actually you still need to put $URL$ and $ID$ in the files, they will only be automatically expanded by SVN in your local copy.
comment:8 by , 14 years ago
Actually, the content of the $Id$ and $URL$ keywords are indeed update by Subversion, but they have to be present for it to do that. So one just has to insert empty versions into the legal headers, like so: * $URL$ * $Id$ Anyway, I made the change and committed your patch. Thanks!
comment:9 by , 14 years ago
| Owner: | set to |
|---|---|
| Status: | new → closed |
comment:10 by , 6 years ago
| Component: | → GUI |
|---|
A patch file containg the above changes.