[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:00:53 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=25415
jmalonzo at gmail.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #30465|review? |review-
Flag| |
------- Comment #17 from jmalonzo at gmail.com 2009-05-23 00:00 PDT -------
(From update of attachment 30465)
> -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.
> +typedef int (*advanceFunc) (int);
> +
> +static int increaseInt(int i) {
> + return i + 1;
> +}
> +
> +static int decreaseInt(int i) {
> + return i - 1;
> +}
> +
> +typedef enum {
> + DirectionForward,
> + DirectionBackwards
> +} Direction;
See enum comment above.
> +
> +static bool findCharacterAttribute(isCharacterAttribute predicateFunction, PangoLogAttr* attributes, Direction direction, int startOffset, int attrsLength, int* resultOffset)
> +{
attrsLength should be unsigned. can starOffset be negative here?
> + int i;
You can move this in for loop below.
> + advanceFunc advanceFunc = direction == DirectionForward ? increaseInt : decreaseInt;
> +
> + *resultOffset = -1;
> +
> + for (i = startOffset; i >= 0 && i < attrsLength; i = advanceFunc(i)) {
int i = startOffset;
> + if (predicateFunction(attributes+i)) {
there should be spaces in attributes+1
> +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.
> +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?
> + if (boundaryType == ATK_TEXT_BOUNDARY_CHAR) {
> + int effectiveOffset;
> +
> + switch(getTextFunctionType) {
space between switch and (.
> + 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?
> + *startOffset = effectiveOffset;
> + *endOffset = effectiveOffset + 1;
what should be the behaviour if effectiveOffset is NULL? should we set
start/end offsets to 0?
> + } else {
> + PangoLogAttr* attrs = g_new(PangoLogAttr, textLength+1);
spaces in textLength+1
> + PangoLanguage* language = pango_language_get_default();
> + pango_get_log_attrs(cText, -1, -1, language, attrs, textLength+1);
ditto.
> +
> + 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?
> +
> + switch(boundaryType) {
space between switch and (.
> + 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?
> + } 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.
> + 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?
> + 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?
> +}
> +
> +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.
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.
--
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