GNOME Bugzilla – Bug 602963
Improve error reporting in evolution
Last modified: 2009-12-07 18:27:56 UTC
Currently there's no good way to pass errors back from functions in evolution. At the moment, when ever an error is encountered, the general strategy is to simply display a dialog to the user and abort the function. This is not particularly flexible since the innermost function can't (or shouldn't) always decide that the error is something that needs to be reported to the user or not. It would be much more flexible if we just passed an error back to the calling function so that they could decide what to do with it. Evolution uses a custom error system that involves a string tag (of the form "domain:error-id") and a variable number of arguments that can be formatted to generate a user-readable message. Because of this, it doesn't map very well to a GError structure. But I still wanted to emulate the GError idiom (e.g. passing an 'out' param to the function that will be set to a newly-allocated error object if there is an error). So, this patchset introduces a simple EError struct that can represent an evolution error message. It's possible that we would want to enhance this to be a proper gobject with refcounting, etc, but I didn't see the need for that yet. If we think it necessary, it should be easy enough to refactor. Beyond the new struct, I also changed some of the API in e-error.h. Previously, e_error_new() returned a GtkDialog* for displaying the error message. I changed this to return an EError struct (but changed the function signature so that the compiler should complain about any usage that expected the old behavior). I kept the old GtkDialog convenience functionality, but renamed them to match their behavior more closely (e_error_run_dialog(), e_error_new_dialog(), etc). In this series of patchsets, I adapted all of the evolution code to use the new names for the dialog convenience functions, but didn't change the overall design of the error reporting (e.g. the functions that fail still pop up dialogs, they don't yet report the error back to the caller via an out param). The one exception to this is with the classes in the filter/ directory. I changed the validate() vfuncs to return a EError via an out param rather than taking a GtkWidget* parent param to be used to show the error dialog.
Created attachment 148456 [details] [review] Refactor error reporting to separate error from dialogs Previously, Most things reported errors directly. This is evidenced by the fact that e_error_new() returns a GtkDialog*. This patch attempts to de-couple error-reporting from the UI. It introduces a simple stuct (EError) that describes the error which is returned much like a GError by passing it as an output parameter to a function. e_error_new() now returns a newly-allocated EError*, but the function signature has changed to no longer accept a parent GtkWidget, so the API change should be detected at compile time. I kept the convenience dialog functions, but renamed them slightly: - e_error_new() -> e_error_new_dialog() - e_error_run() -> e_error_run_dialog() Build is currently broken because nothing has been ported to use this new API yet.
Created attachment 148457 [details] [review] port filter/ classes over to new EError API
Created attachment 148458 [details] [review] port widgets/ to use new EError API
Created attachment 148459 [details] [review] port shell/ over to the new EError API
Created attachment 148460 [details] [review] port addressbook/ to use new EError API
Created attachment 148461 [details] [review] port composer/ to user new EError API
Created attachment 148462 [details] [review] port mail/ to use new EError API
Created attachment 148463 [details] [review] port calendar/ to the new EError API
Created attachment 148464 [details] [review] port plugins to use new EError API
Created attachment 148465 [details] [review] port modules/ to new EError API. This should be everything now.
Oh, I should also mention, if you'd prefer me to squash these all to a single patch, I can do that as well.
The other half of the problem will be what to do with the EError structs once they bubble up to the top-level caller. I've had some vague ideas about designing a GInterface for our main window and some of our editing windows to implement. The interface would be the final destination for EErrors. It would define some abstract methods for windows to implement -- like get_window() and get_info_bar() -- but the main public-facing function would be something like add_error(), which would take an EError and present it to the user in various possible ways. That would at least keep our "what to do with EErrors" policy centralized. There's all sorts of thorny little issues that I'm still trying to work out. How do we deal with errors piling up? Should some types of errors timeout and disappear automatically? How do we recognize duplicate errors so as not to bombard the user with the same message over and over. (e.g. Red Hat's IMAP server used to issue a warning message with -every- -single- -command- if you were at >90% of your quota. Evolution would pop up a new dialog for every one of them.) How do we know which errors to show in a dialog and which to show in an inline info bar? ...etc... One thing to keep in mind is -- at least historically -- EError isn't just about errors. It also handles informational messages and user prompts ("are you sure you want to proceed? yes/no"), so EAlert might actually be a better name for this framework. The patches all look pretty straight-forward. I'm okay with them going in now as a first step, or you can hold off and keep iterating on the problem.
Created attachment 148771 [details] [review] Rename EError to EAlert to match general use better The EError mechanism is used both for error dialogs as well as basic alerts or user prompts, so we should give it a more general name which matches this use. This patch also cleans up a few includes of e-alert.h (formerly e-error.h) that were not actually being used.
Created attachment 148772 [details] [review] Fix a bug in e_alert_newv() that was causing a crash
rebased and pushed to master on mbarnes advice.
see bug 603457 , exchange and mapi provider should also be updated
jonner, please update the patch status to committed :)