[g-a-devel]gnopernicus - magnifier

[Michael Meeks <michael ximian com>]

Hi there,

	So I started to look at the magnifier stuff - this was (sadly) crashing
for me the other day, that's a bug shame especially since the result
looks so nice.

	It turns out the crash I'm seeing must be in gnome-mag itself, but I
saw a few things in there that curled my hair:

	magnifier/libmag/mag_ctrl.c:

	* Only duplicate what you know will be released, only allocate
	  what you know will be freed etc. etc.:

       region = CORBA_Object_duplicate ( ( (CORBA_Object *)
		(regions->_buffer) )[zoom_region],
				         &ev);

	  why is there a duplicate there ? seems extremely odd to me.

	  [ Interestingly CORBA_Object_duplicate / release are 2
	    methods that don't in fact need to pass a non-NULL ev, since
	    they are purely local methods, and can never fail ].

	* Use the helpful helpers; the whole of the 
	  magnifier_set_zoomer_properties method is somewhat strange:

   	     Bonobo_PropertyBag_setValue (properties,
    				    "is-managed",
				    bonobo_arg_new_from
 					(BONOBO_ARG_BOOLEAN,
					 &bool_val),  &ev);

	  1st tip - whenever you see 'new' think allocate, and see the 
	  above point - this routine leaks great chunks of memory.

	  2ndly - we thought that people would have pain here, so we 
	  already made things easier, use either:

	  bonobo_pbclient_set_boolean (properties, "is-managed",
				       bool_val, NULL);

	  (bonobo-property-bag-client.h) - the typesafe version,
	  which lets you ignore errors with a NULL opt_ev.

	OR:

	    bonobo_pbclient_set (
		properties,
		"is-managed", TC_CORBA_boolean,
		(CORBA_boolean) bool_val,

		"mag-factor-x", TC_CORBA_float,
		(CORBA_float) 2.0,

		NULL);

	  which lets you set multiple properties in a var-arg, (less
	  typesafe but) more convenient fashion.

	* Finally the moral has to be that if you don't know how best to do
something - ask, and I'll try and help :-) best place is 'gcl' -
gnome-components-list gnome org - subscribe :-)

	OH - and (to my horror) I discovered that the global variables in
speech/ are not even module local - so you're exporting an 'env' symbol
globally [!] - plus loads of others: all of these need 'static'
prepending:

CORBA_Environment		env;
Bonobo_ServerInfoList           *servers;
int				last_driver = 0;
CORBA_string driver_name, driver_version;
CORBA_string synth_name, synth_version;
GNOME_Speech_Speaker speaker;
GNOME_Speech_VoiceInfoList *voices;
GNOME_Speech_SynthesisDriver curr_driver;
GNOME_Speech_SynthesisDriver *drivers;
gint i;

	'gint i' !!! grief ;-) [ Excellent, now I don't have to declare that
variable in any of my methods that want to use a loop !!???!?!?!?!? -
Rulezz ].

	It seems that some form of code-review hierarchy would be useful
filtering commits to gnopernicus and/or public posting of all patches -
preferably here, so that the code quality heads in a positive direction.

	Regards,

		Michael.

-- 
 mmeeks gnu org  <><, Pseudo Engineer, itinerant idiot