gEDA-dev: "error-reporting" branch work

[Peter Clifton]

On Wed, 2007-12-05 at 08:40 +0000, Peter TB Brett wrote:
> Hi folks,
> 
> I'd appreciate it if you could review the changes I've made on 
> my "error-reporting" branch:
> 
> http://repo.or.cz/w/geda-gaf/peter-b.git?a=shortlog;h=error-reporting

> Please try it out and see what you think, or better still read the patches and 
> tell me what I can do better.

Comments in line.. I've not use-tested this, but have done what I
consider to be a reasonably thorough visual review.

Hope this is helpful.. (See below)

I wrote these in the opposite order, so read from the bottom commit up.



> commit 808bb88fc166c658dde2eaebf857aa351db17557
> Author: Peter TB Brett <peter at peter-b.co.uk>
> 
>     libgeda: Clean g_assert usage in s_toplevel.c


We don't need to die horribly here, its probably not even necessary to
do any test. (Again, lets assume libgeda works, and glib works).


> commit 86aee629d7cbd531b3cc05ff77ce965c73bf531e
> Author: Peter TB Brett <peter at peter-b.co.uk>
> 
>     libgeda: define s_log_message -> g_message

Why not just change all usage of s_log_message into g_message?


> commit 9369844cdfe9b64e82b91b7f88c9fef9f8bb5421
> Author: Peter TB Brett <peter at peter-b.co.uk>
> 
>     gschem: Show an informative message dialog when loading a file fails.


Curiosity begs me to ask, does this break the use-case, where you start
at a terminal, and type:
gschem my_new_file.sch

(To start a new file and expect to work on it?) If the file doesn't
exist, and I specify it on the command line, I expect to be able to edit
the (blank, new) page, then save as that name. Some apps ask if you want
to make a new file. I prefer not to be bothered.

gedit foo
(doesn't bother me, but does prompt to save even if I made no changes.
Title bar is 'foo (~)')

gvim foo
(doesn't bother me, but displays '"foo" [New file]' in the status bar)

emacs foo
(doesn't bother me)

What do others expect here?


> commit 1f4bf37c402a0b897b0658f6cc237054e45706dd
> Author: Peter TB Brett <peter at peter-b.co.uk>
> 
>     Make libgeda's f_open() & f_open_flags use GError.

@@ -274,22 +280,19 @@ int f_open_flags(TOPLEVEL *toplevel, const gchar *filename,
....
+    g_assert (err == NULL || *err != NULL);

Here you're checking libgeda is working properly. We decided not to do
that? Simply set the contract.. if NULL is returned (no page data), and
a **err variable is passed, then *err will be set.


> commit 21e82934d02a72bb0db70164747ec4cd8ba5ca67
> Author: Peter TB Brett <peter at peter-b.co.uk>
> 
>     libgeda: Make o_read() use GError.

+  g_return_val_if_fail (err == NULL || *err == NULL, NULL);

This is perhaps magic enough to deserve a comment:
	/* Return NULL if error reporting is enabled and the return location
for an error isn't NULL. */

(In general, is this standard practise for GError code? Looking at the
GLib source code, I don't think so.. they just set the error if
necessary, otherwise leave it alone. Perhaps we should too?)

Also, whilst I was digging in GLib, I found:

gchar*              g_filename_display_basename         (const gchar *filename);

Returns the display basename for the particular filename, guaranteed to
be valid UTF-8. The display name might not be identical to the filename,
for instance there might be problems converting it to UTF-8, and some
files can be translated in the display.

And:
gchar*              g_filename_display_name             (const gchar *filename);

We probably should be using them, in case users have files with
non-ascii characters on a non UTF-8 encoded file-system.


I don't really like this:
+    /* Assert that error was set by the sub-function */
+    g_assert (err == NULL || *err != NULL);

g_warning() or similar, yes (if its possible to happen), but otherwise,
I thought we were going to try to avoid killing the process? If really
necessary, we could set the error ourselves here.


> commit e5999cad8a80a4ed3db6c57b077f7de74a9ee3ee
> Author: Peter TB Brett <peter at peter-b.co.uk>
> 
>     libgeda: Add f_has_active_autosave()

(Beware the raptors).

Use:
const gchar*        g_strerror                          (gint errnum);

Not strerror(). From the glib manual:
	"You should use this function in preference to strerror(), because it
returns a string in UTF-8 encoding, and since not all platforms support
the strerror() function."

You should use the _() macro for translatable strings, on the assumption
that these should eventually be translated (in libgeda), and that no-one
will remember where to check for them when I18N support is added.

For now, just define #define _(x) (x)  somewhere private to libgeda,
perhaps even just at the top of each file? (Or add the I18N support!)

+      /* Found an autosave backup. It's newer if error_stat is 0 */

This variable was deleted in the commit, so the comments need fixing.

A small stylistic point, that most gschem code uses:
} else {

Not:
}
else {

(I realise the function you're editing had this before though).


-- 
Peter Clifton

Electrical Engineering Division,
Engineering Department,
University of Cambridge,
9, JJ Thomson Avenue,
Cambridge
CB3 0FA

Tel: +44 (0)7729 980173 - (No signal in the lab!)