"find previous" feature

VERIFIED FIXED in Future

Status

SeaMonkey
UI Design
--
enhancement
VERIFIED FIXED
17 years ago
13 years ago

People

(Reporter: Doron Rosenberg (IBM), Assigned: Doron Rosenberg (IBM))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: need feedback)

Attachments

(8 attachments)

(Assignee)

Description

17 years ago
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...
(Assignee)

Comment 1

17 years ago
Created attachment 32292 [details] [diff] [review]
initial patch, needs cleanup and feedback!
(Assignee)

Updated

17 years ago
Blocks: 39389

Comment 2

17 years ago
Eventually, Simon should be one of the reviewers for this patch.
Severity: normal → enhancement
Keywords: patch
Hardware: PC → All
Whiteboard: need feedback

Comment 3

17 years ago
This would be trivial to do, but that patch probably needs to change now that I 
checked in changes to how Find works.
(Assignee)

Comment 4

17 years ago
Created attachment 33645 [details] [diff] [review]
patch
(Assignee)

Comment 5

17 years ago
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

17 years ago
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++.
(Assignee)

Comment 7

17 years ago
hmm, ok, i had that before but changed it to c++. Going to attach a new patch
that does this via js
(Assignee)

Comment 8

17 years ago
Created attachment 34307 [details] [diff] [review]
trivial patch
(Assignee)

Comment 9

17 years ago
patch attached, simply always sets findBackwards to true. Trivial

Comment 10

17 years ago
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.
(Assignee)

Comment 11

17 years ago
Created attachment 34527 [details] [diff] [review]
new patch
(Assignee)

Comment 12

17 years ago
new patch, created a setFindInPage that returns a findInst.  Any other comments?
Assignee: pchen → doronr

Comment 13

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

Comment 14

17 years ago
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?
(Assignee)

Comment 15

17 years ago
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

Comment 16

17 years ago
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?
(Assignee)

Comment 17

17 years ago
Created attachment 34616 [details] [diff] [review]
another patch
(Assignee)

Comment 18

17 years ago
changes implemented.
(Assignee)

Comment 19

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

Comment 20

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

Comment 21

17 years ago
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
(Assignee)

Comment 22

17 years ago
Created attachment 35402 [details] [diff] [review]
Patch - findutils.js
(Assignee)

Comment 23

17 years ago
Created attachment 35403 [details] [diff] [review]
Patch - Navigator
(Assignee)

Comment 24

17 years ago
Created attachment 35404 [details] [diff] [review]
Patch - Mailnews
(Assignee)

Comment 25

17 years ago
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

Comment 26

17 years ago
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();? 

Comment 28

17 years ago
>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.
(Assignee)

Comment 29

17 years ago
0.9.2 freeze is 7 days away. Do you want this to go in?
(Assignee)

Comment 30

17 years ago
-> 0.9.3, 0.9.2 is too close
Target Milestone: mozilla0.9.2 → mozilla0.9.3
(Assignee)

Comment 31

17 years ago
So other than BrowserCanFind(), any other approval this needs?
(Assignee)

Comment 32

17 years ago
->1.0
Target Milestone: mozilla0.9.3 → mozilla1.0
(Assignee)

Comment 33

16 years ago
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

Comment 35

16 years ago
Why was this futured if there's a patch...?
*** Bug 157772 has been marked as a duplicate of this bug. ***

Comment 37

16 years ago
Doron, what's happened with this? You had a fix and everything.
Keywords: nsbeta1
(Assignee)

Comment 38

16 years ago
missing module owner approval :)

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

Comment 39

16 years ago
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

15 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 15 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?

Comment 42

15 years ago
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

Comment 44

15 years ago
*** 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.