Closed Bug 608174 Opened 16 years ago Closed 14 years ago

"Not found" not cleared from Find bar if subsequently appearing text contains find string

Categories

(Camino Graveyard :: General, defect)

Product:
Camino Graveyard
Component:
General
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: lthompson.22, Assigned: alqahira)

References

Details

(Whiteboard: [camino-2.1.3])

Attachments

(1 file, 6 obsolete files)

Fix, v2.1.1 with sr comments addressed, missing project diff, and MPL2
17.93 KB, patch
stuart.morgan+bugzilla
: superreview+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en; rv:1.9.2.13pre) Gecko/20101027 Camino/2.1a1pre (like Firefox/3.6.13pre) Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en; rv:1.9.2.13pre) Gecko/20101027 Camino/2.1a1pre (like Firefox/3.6.13pre) STR 1. Cmd-F and type in a string that does not appear on this page. 2. Note that Find bar reports "Not found" 3. Type that same string in the Additional Comments field below. 4. Cmd-G until the string you just typed is highlighted (looks like you have to do it twice, as the first one only de-selects the field) AR: "Not found" still appears in the Find bar ER: The "Not found" notice should be cleared since the text was found. (If you use the arrow buttons in the Find bar instead of Cmd-G, the first click will clear the "Not found" notice, a second click highlights the text.) Another set of circumstances in which the "Not found" notice remains after a string is found is if you do a find while a page is still loading (a Bugzilla query with lots of results, for instance), if the text isn't found at first but appears later and gets highlighted with Cmd-G, the Find bar still says "Not found". Reproducible: Always
Does this happen in Firefox, too? I bet it does, because the text didn't exist when the original search was run. It is kinda weird that it eventually finds it, though. Maybe the solution is to disable Cmd-G when a string isn't found? If you can't find it in the first place, how could you possibly find it again? (Safari, for instance, just beeps at you if you Cmd-G after an unsuccessful Cmd-F.) In the STR above, if you substitute Cmd-F in step 4, the "not found" goes away. For that matter, if a string isn't found, we should probably disable those two buttons in the Find bar that let you move from result to result, too.
No, it doesn't happen in Firefox 3.6.11 -- the "new" text is highlighted and the notice in the Find bar is updated appropriately. Same with any word/text processor I've ever used. I just tried two Apple and two third-party programs: If you do a Cmd-F Find and get "not found", then type the same string in the document and do a Cmd-G, the string you just typed is highlighted and the Find window updates the notice, if any, appropriately. Safari seems to do the same thing Camino does, ie. highlight the new text but the "not found" notice persists. > Maybe the solution is to disable Cmd-G when a string isn't found? That would be horrible, and inconsistent in my experience with Mac programs, where you can always run a "find again" on a different document (or tab or page, in a browser) even if it wasn't found in a previous document.
Bug 417794 is related, in the sense the Find bar UI isn't getting updated like it "should" when we run a Find-related menu command. This one is more of an edge case because 1) you have to perform a somewhat contrived user action to clear the error condition (type the very thing you're searching for) and 2) then stop interacting with the Find bar altogether (whereas there's a reasonable expectation that Cmd-E will update the search field of a Find impl, as it did the old Find panel). Still, I think it's a valid bug. (In reply to comment #2) > > Maybe the solution is to disable Cmd-G when a string isn't found? > That would be horrible, and inconsistent in my experience with Mac programs, > where you can always run a "find again" on a different document (or tab or > page, in a browser) even if it wasn't found in a previous document. It would also break Cmd-E, which lets you change the search string on the fly, so, yeah, we don't want to be disabling Cmd-G.
(In reply to comment #2) > Safari seems to do the same thing Camino does, ie. highlight the new text but > the "not found" notice persists. That is not my experience - with your STR, Safari 5 immediately updates to '1 match' after pressing Cmd-G > > Maybe the solution is to disable Cmd-G when a string isn't found? > That would be horrible, and inconsistent in my experience with Mac programs, > where you can always run a "find again" on a different document (or tab or > page, in a browser) even if it wasn't found in a previous document. + 1. I frequently use Cmd-G across multiple (text) documents
> That is not my experience - with your STR, Safari 5 > immediately updates to '1 match' after pressing Cmd-G I only tried it once, must have had a typo. Sorry. Here's a radical thought: Why not just do away with the Find Bar's notice and beep on any not found. It's not like Camino gives detailed information in the Find bar the way that Firefox and Safari do, with match count etc., in which case it would be worth fixing inconsistencies and misinformation. Beeping on not found is a time-honored Mac tradition; Mail, Pages and TextEdit all beep on any not found, even when the Find window is in the foreground and the "not found" notice appears. Safari beeps on Cmd-G not found, Chrome beeps on any not found. Beeping is subject to user configuration in terms of volume and Universal Access, whereas the notice requires your eyes to focus on a place on the screen that really isn't used for anything else. It would solve Bug 546873 which is like an opposite problem to this one. And would address the misinformative notice when wrap fails that I described in Bug 546895 (duped to the wrap bug).
Speaking of overhauling "traditional" Find UI: http://blog.latenightsw.com/?p=393 is worth a look.
So, the buttons eventually call FBC's doFindForwards:, which does [mStatusText setStringValue:(success ? NSLocalizedString(@"", nil) : NSLocalizedString(@"TextNotFound", nil))]; http://mxr.mozilla.org/camino/source/camino/src/find/FindBarController.mm#207 What I think needs to be done here is that *if the Find Bar is showing* and Cmd-G/Cmd-Shift-G is called, then either CHBV's findInPage: or BWC's findActions: needs to call that same code. BWC's mBrowserView (really a BrowserWrapper) knows its mFindBarController, so BWC is probably the easier of the two places to do this and push the call down the pipes to FBC. This is actually going to be *easier* to fix than bug 417794, I think.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Another circumstance where this bug might turn up is if JS posts more content to a page after you've searched already.) Ugh, I missed that CHBV has findActions: too; it handles Cmd-G/Cmd-Shift-G when the Find Bar is *not* showing (BWC's findActions: is only called if the Find Bar *is* showing). CHBV doesn't know its BrowserWrapper, as far as I can tell, so this is probably going to need fixing in the same way as bug 417794, with a notification. This patch basically implements the BWC part; it doesn't compile because of the constants. The constants need to be known to BrowserWindowController, CHBrowserView, and FindBarController; none of them share any common header imports. So I have no idea where to declare them :-( (Also, I love how the find methods are in the middle of a series of throbber-related methods :P Yay organization-by-accretion :P )
I suppose we could push the notification-sending all the way down to CHBV's findInPage: (which both BWC's and CHBV's findActions: call, and I guess FBC's doFindForwards:, too, although I don't understand the mFinder stuff, which goes to BrowserWrapper where it somehow calls a "(id<Find>)mBrowserView", whatever that is), but CHBV and FBC still share no headers. And I have no idea what the architecturally correct location might really be, but I guess it seems a little strange that the embedding view is sending a notification that the UI layer millions of miles away needs (although I guess that's what we're proposing for bug 417794).
(In reply to comment #8) > Ugh, I missed that CHBV has findActions: too; it handles Cmd-G/Cmd-Shift-G > when the Find Bar is *not* showing (BWC's findActions: is only called if the > Find Bar *is* showing). :sigh: Yet another drawback of designing-while-tired; we don't care what happens when the Find Bar isn't showing :P And, in addition, we don't want to update every Find Bar instance when the current tab's find status changes, only the current tab's Find Bar. I have a lovely working implementation of completely the wrong thing now (although half of it might be useful for bug 417794) :P I'll think about this again tomorrow with a fresher head, but I think the right solution is to go back to comment 7/comment 8, and have BWC's findActions: call something like: [[mBrowserView mFindBarController] updateStatusText:success] Dunno if that will compile or not, but that's the idea.
This was the notification-based fix I worked up last night; Ilya suggested adding a new file for the constants, which this does (less the project change, but it compiles anyway). This was the wrong fix because it updates all FBCs, regardless of whether the Find Again/Find Previous happened in the tab with a given FBC.
(In reply to comment #8) > Ugh, I missed that CHBV has findActions: too; it handles Cmd-G/Cmd-Shift-G > when the Find Bar is *not* showing (BWC's findActions: is only called if the > Find Bar *is* showing). That comment wss not quite right, either. 1) FindBarController handles Find Again/Find Previous via the Find Bar buttons, and this is currently the only way the status string gets updated. 2) BWC handles Find Again/Find Previous from the menu/keyboard *only* if the Find Bar is *present* and *focused*. 3) CHBV handles Find Again/Find Previous from the menu/keyboard in the following situations: a) If the Find Bar is *not* present b) If the Find Bar is *present* and *not focused*. To fix this bug, we care about 2) and 3b), for the current BrowserWrapper/CHBV only. The attached patch only fixes 2). > CHBV doesn't know its BrowserWrapper, as far as I can tell, so this is > probably going to need fixing in the same way as bug 417794, with a > notification. The notification-based fix I worked up last night isn't the right fix for this bug, because it causes all FBCs to update their status text, regardless of whether the FindAgain/FindPrevious happened in their tabs or not. So, to fix the entire bug, either CHBV needs to be able to know its parent BrowserWrapper (so that the BW can check to see if the Find Bar is visible, and, if so, tell it to update the status text), or there needs to be a way to only send or act upon notifications from the current CHBV/for the current FBC, or some other method someone else proposes.
(In reply to comment #12) > or there needs to be a way to > only send or act upon notifications from the current CHBV/for the current > FBC [4:56pm] ilya: yeah, sending a notification and filtering on the sender sounds reasonable [4:57pm] sauron: is what i wrote the right way to do that? [4:58pm] ilya: I think so, yeah [4:58pm] sauron: woot! [4:58pm] sauron: object: <CHBrowserView: 0x1634ed60> CHBV: <CHBrowserView: 0x1634ed60> [4:59pm] sauron: object: <CHBrowserView: 0x1298e8f0> CHBV: <CHBrowserView: 0x1634ed60> (for Find in another tab) [4:59pm] sauron: sweet :) I'll try to clean that fix up tonight.
Attached patch Fix, v2 (obsolete) — — Splinter Review
OK, this is the full patch. Lisa, if you're interested, I can spin a build with this for you.
Assignee: nobody → alqahira
Attachment #600287 - Attachment is obsolete: true
Attachment #600504 - Attachment is obsolete: true
Attachment #600509 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #600590 - Flags: review?(cl-bugs-new2)
Attached patch Fix, v2.0.1 (obsolete) — — Splinter Review
This version just makes all of the method comment punctuation consistent, which I noticed when looking into bug 417794.
Attachment #600590 - Attachment is obsolete: true
Attachment #600616 - Flags: review?(cl-bugs-new2)
Attachment #600590 - Flags: review?(cl-bugs-new2)
If I'm not too late here, yes, I could look at a build but only if you think it's worth your time to spin it -- I can see how busy things are around here. :-)
Comment on attachment 600616 [details] [diff] [review] Fix, v2.0.1 Looks good to me. r=cl.
Attachment #600616 - Flags: review?(cl-bugs-new2) → review+
Comment on attachment 600616 [details] [diff] [review] Fix, v2.0.1 Stuart, there was some debate on irc about the proper way/place to handle the constants (given that the two files that need them share no header imports and otherwise don't seem to need to know about each other). It seemed silly to me have an .h *and* corresponding .mm for just two constants, where an (new) .h plus the implementation in an existing file would do, but others argued that the .h + .mm was better. We have cases of both ways in the tree right now.
Attachment #600616 - Flags: superreview?(stuart.morgan+bugzilla)
Comment on attachment 600616 [details] [diff] [review] Fix, v2.0.1 Review of attachment 600616 [details] [diff] [review]: ----------------------------------------------------------------- Overall design is good, just needs a bit of implementation rearranging. Make a new impl file for the constants. Files are cheap, and it's a PITA to have to figure out what file a constant is defined in. ::: Camino.xcodeproj/project.pbxproj @@ +3052,5 @@ > DE74F74D0AB25EBC00FD1D5B /* FeedServiceController.h */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.c.h; name = FeedServiceController.h; path = src/browser/FeedServiceController.h; sourceTree = "<group>"; }; > DE74F7540AB25EDA00FD1D5B /* FeedServiceController.mm */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.cpp.objcpp; name = FeedServiceController.mm; path = src/browser/FeedServiceController.mm; sourceTree = "<group>"; }; > DE74F7610AB2612700FD1D5B /* English */ = {isa = PBXFileReference; lastKnownFileType = wrapper.nib; name = English; path = resources/localized/English.lproj/OpenFeed.nib; sourceTree = "<group>"; }; > DE74F7640AB2618200FD1D5B /* feed.tiff */ = {isa = PBXFileReference; lastKnownFileType = image.tiff; name = feed.tiff; path = resources/images/chrome/feed.tiff; sourceTree = SOURCE_ROOT; }; > + DE81DB1814F87214007ED005 /* FindNotifications.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = FindNotifications.h; path = src/find/FindNotifications.h; sourceTree = SOURCE_ROOT; }; The notifications should live in embedding/ not find/, since embedding/ is what vends the notifications. ::: src/find/FindBarController.mm @@ +292,5 @@ > +- (void)updateFindStatusText:(NSNotification *)notification; > +{ > + BOOL success = [[[notification userInfo] > + objectForKey:kFindStatusNotificationSuccessKey] boolValue]; > + if ([notification object] == [mContentView browserView]) Do this check first as an early return; no need to get the BOOL if the notification isn't for you. So: if (<object> != <browser view>) return; BOOL success = ... @@ +293,5 @@ > +{ > + BOOL success = [[[notification userInfo] > + objectForKey:kFindStatusNotificationSuccessKey] boolValue]; > + if ([notification object] == [mContentView browserView]) > + [mStatusText setStringValue:(success ? NSLocalizedString(@"", nil) : NSLocalizedString(@"TextNotFound", nil))]; Don't copy/paste this code; made a - (void)updateStatusTextWithSuccess:(BOOL)success) helper method and call it from both places that update it.
Attachment #600616 - Flags: superreview?(stuart.morgan+bugzilla) → superreview-
OK, this should have all of Stuart's comments addressed. (In reply to Stuart Morgan from comment #19) > Make a new impl file for the constants. Files are cheap, and it's a PITA to > have to figure out what file a constant is defined in. OK; that's a really solid reason. > > + BOOL success = [[[notification userInfo] > > + objectForKey:kFindStatusNotificationSuccessKey] boolValue]; > > + if ([notification object] == [mContentView browserView]) > > Do this check first as an early return; no need to get the BOOL if the > notification isn't for you. So: That certainly makes sense in retrospect! > > + [mStatusText setStringValue:(success ? NSLocalizedString(@"", nil) : NSLocalizedString(@"TextNotFound", nil))]; > > Don't copy/paste this code; made a > - (void)updateStatusTextWithSuccess:(BOOL)success) > helper method and call it from both places that update it. I had considered doing that originally, but I ended up deciding for only two uses it was a toss-up. But it's there now :-) (Also, Lisa, I haven't forgotten about getting you a build; I've just been tied up with lots of other things :-( )
Attachment #600616 - Attachment is obsolete: true
Attachment #607311 - Flags: superreview?(stuart.morgan+bugzilla)
This is the same as v2.1, except it also includes the project changes that got lost in v2.1 and updates the new files to use MPL2 boilerplate.
Attachment #607311 - Attachment is obsolete: true
Attachment #610266 - Flags: superreview?(stuart.morgan+bugzilla)
Attachment #607311 - Flags: superreview?(stuart.morgan+bugzilla)
Comment on attachment 610266 [details] [diff] [review] Fix, v2.1.1 with sr comments addressed, missing project diff, and MPL2 sr=smorgan
Attachment #610266 - Flags: superreview?(stuart.morgan+bugzilla) → superreview+
http://hg.mozilla.org/camino/rev/d632504bba98
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [camino-2.1.3]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: