[Webkit-unassigned] [Bug 57806] continue experiment with moving caret by word in visual order
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Apr 6 06:58:13 PDT 2011
https://bugs.webkit.org/show_bug.cgi?id=57806
Ryosuke Niwa <rniwa at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #88319|review? |review-
Flag| |
--- Comment #8 from Ryosuke Niwa <rniwa at webkit.org> 2011-04-06 06:58:13 PST ---
(From update of attachment 88319)
View in context: https://bugs.webkit.org/attachment.cgi?id=88319&action=review
Okay, I think leftmostPositionInRTLBoxInLTRBlock, rightmostPositionInLTRBoxInRTLBlock, & lastWordBreakInBox look pretty sane to me. They should be good to go after this iteration. The rest seems to require a couple more iterations. Thanks for patiently improving the patch :)
> Source/WebCore/editing/visible_units.cpp:1179
> + VisiblePosition leftmost;
This variable doesn't seem to be ever used. Please remove.
> Source/WebCore/editing/visible_units.cpp:1183
> + if (previousLeaf && !previousLeaf->isLeftToRightDirection())
Let's put a blank line right above this line to improve the readability.
> Source/WebCore/editing/visible_units.cpp:1196
> + VisiblePosition rightmost;
Ditto; can't find any reference.
> Source/WebCore/editing/visible_units.cpp:1200
> + if (nextLeaf && nextLeaf->isLeftToRightDirection())
Ditto about inserting a blank line here.
> Source/WebCore/editing/visible_units.cpp:1220
> + if (boundaryPosition.isNotNull()) { // boundaryPosition is not null only in the above 2 cases.
Please do an early return instead.
> Source/WebCore/editing/visible_units.cpp:1232
> +static bool positionIsVisuallyOrderedInBox(const VisiblePosition& wordBreak, const InlineBox* box, int& offsetOfWordBreak)
I don't think positionIsVisuallyOrderedInBox is a descriptive name here. It explains what the function is checking but it doesn't explain anything in the context of nextWordBreakInBoxInsideBlockWithDifferentDirectionality. Please rename.
> Source/WebCore/editing/visible_units.cpp:1237
> + // "previousOffset < offsetOfWordBreak" is to guarantee that the word breaks collected are visually ordered.
I don't think this comment is helpful. It doesn't explain why "previousOffset < offsetOfWordBreak" guarantees that the words breaks are visually ordered.
> Source/WebCore/editing/visible_units.cpp:1262
> + isLastWordBreakInBox = false;
> + if (wordBreak == previousWordBreak) {
> + isLastWordBreakInBox = true;
It seems redundant to set isLastWordBreakInBox to false if we're immediately setting it back to true. I'd move isLastWordBreakInBox = false below the if clause.
> Source/WebCore/editing/visible_units.cpp:1304
> +static VisiblePosition wordBreakInBox(const InlineBox* box, TextDirection blockDirection, const VisiblePosition& previousWordBreak, int& offsetOfWordBreak, bool& isLastWordBreakInBox)
> +{
> + isLastWordBreakInBox = false;
> + VisiblePosition wordBreak;
> + if (box->direction() == blockDirection) {
> + wordBreak = previousWordBreakInBoxInsideBlockWithSameDirectionality(box, previousWordBreak, offsetOfWordBreak);
> + if (wordBreak.isNull())
> + isLastWordBreakInBox = true;
> + } else
> + wordBreak = nextWordBreakInBoxInsideBlockWithDifferentDirectionality(box, previousWordBreak, offsetOfWordBreak, isLastWordBreakInBox);
> + return wordBreak;
> +}
> +
> +static void collectWordBreaksInBox(const InlineBox* box, TextDirection blockDirection, Vector<std::pair<VisiblePosition, int>, 50>& wordBoundaries)
> +{
> + wordBoundaries.clear();
> + int offset = invalidOffset;
> + bool isLastWordBreakInBox = false;
> + VisiblePosition wordBreak = wordBreakInBox(box, blockDirection, VisiblePosition(), offset, isLastWordBreakInBox);
> + while (!isLastWordBreakInBox) {
> + wordBoundaries.append(std::make_pair(wordBreak, offset));
> + wordBreak = wordBreakInBox(box, blockDirection, wordBreak, offset, isLastWordBreakInBox);
> + }
> + if (!wordBreak.isNull())
> + wordBoundaries.append(std::make_pair(wordBreak, offset));
> +}
As I said in the previous comment, I don't see why these two functions can't be merged. In fact, I just did it for you (please verify):
static void collectWordBreaksInBox(const InlineBox* box, TextDirection blockDirection, Vector<std::pair<VisiblePosition, int>, 50>& wordBoundaries)
{
wordBoundaries.clear();
VisiblePosition wordBreak;
int offsetOfWordBreak = invalidOffset;
while (1) {
if (box->direction() == blockDirection) {
wordBreak = previousWordBreakInBoxInsideBlockWithSameDirectionality(box, wordBreak, offsetOfWordBreak);
if (wordBreak.isNull())
break;
wordBoundaries.append(std::make_pair(wordBreak, offset));
} else {
bool isLastWordBreakInBox = false;
wordBreak = nextWordBreakInBoxInsideBlockWithDifferentDirectionality(box, wordBreak, offsetOfWordBreak, isLastWordBreakInBox);
if (wordBreak.isNotNull())
wordBoundaries.append(std::make_pair(wordBreak, offset));
if (isLastWordBreakInBox)
break;
}
}
}
Once I did that, it was obvious to me that we should have two different functions for same directionality and different directionality as in:
static void collectWordBreaksInBoxInsideBlockWithSameDirectionality(const InlineBox* box, Vector<std::pair<VisiblePosition, int>, 50>& wordBoundaries)
{
wordBoundaries.clear();
VisiblePosition wordBreak;
int offsetOfWordBreak = invalidOffset;
while (1) {
wordBreak = previousWordBreakInBoxInsideBlockWithSameDirectionality(box, wordBreak, offsetOfWordBreak);
if (wordBreak.isNull())
break;
wordBoundaries.append(std::make_pair(wordBreak, offset));
}
}
static void collectWordBreaksInBoxInsideBlockWithDifferntDirectionality(const InlineBox* box, Vector<std::pair<VisiblePosition, int>, 50>& wordBoundaries)
{
wordBoundaries.clear();
VisiblePosition wordBreak;
int offsetOfWordBreak = invalidOffset;
while (1) {
bool isLastWordBreakInBox = false;
wordBreak = nextWordBreakInBoxInsideBlockWithDifferentDirectionality(box, wordBreak, offsetOfWordBreak, isLastWordBreakInBox);
if (wordBreak.isNotNull())
wordBoundaries.append(std::make_pair(wordBreak, offset));
if (isLastWordBreakInBox)
break;
}
}
> Source/WebCore/editing/visible_units.cpp:1322
> +static int greatestValueUnder(int offset, bool boxAndBlockInSameDirection, const Vector<std::pair<VisiblePosition, int>, 50>& wordBoundaries)
You should rename wordBoundaries to something like orderedWordBoundaries to signify the fact they're ordered. Or otherwise things doesn't make sense.
Also I would have much preferred if you defined a struct instead of using a pair here so that I know what a pair represents.
Nit: s/boxAndBlockInSameDirection/boxAndBlockAreInSameDirection/.
> Source/WebCore/editing/visible_units.cpp:1394
> + if (box == boxOfWordBreak && offsetOfWordBreak != box->caretMaxOffset() && offsetOfWordBreak != box->caretMinOffset())
> + return true;
> + return false;
Nit: we should do:
return box == boxOfWordBreak && offsetOfWordBreak != box->caretMaxOffset() && offsetOfWordBreak != box->caretMinOffset();
instead.
> Source/WebCore/editing/visible_units.cpp:1397
> +static VisiblePosition positionAfterNextWord(const VisiblePosition& position)
Oops, this should probably named as positionAfterWord instead. Since if we're in "he^llo world", then this function will return "hello^ world".
> Source/WebCore/editing/visible_units.cpp:1400
> + VisiblePosition next = nextWordPosition(position);
> + VisiblePosition nextNext = nextWordPosition(next);
next and nextNext don't make any sense to me. Next what? It's much clearer to call next as beforeNextWord because it signifies the fact nextWordPosition returns the position between the next word and the whitespace immediately before the next word. It then becomes clear from your other comment that we must move forward again to obtain beforeWordAfterNextWord or afterNextWord & backward in order obtain the position after the current word.
> Source/WebCore/editing/visible_units.cpp:1406
> +static VisiblePosition positionBeforePreviousWord(const VisiblePosition& position)
Ditto. This should be named as positionBeforeWord
> Source/WebCore/editing/visible_units.cpp:1409
> + VisiblePosition previous = previousWordPosition(position);
> + VisiblePosition previousPrevious = previousWordPosition(previous);
Ditto about local variable names here.
> Source/WebCore/editing/visible_units.cpp:1446
> + // Otherwise, first collect all word breaks in terms of (VisiblePosition, offset) inside the box.
Please get rid of this comment. It repeats the function name collectWordBreaksInBox.
> Source/WebCore/editing/visible_units.cpp:1447
> + Vector<std::pair<VisiblePosition, int>, 50> wordBoundaries;
Why 50? That sounds like an arbitrary limit to me. I'd rather not to specify any number here.
> Source/WebCore/editing/visible_units.cpp:1449
> + // Then, search for word boundaries inside the box.
Please remove this comment. It repeats what code says.
> Source/WebCore/editing/visible_units.cpp:1455
> + // If not found, search for word boundaries in visually adjacent boxes.
Please remove this comment. It repeats the code.
> Source/WebCore/editing/visible_units.cpp:1456
> + return leftWordBoundary(box->prevLeafChild(), invalidOffset, blockDirection);
Mn... this is a tail recursion. I wonder if we can make it iterative in a some followup patch.
--
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