Closed Bug 77704 Opened 20 years ago Closed 18 years ago

"find previous" feature

Categories

(SeaMonkey :: UI Design, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Future

People

(Reporter: doronr, Assigned: doronr)

References

Details

(Whiteboard: need feedback)

Attachments

(8 files)

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...
Blocks: 39389
Eventually, Simon should be one of the reviewers for this patch.
Severity: normal → enhancement
Keywords: patch
Hardware: PC → All
Whiteboard: need feedback
This would be trivial to do, but that patch probably needs to change now that I 
checked in changes to how Find works.
Attached patch patchSplinter Review
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.
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++.
hmm, ok, i had that before but changed it to c++. Going to attach a new patch
that does this via js
Attached patch trivial patchSplinter Review
patch attached, simply always sets findBackwards to true. Trivial
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.
Attached patch new patchSplinter Review
new patch, created a setFindInPage that returns a findInst.  Any other comments?
Assignee: pchen → doronr
How about 'initFindInPage' rather than 'setFindInPage'. Other than that, r=
sfraser
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?
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.
Status: NEW → ASSIGNED
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?
Attached patch another patchSplinter Review
changes implemented.
so I assume r=sfraser, anyone up for a sr=?
What about the associated XUL? If you check this in as is, you'll break Find Next 
when the user was last finding backwards.
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
Attached patch Patch - MailnewsSplinter Review
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.
Target Milestone: --- → mozilla0.9.2
This needs running past the UI folks.
Well, the Ctrl+G, Ctrl+Shift+G solution seems logical. 

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

>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.
0.9.2 freeze is 7 days away. Do you want this to go in?
-> 0.9.3, 0.9.2 is too close
Target Milestone: mozilla0.9.2 → mozilla0.9.3
So other than BrowserCanFind(), any other approval this needs?
->1.0
Target Milestone: mozilla0.9.3 → mozilla1.0
future I guess
Target Milestone: mozilla1.0 → Future
mass moving open bugs pertaining to find in page/frame to pmac@netscape.com as
qa contact.

to find all bugspam pertaining to this, set your search string to
"AppleSpongeCakeWithCaramelFrosting".
QA Contact: sairuh → pmac
Why was this futured if there's a patch...?
*** Bug 157772 has been marked as a duplicate of this bug. ***
Doron, what's happened with this? You had a fix and everything.
Keywords: nsbeta1
missing module owner approval :)

If people agree, I can update this probably to work.
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.
checked in
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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?
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.
QA Contact: pmac → sairuh
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).
Status: RESOLVED → VERIFIED
*** Bug 172612 has been marked as a duplicate of this bug. ***
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.