Opened 9 years ago

Closed 9 years ago

Last modified 10 months ago

#9160 closed patch

GSoC: Added unit test and unified error message display

Reported by: SF/sud03r Owned by: fingolfin
Priority: normal Component: GUI
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)

gsoc_scummvm.patch (6.5 KB) - added by SF/sud03r 9 years ago.
A patch file containg the above changes.
modified_scummvm.patch (6.4 KB) - added by SF/sud03r 9 years ago.
the updated patch with required changes
scummvm_final.patch (6.5 KB) - added by SF/sud03r 9 years ago.
final updated file with changes suggested by fingolfin
gui_changes.patch (8.6 KB) - added by SF/sud03r 9 years ago.
Separated error dialog functions to gui/error.h, added scummvm header,removed spaces/tabs.

Download all attachments as: .zip

Change History (14)

Changed 9 years ago by SF/sud03r

Attachment: gsoc_scummvm.patch added

A patch file containg the above changes.

comment:1 Changed 9 years ago by lordhoto

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.

Changed 9 years ago by SF/sud03r

Attachment: modified_scummvm.patch added

the updated patch with required changes

comment:2 Changed 9 years ago by SF/sud03r

hi lordHoto!

thanks for your review.
I have updated the patch as required. Hope it is okay now :).

comment:3 Changed 9 years ago by fingolfin

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!

Changed 9 years ago by SF/sud03r

Attachment: scummvm_final.patch added

final updated file with changes suggested by fingolfin

comment:4 Changed 9 years ago by SF/sud03r

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 Changed 9 years ago by fingolfin

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.

Changed 9 years ago by SF/sud03r

Attachment: gui_changes.patch added

Separated error dialog functions to gui/error.h, added scummvm header,removed spaces/tabs.

comment:6 Changed 9 years ago by SF/sud03r

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 Changed 9 years ago by lordhoto

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 Changed 9 years ago by fingolfin

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 Changed 9 years ago by fingolfin

Owner: set to fingolfin
Status: newclosed

comment:10 Changed 10 months ago by digitall

Component: GUI
Note: See TracTickets for help on using tickets.