[Webkit-unassigned] [Bug 25415] [GTK][ATK] Please implement support for get_text_at_offset
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat May 23 00:40:16 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=25415
------- Comment #19 from xan.lopez at gmail.com 2009-05-23 00:40 PDT -------
(In reply to comment #17)
> (From update of attachment 30465 [review])
> > -static gchar* webkit_accessible_text_get_text_after_offset(AtkText* text, gint offset, AtkTextBoundary boundary_type, gint* start_offset, gint* end_offset)
> > +typedef enum {
> > + AfterOffset,
> > + AtOffset,
> > + BeforeOffset
> > +} GetTextFunctionType;
>
> enum GetTextFunctionType would do.
Ah, right, C habits :)
>
> > +
> > +static bool findCharacterAttribute(isCharacterAttribute predicateFunction, PangoLogAttr* attributes, Direction direction, int startOffset, int attrsLength, int* resultOffset)
> > +{
>
> attrsLength should be unsigned. can starOffset be negative here?
Right, I think both can be unsigned.
>
> > + int i;
>
> You can move this in for loop below.
Right.
>
> > + advanceFunc advanceFunc = direction == DirectionForward ? increaseInt : decreaseInt;
> > +
> > + *resultOffset = -1;
> > +
> > + for (i = startOffset; i >= 0 && i < attrsLength; i = advanceFunc(i)) {
>
> int i = startOffset;
Right.
>
> > + if (predicateFunction(attributes+i)) {
>
> there should be spaces in attributes+1
Right.
>
> > +static bool findCharacterAttributeSkip(isCharacterAttribute predicateFunction, unsigned skip, PangoLogAttr* attributes, Direction direction, int startOffset, int attrsLength, int* resultOffset)
> > +{
> > + int tmpOffset;
> > + bool retValue;
> > +
> > + retValue = findCharacterAttribute(predicateFunction, attributes, direction, startOffset, attrsLength, &tmpOffset);
>
> Better to assign value in the declaration.
Right.
>
> > +static gchar* getTextHelper(GetTextFunctionType getTextFunctionType, AtkText* textObject, gint offset, AtkTextBoundary boundaryType, gint* startOffset, gint* endOffset)
> > +{
> > + AccessibilityObject* coreObject = core(textObject);
> > + String text;
> > +
> > + *startOffset = *endOffset = -1;
> > +
> > + if (coreObject->isTextControl())
> > + text = coreObject->text();
> > + else
> > + text = coreObject->textUnderElement();
> > +
> > + char* cText = g_strdup(text.utf8().data());
> > + glong textLength = g_utf8_strlen(cText, -1);
>
> -1 means cText is nul-terminated. Is this going to be the case? Can't we just
> use strlen here?
Yes, it's NULL terminated. strlen() is wrong, that gives the number of bytes, I
need the number of characters (and anyway strlen would crash just the same if
it were not NULL terminated? Or you meant that as a different issue).
>
> > + if (boundaryType == ATK_TEXT_BOUNDARY_CHAR) {
> > + int effectiveOffset;
> > +
> > + switch(getTextFunctionType) {
>
> space between switch and (.
Right.
>
> > + case AfterOffset:
> > + effectiveOffset = offset + 1;
> > + break;
> > + case BeforeOffset:
> > + effectiveOffset = offset - 1;
> > + break;
> > + case AtOffset:
> > + effectiveOffset = offset;
> > + break;
> > + default:
> > + g_assert_not_reached();
> > + }
>
> Is g_asset_not_reached the correct behaviour here? Can't we set effectiveOffset
> to NULL in this case?
Since there are only three functions calling this, it's in theory impossible
that the default would be reached here, so I think an assert is OK (IMHO
asserts should be used for 'this is in theory impossible, or should never
happen').
>
> > + *startOffset = effectiveOffset;
> > + *endOffset = effectiveOffset + 1;
>
> what should be the behaviour if effectiveOffset is NULL? should we set
> start/end offsets to 0?
What you mean with NULL exactly? effectiveOffset is a number, not a pointer.
>
> > + } else {
> > + PangoLogAttr* attrs = g_new(PangoLogAttr, textLength+1);
>
> spaces in textLength+1
Right.
>
> > + PangoLanguage* language = pango_language_get_default();
> > + pango_get_log_attrs(cText, -1, -1, language, attrs, textLength+1);
>
> ditto.
Right.
>
> > +
> > + isCharacterAttribute predicate;
> > +
> > + if (boundaryType == ATK_TEXT_BOUNDARY_WORD_START)
> > + predicate = isWordStart;
> > + else if (boundaryType == ATK_TEXT_BOUNDARY_WORD_END)
> > + predicate = isWordEnd;
> > + else if (boundaryType == ATK_TEXT_BOUNDARY_SENTENCE_START)
> > + predicate = isSentenceStart;
> > + else if (boundaryType == ATK_TEXT_BOUNDARY_SENTENCE_END)
> > + predicate = isSentenceEnd;
> > + else
> > + g_assert_not_reached();
>
> what should be the default behaviour here?
OK, this is wrong now, because we miss the LINE boundary implementations, and
this will crash if someone tries to use them. I think I should just set
*startOffset and *endOffset to -1 beforehand and here do a goto to the end of
the function (as a temporary solution until everything is implemented). When
LINE boundary is done I think an assert would be again correct, since that last
option should never be reached.
>
> > +
> > + switch(boundaryType) {
>
> space between switch and (.
Right.
>
> > + case ATK_TEXT_BOUNDARY_WORD_START:
> > + case ATK_TEXT_BOUNDARY_SENTENCE_START:
> > + if (getTextFunctionType == AfterOffset) {
> > + // Take the item after the current one in any case
> > + findCharacterAttribute(predicate, attrs, DirectionForward, offset+1, textLength + 1, startOffset);
> > + findCharacterAttributeSkip(predicate, 1, attrs, DirectionForward, offset+1, textLength + 1, endOffset);
>
> Spaces in offset+1. Also, what's the difference between the two?
Sorry, between what two?
>
> > + } else if (getTextFunctionType == AtOffset) {
> > + // Take the item at point if the offset is in an item or
> > + // the item before otherwise
> > + findCharacterAttribute(predicate, attrs, DirectionBackwards, offset, textLength + 1, startOffset);
> > + if (!findCharacterAttribute(predicate, attrs, DirectionForward, offset+1, textLength + 1, endOffset)) {
> > + findCharacterAttribute(oppositePredicate(predicate), attrs, DirectionForward, offset+1, textLength + 1, endOffset);
>
> spaces in offset+1.
Right.
>
> > + findCharacterAttribute(predicate, attrs, DirectionForward, offset+1, textLength + 1, endOffset);
>
> ditto.
>
> > + } else {
> > + // Take the item before the point in any case
> > + if (!findCharacterAttributeSkip(predicate, 1, attrs, DirectionBackwards, offset, textLength + 1, startOffset)) {
> > + int tmpOffset;
> > + // No match before offset, take the first opposite match at or before the offset
> > + findCharacterAttribute(predicate, attrs, DirectionBackwards, offset, textLength + 1, &tmpOffset);
> > + findCharacterAttribute(oppositePredicate(predicate), attrs, DirectionBackwards, tmpOffset-1, textLength + 1, startOffset);
>
> spaces in tmpOffset-1.
>
> > + default:
> > + g_assert_not_reached();
>
> is asserting the right behaviour here?
Since we will exit early for unknown boundaries, I think it is.
>
> > + char* start = g_utf8_offset_to_pointer(cText, (glong)*startOffset);
> > + char* end = g_utf8_offset_to_pointer(cText, (glong)*endOffset);
> > + char* resultText = g_strndup(start, end - start);
> > + g_free(cText);
> > + return resultText;
>
> should we g_strdup resultText?
I am already doing that, see two lines up.
>
> > +}
> > +
> > +static gchar* webkit_accessible_text_get_text_after_offset(AtkText* text, gint offset, AtkTextBoundary boundaryType, gint* startOffset, gint* endOffset)
> > +{
> > + return getTextHelper(AfterOffset, text, offset, boundaryType, startOffset, endOffset);
> > +}
> > +
> > +static gchar* webkit_accessible_text_get_text_at_offset(AtkText* text, gint offset, AtkTextBoundary boundaryType, gint* startOffset, gint* endOffset)
> > +{
> > + return getTextHelper(AtOffset, text, offset, boundaryType, startOffset, endOffset);
> > }
> >
> > -static gchar* webkit_accessible_text_get_text_before_offset(AtkText* text, gint offset, AtkTextBoundary boundary_type, gint* start_offset, gint* end_offset)
> > +static gchar* webkit_accessible_text_get_text_before_offset(AtkText* text, gint offset, AtkTextBoundary boundaryType, gint* startOffset, gint* endOffset)
> > +{
> > + return getTextHelper(BeforeOffset, text, offset, boundaryType, startOffset, endOffset);
> > +}
> > +
> > +static gunichar webkit_accessible_text_get_character_at_offset(AtkText* text, gint offset)
>
> I think we should start using WebKit-style for the function names here.
Next patch perhaps, to do it all at once?
>
> r- for the style issues and the crash Joanmarie reported in comment #16. Would
> also be nice to work out the default behaviour instead of just asserting.
>
OK, thanks!
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
More information about the webkit-unassigned
mailing list