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

[Bill Haneman <bill haneman sun com>]

On Wed, 2002-12-18 at 10:39, Michael Meeks wrote:
> Hi Bill,
Thanks a lot Michael for the quick feedback.

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

Ouch! Quite right.  It does exactly what you suggest below (points 
into the CORBA sequence) but then as you point out, _freeRanges doesn't
do the corresponding counter-hack to make this work.  This approach
(even with the fixes you mention below) still seems like a hack to me. 
Would it be better to do a proper copy into the AccessibleTextRange
array, and free the CORBA structs?

> 		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

I don't think it's bunk at all, except that it doesn't align with the
current implementation of get_accessible_text_ranges_from_range_seq.
Note that the range list was intended to be a null-terminated array.  So
the brokenness is in the ranges_from_range_seq method, not here.
Of course your main implied point is correct, they do need to agree with
one another about the implementation details :-)

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

g_slist_free does a deep free ?!? The glib docs don't make it sound that
way to me.  Guess I should have read the sources instead :-(  

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

Yes, sounds good.  I am usually hesitant to g_alloc CORBA structs ;-)

> 
> 	I havn't looked much deeper than that - those were the only problems I
> could see immediately.
> 
> 	HTH,
> 
> 		Michael.
-- 
Bill Haneman <bill haneman sun com>