cmd-F (find dialog) should focus quicksearch when organizing bookmarks

RESOLVED FIXED in Camino2.0

Status

Camino Graveyard
Bookmarks
RESOLVED FIXED
14 years ago
11 years ago

People

(Reporter: Dave Aton, Assigned: froodian (Ian Leue))

Tracking

({access, fixed1.8.1.1})

unspecified
Camino2.0
PowerPC
Mac OS X
access, fixed1.8.1.1

Details

Attachments

(1 attachment, 2 obsolete attachments)

8.00 KB, patch
Stuart Morgan
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (000000000; 0; 000 000 00 0 000000; 00000; 0000000) 00000000000000 00000000000
Build Identifier: 2004041608 (v0.7+)

Downloaded and installed Camino nightly build. Started up. Opened Bookmarks.
Opened bookmark group Mac Software using triangle, and saw VersionTracker.
Pressed Command-F and entered "version" in dialog box (without quotes), and
clicked Find Next button. Did not display the VersionTracker bookmark.

Reproducible: Always
Steps to Reproduce:
1. See Details section above.
2.
3.

Actual Results:  
No match displayed.

Expected Results:  
Showed match on "VersionTracker" bookmark.

Comment 1

14 years ago
Confirmed using Camino/2004040108. Find command operates on the page beneath the
Bookmarks editor, not the Bookmarks themselves.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

14 years ago
Yup we shouldn't allwo suers to do a find in the bookmarks manager.
(Reporter)

Comment 3

14 years ago
(In reply to comment #2)
> Yup we shouldn't allwo suers to do a find in the bookmarks manager.

I disagree. Camino should behave like Mozilla. Many users have hundreds of
bookmarks acquired over many years, or bookmarks in different folders that have
something in common. Being able to find and display those in a separate window
is essential. For me, it's a deal-breaker when considering a switch from Mozilla
to Camino.
we should disable find when the manager is present, search is done with the
search field at the bottom of the manager.
Status: NEW → ASSIGNED
Target Milestone: --- → Camino1.0
(Reporter)

Comment 5

14 years ago
(In reply to comment #4)
> we should disable find when the manager is present, search is done with the
> search field at the bottom of the manager.

IMO, a more intuitive would be to make Find open the search field and move the
cursor there, but most would be just to let the user use the Find command the
way s/he probably thinks is normal. (I don't think the average user distingushes
between "Find" and "Search.")
*** Bug 258973 has been marked as a duplicate of this bug. ***
In bug 258973 comment 4, Simon says: Yeah, I vote for just focussing the
quicksearch field.

Tidying summary, too.
Summary: Bookmarks window - Find command fails to match on existing bookmark → cmd-F (find dialog) should focus quicksearch when organizing bookmarks

Comment 8

12 years ago
(In reply to comment #7)
> In bug 258973 comment 4, Simon says: Yeah, I vote for just focussing the
> quicksearch field.

But what do you do if the Find panel is already showing? Should we hide it when
switching to bookmarks, and reshow when we leave?
*** Bug 301652 has been marked as a duplicate of this bug. ***
(In reply to comment #8)
> But what do you do if the Find panel is already showing? Should we hide it when
> switching to bookmarks, and reshow when we leave?

At first thought that seems wacky, but there's precedent of sorts with the View
Options inspector in the Finder and the Keyboard Viewer, Character Palette, and
Font panel elsewhere in the OS (they hide when switching to apps that have not
called them and reappear when switching back to the original apps).
this is a good idea, but the code would require a lot of changes to do it right (firstResponder, etc) and we still don't have a good answer for what happens when the find dialog is already present.
Target Milestone: Camino1.0 → Camino1.1

Comment 12

12 years ago
If the find window is replaced by an "in window" search bar  with the same functionality the described issue won't be a problem anymore. See #258211.
Status: ASSIGNED → NEW
Keywords: access
OS: Mac OS X 10.2 → Mac OS X 10.3
QA Contact: bookmarks
Target Milestone: Camino1.1 → Camino2.0

Updated

11 years ago
Depends on: 337144

Updated

11 years ago
No longer depends on: 337144
(Assignee)

Comment 13

11 years ago
Created attachment 245165 [details] [diff] [review]
Patch

This patch may be a bit controversial... but I like it.  The find panel is useless in the bm manager anyway.
Assignee: mikepinkerton → stridey
Status: NEW → ASSIGNED
Attachment #245165 - Flags: review?(joshmoz)
(Assignee)

Comment 14

11 years ago
Created attachment 245228 [details] [diff] [review]
Better

This just gets rid of the find dialog when the bookmark manager comes up, not when we call the IBActions for opening it.  It's the Right Way, and works if you type "about:bookmarks" (which the previous patch didn't).
Attachment #245165 - Attachment is obsolete: true
Attachment #245228 - Flags: review?(joshmoz)
Attachment #245165 - Flags: review?(joshmoz)

Updated

11 years ago
Attachment #245228 - Flags: review?(joshmoz) → review?(stuart.morgan)

Comment 15

11 years ago
Comment on attachment 245228 [details] [diff] [review]
Better

>+  if (mFindDialog != nil)
>+    [mFindDialog close];

The |if| check here doesn't do anything useful.

>-// XXX this is only used to show bm after an import

I think the reason this comment was here is that it's kind of a hack. Exposing internals like this isn't generally a good idea, and it's something I want to clean up in Camino's code, so adding more would make me sad.

Instead, how about having MC call a method in the frontmost BWC (if any) to give it a chance to handle Find if it wants to, which returns a BOOL for whether or not it did.  That gives an extensible way for BWC to do content-specific things with Find without MC having to know the details.
Attachment #245228 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 16

11 years ago
Created attachment 248480 [details] [diff] [review]
Patch

Addresses review comments.
Attachment #245228 - Attachment is obsolete: true
Attachment #248480 - Flags: review?(stuart.morgan)

Comment 17

11 years ago
Comment on attachment 248480 [details] [diff] [review]
Patch

r=me, this is much cleaner, thanks. Please document performFindCommand to explain that the return value is whether or not it was handled though.

Also, we need a follow-up bug or two on making the find panel behavior better:
- if you switch windows or tabs to an existing bookmark manager, it stays showing
- if you toggle on the bookmark manager, causing the find panel to vanish, then toggle it off again, the find panel should come back.

Despite that, I still think this behavior is still an improvement over where we are now, so I'm in favor of landing this now and addressing those issues later.
Attachment #248480 - Flags: superreview?(mikepinkerton)
Attachment #248480 - Flags: review?(stuart.morgan)
Attachment #248480 - Flags: review+
Comment on attachment 248480 [details] [diff] [review]
Patch

sr=pink

regarding performFindCommand, it seems like it should have a better name, but i can't think of one. Definitely needs to be documented.
Attachment #248480 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 19

11 years ago
Checked in on trunk and MOZILLA_1_8_BRANCH.  Filed followup work as a single bug (bug 364255).
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.