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

[Michael Meeks <michael ximian com>]

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

Index: spgs.c
===================================================================
RCS file: /cvs/gnome/gnopernicus/speech/libsrs/spgs.c,v
retrieving revision 1.12
diff -u -p -u -r1.12 spgs.c
--- spgs.c	11 Nov 2002 19:15:59 -0000	1.12
+++ spgs.c	9 Dec 2002 14:52:16 -0000
@@ -33,7 +33,6 @@
 CORBA_Environment		env;
 Bonobo_ServerInfoList           *servers;
 int				last_driver = 0;
-gint choice = 0;
 CORBA_string driver_name, driver_version;
 CORBA_string synth_name, synth_version;
 GNOME_Speech_Speaker speaker;
@@ -93,139 +92,100 @@ void gs_shut_up()
 }
 
 
-int gs_init (TTS_ENGINE *engine)
+int
+gs_init (TTS_ENGINE *engine)
 {
-    int i;
+	int i;
+	CORBA_Environment ev;
+
+	CORBA_exception_init (&ev);
 
-	CORBA_exception_init (&env);
 	if (!bonobo_init (NULL, NULL) )
-	{
-    	    fprintf (stderr, "Can't initialize Bonobo.\n");
-	}
+		fprintf (stderr, "Can't initialize Bonobo.\n");
 
-	servers = bonobo_activation_query ("repo_ids.has ('IDL:GNOME/Speech/SynthesisDriver:0.2')",
-				    	    NULL,
-					    &env);
-	if (env._major != CORBA_NO_EXCEPTION)
-	{
-    	    fprintf (stderr, "Couldn't activate GNOME-SPEECH server.\n");
-	}
+	servers = bonobo_activation_query (
+		"repo_ids.has ('IDL:GNOME/Speech/SynthesisDriver:0.2')",
+		NULL, &ev);
+
+	if (BONOBO_EX (&ev))
+		fprintf (stderr, "Couldn't activate GNOME-SPEECH server.\n");
 
 	if (!servers)
-	{
-    	    fprintf (stderr, "No GNOME-SPEECH drivers found.\n");
-	}
+		fprintf (stderr, "No GNOME-SPEECH drivers found.\n");
 
-	drivers = (GNOME_Speech_SynthesisDriver*) g_malloc0(servers->_length * sizeof(GNOME_Speech_SynthesisDriver));
+	drivers = g_new0 (GNOME_Speech_SynthesisDriver, servers->_length);
 
-	for (i = 0; i < servers->_length; i++)
-	{
-    	    CORBA_Object obj;
-    	    GNOME_Speech_SynthesisDriver driver;
-    	    Bonobo_ServerInfo *info;
-
-    	    info = &servers->_buffer[i];
-
-            printf ("Atempting to activate %s.\n", info->iid);
-
-    	    obj = bonobo_activation_activate_from_id ((const Bonobo_ActivationID)info->iid,
-						      0,
-						      NULL,
-						      &env);
-    	    if (CORBA_Object_is_nil (obj, &env) )
-	    {
-	  	fprintf (stderr, "Activation of server %s failed.\n", info->iid);
-	    }
-    	    else
-	    {
+	for (i = 0; i < servers->_length; i++) {
+		int voice_idx;
+		CORBA_Object obj;
+		Bonobo_ServerInfo *info;
+		GNOME_Speech_SynthesisDriver driver;
 
-		fprintf (stderr, "Activation of server %s succeeded.\n", info->iid);
+		info = &servers->_buffer[i];
 
-		driver = (GNOME_Speech_SynthesisDriver) obj;
+		printf ("Atempting to activate %s.\n", info->iid);
 
-		/*if (GNOME_Speech_SynthesisDriver_driverInit (driver, &env))*/
-		if (driver)/*an information to verify if the driver init was ok*/
-		{
-	    		drivers[last_driver++] = driver;
+		CORBA_exception_init (&ev);
 
-		driver_name = GNOME_Speech_SynthesisDriver__get_driverName (driver, &env);
-		if (!BONOBO_EX (&env))
-		{
-    		    printf ("Driver name: %s\n", driver_name);
-    		    CORBA_free (driver_name);
-		}
-/*This is not so important and it's time consuming I think*/
-/*
-		driver_version = GNOME_Speech_SynthesisDriver__get_driverVersion (driver, &env);
-		if (!BONOBO_EX (&env))
-		{
-    		    printf ("Driver version: %s\n", driver_version);
-    		    CORBA_free (driver_version);
-		}
-		synth_name = GNOME_Speech_SynthesisDriver__get_synthesizerName (driver, &env);
-		if (!BONOBO_EX (&env))
-		{
-    		    printf ("Synthesizer name: %s\n", synth_name);
-    		    CORBA_free (synth_name);
-		}
-		synth_version = GNOME_Speech_SynthesisDriver__get_synthesizerVersion (driver, &env);
-		if (!BONOBO_EX (&env))
-		{
-    		    printf ("Synthesizer Version: %s\n", synth_version);
-    		    CORBA_free (synth_version);
+		obj = bonobo_activation_activate_from_id (
+			info->iid, 0, NULL, &ev);
+
+		if (BONOBO_EX (&ev) || obj == CORBA_OBJECT_NIL) {
+			fprintf (stderr, "Activation of server %s failed.\n", info->iid);
+			continue;
 		}
-*/		
 
-		/* Display list of voices */
+		fprintf (stderr, "Activation of server %s succeeded.\n", info->iid);
 
-		voices = GNOME_Speech_SynthesisDriver_getAllVoices (driver, &env);
+		driver = (GNOME_Speech_SynthesisDriver) obj;
 
-		if (!voices) 
-		{
-		    printf ("No voices!!!.\n");
-		    /************RETURN SMTH/EXIT***********/
-		    exit(0);
+		drivers [last_driver++] = driver;
+
+		driver_name = GNOME_Speech_SynthesisDriver__get_driverName (driver, &ev);
+		if (!BONOBO_EX (&ev)) {
+			printf ("Driver name: %s\n", driver_name);
+			CORBA_free (driver_name);
+		} else
+			CORBA_exception_free (&ev);
+
+		/* Display list of voices */
+		voices = GNOME_Speech_SynthesisDriver_getAllVoices (driver, &ev);
+		if (BONOBO_EX (&ev) || !voices || !voices->_length) {
+			printf ("No voices!!!.\n");
+			continue;
 		}
+
 		/*this information will be needed for the GUI to display it to user,
 		  so the user can chose a peculiar voice. Maybe some gconf keys created for
 		  them ?!?*/
 		/*  fprintf (stderr,"\n Name of the voices : \n");
-		for (i = 0; i < voices->_length; i++) 
-		{
-		GNOME_Speech_Speaker_setParameterValue (speaker,
-				    			"voice",
-				    			i+1,
-				    			&env);
-		fprintf (stderr, "%d. %s\n", i+1, voices->_buffer[i].name);
-		}
+		    for (i = 0; i < voices->_length; i++) {
+		    GNOME_Speech_Speaker_setParameterValue (speaker,
+		    "voice", i, &ev);
+		    fprintf (stderr, "%d. %s\n", i+1, voices->_buffer[i].name);
+		    }
 		*/
 
-		/*select a voice, a default should be required here 
-		choice = 1 ;
-		*/
-		/*to be sure that the chosen voice is approriate, verify*/
-		if (choice < 1 || choice > voices->_length) 
-		    choice = 1;
-  
-		speaker = GNOME_Speech_SynthesisDriver_createSpeaker (driver,
-							&voices->_buffer[choice-1], 
-							&env);
-		/*  GNOME_Speech_Speaker_say (speaker, "Welcome", &env);*/
-
-
-		}
-		else
-	 		 printf ("Driver initialization failed :-(.\n");
-
-	    }
+		/* FIXME: select a voice, a default should be required here */
+		voice_idx = 0;
+		if (voice_idx < 0 || voice_idx >= voices->_length)
+			continue;
+		
+		speaker = GNOME_Speech_SynthesisDriver_createSpeaker (
+			driver, &voices->_buffer[voice_idx], &ev);
+		/*  GNOME_Speech_Speaker_say (speaker, "Welcome", &ev);*/
+		
+		if (BONOBO_EX (&ev) || !speaker)
+			printf ("Driver initialization failed :-(.\n");
 	}
-        if (last_driver)
-	{
-   	/* fill the engine structure with the actual functions */
+
+        if (last_driver) {
+		/* fill the engine structure with the actual functions */
    		engine->ShutUp    = gs_shut_up;
 		engine->Speak     = gs_speak;
 		engine->Terminate = gs_terminate;
 	}
+
 	return last_driver;
 }
 
@@ -235,5 +195,5 @@ void gs_terminate ()
 /*	CORBA_free(voices); ????*/
 /*	CORBA_free(drivers);*/
 	CORBA_free(servers);
-	
 }
+