*** Bug 100942 has been marked as a duplicate of this bug. ***

I note that in the bug report 100944 the proposal for
getRangeFromBounds seems to have changed from the initial proposal.

Do the paraameters for atk_text_get_range_within_bounds() also need to
change?

Should the function atk_text_get_range_extents() return -1  for x, y,
width and height if the offset values are invalid?

If so, should the same apply to atk_text_get_character_offset()?

Hi Padraig:

Yes, since there could be multiple contiguous ranges within a bounding
box, I think the Atk API should change too.  I am not sure what we
should return for our 'RangeList', or if perhaps there is a better way
for ATK to indicate one (via 'out params' or such, as the original
proposal did with the start and end offsets).  I don't mind leaving
that to your judgement.

I assume you mean ..._get_character_extents above.  I would return
width and height of zero for all 'invalid' cases; am not sure about -1
for x and y, since in some coordinate systems these would be valid and
meaningful offset values.  Personally I think it's reasonable to say
that the return values for these methods are "undefined" when invalid
offsets are given... though perhaps '0' should always give usable results.

Created attachment 13466 [details] [review]
Proposed new API

Bill,

I have attached a proposal for the new API for the get_bounded_ranges
function.

Can we get agreement on this?

Padraig:

this looks good - my only suggestion is that perhaps the
atk_text_get_bounded_ranges() call might, rather than returning offset
lists into int** pointers, return something more structured.

Most clients of this API will ultimately be interested not only in the
offset ranges but the geometry of the ranges within the outer bounding
box.  Thus after calling atk_text_get_bounded_ranges() they might call
atk_text_get_text_range (start, end) and atk_text_get_range_extents
(start, end) on each of the ranges.

This is likely to be somewhat redundant since in the process of
calculating the 'bounded ranges', the original function call will
probably have amassed most of the info necessary for getting the range
extents, etc.  So it might be more efficient to return not only the
bounding offsets for the returned ranges, but their bounds as well.

with that in mind, we could return an array of AtkTextRange structs:

struct AtkTextRange {
    AtkTextRectangle bounds;
    gint startOffset;
    gint endOffset;
};

or perhaps even include the text string itself:
    ...
    gint endOffset;
    char *content;
};

This means that the caller would have to do more g_free()'ing, but it
would eliminate the need for multiple subsequent client API calls in
many cases.

What do you think?

-Bill
 

Created attachment 13554 [details] [review]
Updated proposed API

Bill,

I have updated the proposed API and provided an implementation.

I would appreciate your comments on the attachment.

Hi Padraig:

API-wise the patch looks great (except for my concerns about an
apparent API change in atk_text_get_character_extents).  I like the
inclusion of text content and offsets in the TextRange structure which
as we discussed is likely to be very useful to ATs.

I see that atk_text_real_get_character_extents and
atk_text_get_character_extents don't check to see that the input
AtkTextRectangle param is non-null before poking it.  Seems as though
we should. 

The patch doesn't seem to actually connect the API to the vtable
entries, unless I am missing something... seems to be missing the
changes to class_init.

The one issues with the patch is that atk_text_get_character_extents
changes its API signature.  Since this method has been around in the
stable branch for awhile, I don't think we can do this even though
your new API is more elegant.  I would suggest making this new
text-char-extents API internal so that it can conveniently be used by
your range-bounds implementation, and wrap it in the old API for
public use.

Bill,

I am confused by your comments about atk_text_get_character_extents().
There is no change to the API. The only change is to ensure that the
width returned is not negative.

The function atk_text_real_get_range_extents() is called in
atk_text_get_range_extents() if no other implementation has been
provided.

you're right of course.  I can't see where I misread the patch
yesterday, but it seems I did.

Looks good as-is.  The use of a default implementation based on
whether the vtable entry is null seems unusual but perhaps it's
necessary for interface classes, not sure.  At any rate I don't think
it's harmful.

Patch has been applied to CVS HEAD, i.e. after branch for gnome-2-2.

I have discovered how to specify default implementations in the vtable
for interfaces. I will commit a change for that.