gEDA-dev: "error-reporting" branch work

[Peter TB Brett]

On Wednesday 05 December 2007 12:12:54 Peter Clifton wrote:
> On Wed, 2007-12-05 at 08:40 +0000, Peter TB Brett wrote:

> > 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).

Seems good, fixed.

> > 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?

Will do eventually -- this is less effort at least initially.

> > 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?

That sounds like a good idea -- I was trying to keep changes to gschem's 
actual behaviour minimal, so just made the minimum changes required to make 
there be *some* visual feedback as to why loading a file failed.  I'll see if 
I can make this distinction if I get round to it.

> > 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.

It's from the GError examples.  It seemed like a sensible thing to do at the 
time, but I'll ditch it.

> > 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?)

Once again, it was taken from the GError examples.  I've added the comment as 
suggested.

> 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.

Probably, yes.  Were you referring to 9d2d7b0042a75f0494?

> 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.

Removed entirely -- lets assume GLib works.

> > 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."

Fixed.

> 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!)

I was going to go through and do this through the whole of libgeda in a single 
patch (or probably series of patches) but didn't want to clutter this series 
with that.

> +      /* Found an autosave backup. It's newer if error_stat is 0 */
>
> This variable was deleted in the commit, so the comments need fixing.

Fixed.

> A small stylistic point, that most gschem code uses:
> } else {
>
> Not:
> }
> else {
>
> (I realise the function you're editing had this before though).

Fixed.

                              Peter


-- 
Peter Brett

Electronic Systems Engineer
Integral Informatics Ltd
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
Url : http://www.seul.org/pipermail/geda-dev/attachments/20071206/8b86aea8/attachment.pgp