Re: [g-a-devel]gnopernicus - speech/libsrs ...

["Draghi Puterity (RO)" <mp baum ro>]

Hi Michael,

thank you for feedback. You are absolutely right, this module is ugly and it
was a quick and dirty shot, just to have something working with some speech
engines at the beginning, then Q&D adapted to the old gnome speech, then to
the newer. Basically it was done by more developers by cutting and pasting
from different example programs we found around. We want it to reengineer it
when we have a more evolved GS, which will be very soon (we are actually
waiting for speech markers). We will apply then your valuable feedback in
all areas.

Until then, it's OK to commit.

Best regards,
Draghi

---- Original Message -----
From: "Adi Dascal" <ad baum ro>
To: <mp baum ro>
Sent: Tuesday, December 10, 2002 8:03 PM
Subject: Fwd: [g-a-devel]gnopernicus - speech/libsrs ...


>
>
> ----------  Forwarded Message  ----------
>
> Subject: [g-a-devel]gnopernicus - speech/libsrs ...
> Date: 09 Dec 2002 15:04:39 +0000
> From: Michael Meeks <michael ximian com>
> To: Draghi Puterity <mp baum de>
> Cc: accessibility mailing list <gnome-accessibility-devel gnome org>
>
> Hi guys,
>
> I found a problem with this - whereby I had an old / duff speech driver
> around that wasn't loadable, and the exception poisoned your looping
> through the sound drivers. I had a poke at the code to fix this (fairly
> trivial) problem - and discovered a bit of a rat's nest.
>
> I've substantially re-written that file (spgs.c) I attach the patch;
> here are my comments - presumably they'll be useful / interesting to
> your hackers:
>
> * Use (void) for methods that do not take any arguments always.
>
> * Use g_new0 (int, 3) to allocate 3 int *s, don't use
>   (int *)g_malloc0 (sizeof(int) * 3) - it's more painful,
>   error prone & harder to read.
>
> * _Always_ check 'ev' - when you do a CORBA call - you _have_
>   to check the environment before using any return value. The
>   return value is undefined. Thus if I do:
>
>     obj = GNOME_Speech_getFoo (obj, &ev);
>
>   it is illegal to do:
>
>     if (!obj) g_error ("Error\n");
>
>   and then use 'obj' - since it may well be a completely random
>   value, which will SEGV the next CORBA method call. This is
>   _not_ a theoretical concern.
>
> * If you get a dirty exception in an exception environment - you
>   have to CORBA_exception_init it again, for it to be freed -
>   otherwise you potentially leak an exception and/or you seem
>   to get exceptions from perfectly valid method calls - since
>   you're just seeing the old one.
>
> * Don't use CORBA_Object_is_nil, just do obj == CORBA_OBJECT_NIL
>   it's more efficient and readable.
>
> * the '********' blocked comments are unusual - normally people
>   put the string 'FIXME' in where there is something to be fixed
>   later - that's easier to grep for than a char used in comments
>   and regexps.
>
> * Instead of nesting comparisons for exception handling, use the
>   powerful C control flow statements:
>
> Bad:
> for (i = 0; i < 10; i++) {
> if (!do_foo ()) {
> printf ("didn't do foo\n");
> } else {
> if (!do_baa ()) {
> printf ("didn't do baa\n");
> } else {
> ... bulk of statements ...
> }
> }
> }
>
>   that just blows the conceptual indentation stuff instead
>   either split the code out or use:
>
> Good:
> for (i = 0; i < 10; i++) {
> if (!do_foo ()) {
> printf ("didn't do foo\n");
> continue;
> }
> if (!do_baa ()) {
> printf ("didn't do baa\n");
> continue;
> }
> ... bulk of statements ...
> }
>
> * Use descriptive variable names; 'choice' is a terrible
>   variable name for a 'voice index', 'voice_idx' is a better
>   name.
>
> * DO NOT - over use global variables; this module is packed
>   with vile hacks wrt. global vars, you need to use an object
>   oriented approach. cf. the utter mess with last_driver.
>   cf. the total mess with the global 'speaker' that (only)
>   if you're lucky gets set by the lookup loop to something
>   valid.
>
> In short - extremely underwhelmed with this module. The API it provides
> is in every way inferior to the CORBA API it pointlessly wraps. There is
> a case for an API here - but it could be a single method with this
> signature:
>
> GNOME_Speech_Speaker gs_get_user_configured_speaker (void);
>
> And/OR it should be a GObject TTS_ENGINE that has a nice constructor /
> destructor and carries it's per-voice (/whatever) object data around
> cleanly. As for using 3 function pointers on the TTS_ENGINE to flag
> whether or not init succeeded - that's ghastly.
>
> In short - this badly looks as if it was debugged into being, instead
> of being designed, disappointing indeed.
>
> Regards,
>
> Michael.
>
> --
>  mmeeks gnu org  <><, Pseudo Engineer, itinerant idiot
>
> -------------------------------------------------------
>
>
>