After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 602963 - Improve error reporting in evolution
Improve error reporting in evolution
Status: RESOLVED FIXED
Product: evolution
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: Evolution Shell Maintainers Team
Evolution QA team
Depends on:
Blocks:
 
 
Reported: 2009-11-25 17:54 UTC by Jonathon Jongsma
Modified: 2009-12-07 18:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Refactor error reporting to separate error from dialogs (10.10 KB, patch)
2009-11-25 17:54 UTC, Jonathon Jongsma
committed Details | Review
port filter/ classes over to new EError API (13.65 KB, patch)
2009-11-25 17:54 UTC, Jonathon Jongsma
committed Details | Review
port widgets/ to use new EError API (1.79 KB, patch)
2009-11-25 17:54 UTC, Jonathon Jongsma
committed Details | Review
port shell/ over to the new EError API (3.98 KB, patch)
2009-11-25 17:54 UTC, Jonathon Jongsma
committed Details | Review
port addressbook/ to use new EError API (8.67 KB, patch)
2009-11-25 17:54 UTC, Jonathon Jongsma
committed Details | Review
port composer/ to user new EError API (3.94 KB, patch)
2009-11-25 17:54 UTC, Jonathon Jongsma
committed Details | Review
port mail/ to use new EError API (18.58 KB, patch)
2009-11-25 17:54 UTC, Jonathon Jongsma
committed Details | Review
port calendar/ to the new EError API (8.14 KB, patch)
2009-11-25 17:54 UTC, Jonathon Jongsma
committed Details | Review
port plugins to use new EError API (15.47 KB, patch)
2009-11-25 17:54 UTC, Jonathon Jongsma
committed Details | Review
port modules/ to new EError API. (8.97 KB, patch)
2009-11-25 17:54 UTC, Jonathon Jongsma
committed Details | Review
Rename EError to EAlert to match general use better (127.73 KB, patch)
2009-11-30 18:44 UTC, Jonathon Jongsma
committed Details | Review
Fix a bug in e_alert_newv() that was causing a crash (1.07 KB, patch)
2009-11-30 18:44 UTC, Jonathon Jongsma
committed Details | Review

Description Jonathon Jongsma 2009-11-25 17:54:06 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.
Comment 1 Jonathon Jongsma 2009-11-25 17:54:10 UTC
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.
Comment 2 Jonathon Jongsma 2009-11-25 17:54:13 UTC
Created attachment 148457 [details] [review]
port filter/ classes over to new EError API
Comment 3 Jonathon Jongsma 2009-11-25 17:54:15 UTC
Created attachment 148458 [details] [review]
port widgets/ to use new EError API
Comment 4 Jonathon Jongsma 2009-11-25 17:54:18 UTC
Created attachment 148459 [details] [review]
port shell/ over to the new EError API
Comment 5 Jonathon Jongsma 2009-11-25 17:54:20 UTC
Created attachment 148460 [details] [review]
port addressbook/ to use new EError API
Comment 6 Jonathon Jongsma 2009-11-25 17:54:24 UTC
Created attachment 148461 [details] [review]
port composer/ to user new EError API
Comment 7 Jonathon Jongsma 2009-11-25 17:54:27 UTC
Created attachment 148462 [details] [review]
port mail/ to use new EError API
Comment 8 Jonathon Jongsma 2009-11-25 17:54:30 UTC
Created attachment 148463 [details] [review]
port calendar/ to the new EError API
Comment 9 Jonathon Jongsma 2009-11-25 17:54:33 UTC
Created attachment 148464 [details] [review]
port plugins to use new EError API
Comment 10 Jonathon Jongsma 2009-11-25 17:54:36 UTC
Created attachment 148465 [details] [review]
port modules/ to new EError API.

This should be everything now.
Comment 11 Jonathon Jongsma 2009-11-25 18:01:45 UTC
Oh, I should also mention, if you'd prefer me to squash these all to a single patch, I can do that as well.
Comment 12 Matthew Barnes 2009-11-26 00:57:04 UTC
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.
Comment 13 Jonathon Jongsma 2009-11-30 18:44:27 UTC
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.
Comment 14 Jonathon Jongsma 2009-11-30 18:44:30 UTC
Created attachment 148772 [details] [review]
Fix a bug in e_alert_newv() that was causing a crash
Comment 15 Jonathon Jongsma 2009-11-30 20:15:08 UTC
rebased and pushed to master on mbarnes advice.
Comment 16 Akhil Laddha 2009-12-01 10:35:01 UTC
see bug 603457 , exchange and mapi provider should also be updated
Comment 17 Chenthill P 2009-12-02 15:17:28 UTC
jonner, please update the patch status to committed :)