[Webkit-unassigned] [Bug 57336] experiment with moving caret by word in visual order
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Mar 30 22:34:52 PDT 2011
https://bugs.webkit.org/show_bug.cgi?id=57336
Ryosuke Niwa <rniwa at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #87628|review? |review-
Flag| |
--- Comment #11 from Ryosuke Niwa <rniwa at webkit.org> 2011-03-30 22:34:52 PST ---
(From update of attachment 87628)
View in context: https://bugs.webkit.org/attachment.cgi?id=87628&action=review
> Source/WebCore/editing/SelectionController.cpp:451
> + // FIXME: remove later.
Since we've already added this comment to where we declared WebKitVisualWordGranularity, I don't think we need to repeat ourselves here.
> Source/WebCore/editing/SelectionController.cpp:493
> + // FIXME: remove later.
Ditto.
> Source/WebCore/editing/SelectionController.cpp:578
> + // FIXME: remove later.
Ditto.
> Source/WebCore/editing/SelectionController.cpp:621
> + // FIXME: remove later.
Ditto.
> Source/WebCore/editing/SelectionController.cpp:668
> + // FIXME: remove later.
Ditto.
> Source/WebCore/editing/SelectionController.cpp:746
> + // FIXME: remove later.
Ditto.
> Source/WebCore/editing/VisibleSelection.cpp:383
> + // FIXME: remove later.
Ditto.
> Source/WebCore/editing/visible_units.cpp:1160
> + // For LTR text, previousWordPosition() returns the word break at the left boundary of a word,
> + // nextWordPosition() returns the word break at the right boundary of a word.
> + // For RTL text, previousWordPosition() returns the word break at the right boundary of a word,
> + // nextWordPosition() returns the word break at the left boundary of a word.
I don't understand why we need this comment here. It seems like we want these comments to be right above previousWordPosition and nextWordPosition instead.
Or, you should explain how these things affect what we do in this function. Why explaining what nextWordPosition and previousWordPosition do isn't particularly helpful here
because this comment appears before any code where this information is relevant.
> Source/WebCore/editing/visible_units.cpp:1165
> + // In LTR block, ctrl-arrow moves cursor to the left boundary of words,
> + // so we should use previousWordPosition() for LTR text, in which offsets collected from visually right to visually left.
> + // In RTL block, ctrl-arrow moves cursor to the right boundary of words,
> + // so we should use previousWordPosiiton() for RTL text, in which, offsets collected from visually left to visually right.
This comment isn't intelligible for me. What does ctrl-arrow mean? Which arrow key are we talking about?
You say we should use previousWordPosition() for LTR text but you don't really explain why we should.
And I can't make a sense of previousWordPosition() "collecting" offsets visually right to left.
> Source/WebCore/editing/visible_units.cpp:1185
> + wordBreak.getInlineBoxAndOffset(boxContainingPreviousWordBreak, offsetOfWordBreak);
I'd declare boxContainingPreviousWordBreak and offsetOfWordBreak here instead of at the beginning of the function. It's okay to have multiple local variables of the same name as long as they are in separate blocks.
> Source/WebCore/editing/visible_units.cpp:1198
> + if (box->direction() == blockDirection)
> + wordBreak = previousWordBreakInBoxInsideBlockWithSameDirectionality(box, wordBreak, offsetOfWordBreak);
I'm a bit confused here. If we're looking for a word boundary in the text direction of block and box and block have the same directionality, shouldn't we be looking for the next word boundary instead of previous one?
> Source/WebCore/editing/visible_units.cpp:1204
> + } while (1);
Since you're putting 1 in while's condition, I'd use the regular while instead of do-while.
> Source/WebCore/editing/visible_units.cpp:1221
> + const InlineBox* adjacentBox = box;
> + while (adjacentBox) {
> + VisiblePosition wordBreak;
> + if ((directionToSearch == WebCore::DirectionRight && blockDirection == LTR)
> + || (directionToSearch == WebCore::DirectionLeft && blockDirection == RTL))
> + wordBreak = findWordBoundaryInBoxInBlockDirection(adjacentBox, offset, blockDirection);
> + // FIXME: "else" not implemented yet.
> + if (!wordBreak.isNull())
> + return wordBreak;
> + adjacentBox = (directionToSearch == WebCore::DirectionLeft) ? adjacentBox->nextLeafChild() : adjacentBox->prevLeafChild();
> + offset = invalidOffset;
> + }
Since this code is fairly simple, I'd duplicate code in leftWordBoundary and rightWordBoundary rather than adding a function that takes directionToSearch.
You can then rewrite this while loop as a simple for loop (leftWordBoundary case):
static VisiblePosition leftWordBoundary(const InlineBox* box, int offset, TextDirection blockDirection, const VisiblePosition& visiblePosition)
{
for (const InlineBox* adjacentBox = box; adjacentBox; adjacentBox = adjacentBox->prevLeafChild()) {
if (blockDirection == RTL)
wordBreak = findWordBoundaryInBoxInBlockDirection(adjacentBox, adjacentBox == box ? offset : invalidOffset, blockDirection);
// FIXME: Implement "else" case.
if (wordBreak.isNotNull())
return wordBreak;
}
}
> Source/WebCore/editing/visible_units.cpp:1222
> + return visiblePosition;
I don't understand the point of returning visiblePosition here. Is this visiblePosition used in the "else" case? If not, I'd just return null here and take care of that in leftWordPosition and rightWordPosition.
> Source/WebCore/editing/visible_units.cpp:1238
> + // In LTR block, ctrl-arrow moves cursor to the left boundary of words.
> + // In RTL block, ctrl-arrow moves cursor to the right boundary of words.
I can't make sense of this comment. I think you should remove it.
> LayoutTests/editing/selection/move-by-word-visually-expected.txt:7
> +"abc def hij opq rst"[0, 4, 8, 12, 16, 19] FAIL expected: [4, 8, 12, 16, 19, 19]
> +Move left by one word
> +"abc def hij opq rst"[16, 16, 12, 8, 4, 0] FAIL expected: [16, 12, 8, 4, 0, 0]
Very nice output! I can make sense of outputs now. But the expected result for the second row doesn't look right.
Since we have "0 4 8 12 16 19|19 16 12 8 4 0" in the title, shouldn't we see FAIL expected [19, 16, 12, 8, 4, 0] instead?
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list