77704 - "find previous" feature

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

[Doron Rosenberg (IBM)]

We need a find previous method where we can via a menuitem find the previous
occurance (needed for example in view source context menu bug).  I have a
working patch attached, which probably needs to be polished and feedback.

Currently, this is how I do it:

I modified nsFindComponent::FindNext to have a additional variable (PRBool)
passed to it called aFindPrevious.  If it is true, I set SearchBackwards true,
and after the search, reset it to the original value.  So if nothing was
searched for and search previous is invoked, the SearchBackwards checkbox will
be checked.

I also added changes to nsBrowserInstance to work with this, even though I
understand that is being killed.

It might make more sense to use a aFindNext variable, so feedback is welcome.  I
originally had a FindPrevious() function, but thought that combining the 2 into
one function was better.

Please keep in mind that the patch attached needs cleanup and has useless
fprintf()'s for some simple debug work commented out...

Comment 1

[Doron Rosenberg (IBM)]

Attachment: initial patch, needs cleanup and feedback!

Comment 2

[Kathleen :Brade]

Eventually, Simon should be one of the reviewers for this patch.

Comment 3

[Simon Fraser [no longer active]]

This would be trivial to do, but that patch probably needs to change now that I 
checked in changes to how Find works.

Comment 4

[Doron Rosenberg (IBM)]

Attachment: patch

Comment 5

[Doron Rosenberg (IBM)]

the new patch is only backend, all js calls to findnext() need just to pass a n
additional boolean (findPrevious), like in findutils.js and the rest.

Comment 6

[Simon Fraser [no longer active]]

I think this is the wrong approach. I think you just need to make a 
'findPreviousInPage()' in findUtils.js that sets findInst.findBackwards to false, 
and not change any of the C++.

Comment 7

[Doron Rosenberg (IBM)]

hmm, ok, i had that before but changed it to c++. Going to attach a new patch
that does this via js

Comment 8

[Doron Rosenberg (IBM)]

Attachment: trivial patch

Comment 9

[Doron Rosenberg (IBM)]

patch attached, simply always sets findBackwards to true. Trivial

Comment 10

[Simon Fraser [no longer active]]

See, wasn't that easy  :)

You might like to factor the common code that you copy/pasted into a new 
function, since there is quite a lot of it.

Comment 11

[Doron Rosenberg (IBM)]

Attachment: new patch

Comment 12

[Doron Rosenberg (IBM)]

new patch, created a setFindInPage that returns a findInst.  Any other comments?

Comment 13

[Simon Fraser [no longer active]]

How about 'initFindInPage' rather than 'setFindInPage'. Other than that, r=
sfraser

Comment 14

[kinmoz]

So if findBackwards is set to true, won't FindNext do the same thing as 
findPrevious?

Should we modify FindNext to always look forwards too?

What about the findAgain keyboard shortcut, what should it do?

Comment 15

[Doron Rosenberg (IBM)]

if you select the backwards checkbox in the finddialog, then find next should
observe that. That is how we used to do it. However, now that there is a find
previous, I guess we can change this. should I?

I'll also file a bug about giving it a shortcut and menuitem.

Comment 16

[Simon Fraser [no longer active]]

The problem here is that having a findPreviousInPage function make the usage of 
these fuctions ambiguous. findNextInPage may either find forwards or backwards, 
depending on the service settings. Your findPreviousInPage will aways find 
backwards. So maybe findNextInPage should always find forwards?

Comment 17

[Doron Rosenberg (IBM)]

Attachment: another patch

Comment 18

[Doron Rosenberg (IBM)]

changes implemented.

Comment 19

[Doron Rosenberg (IBM)]

so I assume r=sfraser, anyone up for a sr=?

Comment 20

[Simon Fraser [no longer active]]

What about the associated XUL? If you check this in as is, you'll break Find Next 
when the user was last finding backwards.

Comment 21

[Doron Rosenberg (IBM)]

ok, i will talk to the UI people about adding "find previous" and giving it a
keycombo and renaming "find again" to "find next". I have it working in my tree
but not sure what keycombo to give them

Comment 22

[Doron Rosenberg (IBM)]

Attachment: Patch - findutils.js

Comment 23

[Doron Rosenberg (IBM)]

Attachment: Patch - Navigator

Comment 24

[Doron Rosenberg (IBM)]

Attachment: Patch - Mailnews

Comment 25

[Doron Rosenberg (IBM)]

ok, attached 3 patches

as per mpt, changed the accesskeys and keycombos. 

findutils.js: the original find changes, renamed the function to use "next"
instead of "again", also canFindAgain() is canFind() (as it is for both previous
and next).

navigator: ui changes (renaming "again" to "next" and adding "previous")
mailnews: ui changes (renaming "again" to "next" and adding "previous")

I tested both mail and navigator and did  not see any regressions. find next and
find previous worked.  I did not patch composer as it does not seem to use
findutils.js

ccing blake for the nav changes and sspitzer for mailnews changes.

Comment 26

[Simon Fraser [no longer active]]

This needs running past the UI folks.

Comment 27

[Ben Goodger (use ben at mozilla dot org for email)]

Well, the Ctrl+G, Ctrl+Shift+G solution seems logical. 

code wise, do we need BrowserCanFind(); when all it does is call canFindInPage();? 

Comment 28

[Simon Fraser [no longer active]]

>do we need BrowserCanFind(); when all it does is call canFindInPage();? 

Yeah, it's consistent with the call levels of the other find-related functions, 
and a bottleneck that could be enhanced later.

Comment 29

[Doron Rosenberg (IBM)]

0.9.2 freeze is 7 days away. Do you want this to go in?

Comment 30

[Doron Rosenberg (IBM)]

-> 0.9.3, 0.9.2 is too close

Comment 31

[Doron Rosenberg (IBM)]

So other than BrowserCanFind(), any other approval this needs?

Comment 32

[Doron Rosenberg (IBM)]

->1.0

Comment 33

[Doron Rosenberg (IBM)]

future I guess

Comment 34

[sairuh (rarely reading bugmail)]

mass moving open bugs pertaining to find in page/frame to [e-mail] as
qa contact.

to find all bugspam pertaining to this, set your search string to
"AppleSpongeCakeWithCaramelFrosting".

Comment 35

[Blake Ross]

Why was this futured if there's a patch...?

Comment 36

[Mats Palmgren (inactive)]

*** Bug 157772 has been marked as a duplicate of this bug. ***

Comment 37

[Aaron Leventhal]

Doron, what's happened with this? You had a fix and everything.

Comment 38

[Doron Rosenberg (IBM)]

missing module owner approval :)

If people agree, I can update this probably to work.

Comment 39

[Aaron Leventhal]

I don't think you need menu items, just the accelerator. Menu items might bloat
the menus too much.

However, that's just my opinion. If you decide to go with just the accelerator,
then I give my approval as keyboard component owner.

Comment 40

[Aaron Leventhal]

checked in

Comment 41

[sairuh (rarely reading bugmail)]

aaronl, since the fix for this was part of bug 167921 (which dealt with
typeahead find), will this appear in only mozilla builds, or both moz and
commercial?

Comment 42

[Aaron Leventhal]

It will appear in commercial too.

Doron, sorry I didn't mean to take this from under you -- it just involved a
bunch of stuff I was doing with typeaheadfind, so it made sense to do it all at
once.

Comment 43

[sairuh (rarely reading bugmail)]

vrfy'd fixed on both commercial and mozilla trunk builds, 2002.09.26.08. Find
Previous is now under the Edit menu; accel-shift-G works as the shortcut (as
does shift-F3).

Comment 44

[Aaron Leventhal]

*** Bug 172612 has been marked as a duplicate of this bug. ***