Re: [g-a-devel]Re: at-spi text API patch for 100944, as previously discussed.

[Michael Meeks <michael ximian com>]

Hi Bill,

On Wed, 2002-12-18 at 00:06, Bill Haneman wrote:
> FWIW the latest version should always be in bugzilla

	Ok - so, reading it through there are some good things here. Just a few
comments.

	* Add a CORBA_any to the Range thing - it cost little to marshal
	  an extra CORBA_tk_null or somesuch, but it could be most 
	  valuable in future.

	* It's nice to see the extra indirection on the 
	  AccessibleTextRange array, so we can expand that later - good.

	* Great to see the duplicate non-moved mouse event culling :-)

	OTOH, there are a couple of problems:

	* get_accessible_text_ranges_from_range_seq
		+ leaks the sequence
		+ doesn't copy the contents into array

		I suggest fixing that by (for now) by not allocating the
	'array', and pointing into the CORBA sequence itself. For now 
	you could make that work by NULL'ing seq->_buffer and then 
	CORBA_freeing that, then subsequently CORBA_freeing (ranges[0])
	which must point to the first element of that array.

	* _freeRanges is just bunk ;-) if you do the above it should be:

		CORBA_free (ranges [0]);
		g_free (ranges);  // of course you didn't alloc array

	* _spi_text_range_seq_from_gslist

		This method deep frees the structure in the list, and
	but memcopies a pointer to the string 'contents' into the 
	sequence resulting in a slew of FMRs. Also range->content is
	allocated with the g_malloc set of allocators, instead of the
	CORBA allocators.

		It would be entirely better here to just g_alloc the
	Text_Range struct in getBoundedRanges, CORBA_string_dup the
	contents in there, and build the g_slist. [ ensure you g_free
	the return value from the atk_..._get_text ].

		Then in seq_from_gslist, do a memcopy of the holding
	Text_Range struct into the sequence, and g_free it; then you
	have transfered ownership more cleanly.

	I havn't looked much deeper than that - those were the only problems I
could see immediately.

	HTH,

		Michael.

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