132894 - "Find again" jumps over text it should have matched

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Malcolm Parsons]

I did a search for "01179" on a webpage containing lots of text like this:

01179532025 01179651721 01179653008 01179661578 
01179674953 01179690091 01179690808 01179692875

The first occurence is found correctly, but repeated searches randomly miss out
text that should have matched.

Comment 1

[Malcolm Parsons]

Attachment: This file shows the problem.

Comment 2

[Samir Gehani]

-> XPApps: GUI Features for find

Comment 3

[Paul Wyskoczka]

--> find   qa [e-mail]

Comment 4

[Boris Zbarsky [:bzbarsky]]

To akkana.  I'm seeing this...

Comment 5

[Akkana Peck]

I see it too.  Here's what's happening: the numbers that get skipped are numbers
that come after a number ending in 0, which matches the first character of the
pattern string.  So it matches the last char in that node, gets the next node
and sets restart to the first character in that node, fails the comparison, then
resets the pattern and goes back to restart+1.

It needs to set restart to -1 (i.e. in the previous node) so that when it
increments restart, it ends up on the first character of the present node rather
than the second character.  It needs to do that in a way that's general enough
that it still works if we do have to go back to the previous node (or several
nodes back) in order to restart the comparison.  Working on it ...

Comment 6

[Akkana Peck]

Attachment: A fix

This seems to fix it.  I thought more would be needed, but this fixes the test
case in both forward and reverse directions.  The fix is simple: when we get a
new node, if pindex is past the starting point, then we set "restart" to be out
of bounds.

The patch also includes a minor code change where we get the text fragment
length only once and cache it instead of repeatedly getting it, since I noticed
it was being reused a lot.

We still pass all the regression tests with this change.I'll work on adding
this case to the find regression tests.

Comment 7

[Charles Manske]

Comment on attachment 76137 [details] [diff] [review]
A fix

r=cmanske
yep, this makes sense!

Comment 8

[Akkana Peck]

Attachment: Better fix, with code cleanup

That fix did fix the problem, and passed all the regression tests, but I didn't
understand why it worked (it didn't make sense to me) and that bugged me.  So I
went back, analyzed the code better and now I have a fix that I do understand.

First of all, the code had two confusing pairs of variables: it wasn't at all
clear what the difference was between restart and matchAnchorOffset (they both
fulfilled more or less the same purpose, but it was inconsistent where one
would be used vs. the other), and it wasn't clear what purpose offset was
serving relative to mIterOffset (originally, when nsFind was stateful, it made
sense because we sometimes wanted to remember mIterOffset between invocations,
but now that it's stateless we have no more need to remember the offset).  It
was difficult to understand or debug the code with these two confusing pairs of
variables interfering with each other.

So this patch eliminates restart in favor of matchAnchorOffset (that name is
clearer, since it's closely paired with matchAnchorNode and is only relevant
when matchAnchorNode is set), and eliminates offset and uses mIterOffset
instead.

In the clause where we're initializing the new node (starting around line 664
of the new file), we were dancing around setting restart, then setting findex
to restart.  This was why the example in this bug failed: restart was getting
reset so we weren't going back to the right place in the anchor node.  In the
new code, we set findex to the same thing it would have been set to before, but
we don't set matchAnchorOffset (previously restart).  That variable is only set
at the start of a new match, in the same place where we set matchAnchorNode.

You may notice the extra "+ (mFindBackward ? 1 : 0)": I chose not to add that
to matchAnchorOffset (it had been being added to restart) because I decided it
was clearer to add it when needed (basically, when converting to a range
endpoint or when iterating past the previous point) rather than add it always
and then subtract it at other times (e.g. when setting findex  to it at line
884).

There are also some changes to debug printfs, and some changes getting the
fragment length only once instead of repeatedly.

This passes all the existing regression tests plus the case in the bug (which I
will add to the regression tests soon).

Comment 9

[Charles Manske]

Comment on attachment 76274 [details] [diff] [review]
Better fix, with code cleanup

r=cmanske

Comment 10

[Akkana Peck]

Attachment: Same fix, diff -u this time

Comment 11

[Simon Fraser [no longer active]]

Comment on attachment 76301 [details] [diff] [review]
Same fix, diff -u this time

Comments:

+      // Set our starting point in this node.
+      // If we're going back to the anchor node, which means that we
+      // just ended a partial match, use the saved offset:
+      if (mIterNode == matchAnchorNode)
+	 findex = matchAnchorOffset + (mFindBackward ? 1 : 0);
+
       // mIterOffset, if set, is the range's idea of an offset,
       // and points between characters.  But when translated
       // to a string index, it points to a character.	If we're
       // going backward, this is one character too late and
       // we'll match part of our previous pattern.
-      if (offset >= 0)
-	 restart = offset - (mFindBackward ? 1 : 0);
+      else if (mIterOffset >= 0)
+	 findex = mIterOffset - (mFindBackward ? 1 : 0);

The second and subsequent 'else' clauses there gets a bit lost in all those
comments. How about

  if (foo) {
     // big comment
     code
  }
  else if (bar) {
    // other big comment
    code
  }
  else ...

Fix that if you wish. sr=sfraser

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 76301 [details] [diff] [review]
Same fix, diff -u this time

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 13

[Akkana Peck]

Fix is in.

Comment 14

[Patty Mac]

Verified on windows 98 and linux (2002-04-08-08-TRUNK)