591837 - Add a way to make the findbar search backward by default

Status: NEW

(Toolkit :: Find Toolbar, defect, P5)

Description

[Florian Quèze [:florian]]

Attachment: Patch v1

The current type ahead find code can search both forward and backward, but the current API doesn't allow searching backward for the first find call.

In situations where the most recent content is displayed at the bottom (the case I'm interested in is the conversations window of Instantbird), it makes sense to make the findbar search backward by default. In this case, doing the first find operation forward is not acceptable as it scrolls the view all the way to the top.

The attached patch adds the "reversed" optional attribute to the findbar binding, and does the necessary changes in the type ahead find code.

I would really appreciate if this could be fixed (either with this patch, or
with a simpler solution if anybody can suggest one) because it's one of the only 2 issues (the other is bug 591341) that currently make Instantbird need a patched Mozilla engine.

Comment 1

[Florian Quèze [:florian]]

Comment on attachment 470350 [details] [diff] [review]
Patch v1

Neil: Gavin told me over IRC that he's currently rather busy and doesn't know this code very well. He suggested you may know this code better.

Comment 2

[[e-mail]]

Comment on attachment 470350 [details] [diff] [review]
Patch v1

>   if (aSearchString.IsEmpty()) {
>+    if (selection)
>+      selection->CollapseToStart();
[Personally I think the caller should do this, but what do I know?]

>+  if (selection && atEnd) {
>+    PRInt32 lengthDifference = aSearchString.Length() - mTypeAheadBuffer.Length();
>+    if (lengthDifference == 1 || lengthDifference == -1) {
>+      selectionController->CharacterMove(lengthDifference == 1, PR_TRUE);
>+      nsString newSelection;
>+      selection->ToString(getter_Copies(newSelection));
>+      if (newSelection.Equals(aSearchString)) {
>+        mTypeAheadBuffer = aSearchString;
>+
>+        *aResult = FIND_FOUND;
So, what does this code achieve?

>+  if (selection) {
>+    if (aFindBackwards)
>+      selection->CollapseToEnd();
>+    else
>+      selection->CollapseToStart();
>+  }
[And I would like to see this in FindItNow. See bug 463294. Sigh...]

Comment 3

[Florian Quèze [:florian]]

(In reply to comment #2)
> Comment on attachment 470350 [details] [diff] [review]
> Patch v1
> 
> >   if (aSearchString.IsEmpty()) {
> >+    if (selection)
> >+      selection->CollapseToStart();
> [Personally I think the caller should do this, but what do I know?]
This is something you are doing in bug 463294, right?

> >+  if (selection && atEnd) {
> >+    PRInt32 lengthDifference = aSearchString.Length() - mTypeAheadBuffer.Length();
> >+    if (lengthDifference == 1 || lengthDifference == -1) {
> >+      selectionController->CharacterMove(lengthDifference == 1, PR_TRUE);
> >+      nsString newSelection;
> >+      selection->ToString(getter_Copies(newSelection));
> >+      if (newSelection.Equals(aSearchString)) {
> >+        mTypeAheadBuffer = aSearchString;
> >+
> >+        *aResult = FIND_FOUND;
> So, what does this code achieve?
In case only on character was added or removed at the end of the search string, it will try to preserve the current match, ignoring the default search direction. This makes the "type ahead" feature work even when searching backward (without this it jumps to the next match when typing another character in the search field). When searching forward, it doesn't change the results (at least for all the cases I've tested), and it may be a bit faster.

> >+  if (selection) {
> >+    if (aFindBackwards)
> >+      selection->CollapseToEnd();
> >+    else
> >+      selection->CollapseToStart();
> >+  }
> [And I would like to see this in FindItNow. See bug 463294. Sigh...]
I'm not sure if you want me to change something or if you are just noting with a touch of understandable disappointment that our patches bitrot each other.

Comment 4

[[e-mail]]

(In reply to comment #3)
> > So, what does this code achieve?
> In case only on character was added or removed at the end of the search string,
> it will try to preserve the current match, ignoring the default search
> direction. This makes the "type ahead" feature work even when searching
> backward (without this it jumps to the next match when typing another character
> in the search field). When searching forward, it doesn't change the results (at
> least for all the cases I've tested), and it may be a bit faster.
I'm not convinced that it finds in the same way that real find finds. In particular, I noticed that real find has a bunch of special cases to deal with soft hyphens and new paragraphs. It may be possible to do a real find with a start and end at the same point instead.

> I'm not sure if you want me to change something or if you are just noting with
> a touch of understandable disappointment that our patches bitrot each other.
Well, that and it looks like I need a new reviewer... any suggestions? ;-)

Comment 5

[Florian Quèze [:florian]]

(In reply to comment #4)
> It may be possible to do a real find with a
> start and end at the same point instead.
You think there would be a way to do a real find that goes forward if it only expands the match and goes backward otherwise? I would be (pleasantly) surprised to see such a strange use case already supported.

Comment 6

[Patrick Cloke [:clokep]]

Maybe Gavin has freed up a bit now? :)

Comment 7

[Mike de Boer [:mikedeboer]]

Florian, this still looks like a legitimate use-case to me... is there still a findbar consumer that'd benefit from this feature?

Comment 8

[Florian Quèze [:florian]]

(In reply to Mike de Boer [:mikedeboer] from comment #7)
> Florian, this still looks like a legitimate use-case to me... is there still
> a findbar consumer that'd benefit from this feature?

Yes, the conversation area in Instantbird, and Thunderbird chat.