160771 - Find panel inconsistent with other cocoa apps

Status: RESOLVED FIXED

(Camino Graveyard :: General, enhancement)

Description

[Richard Schreyer]

Chimera's find panel works quite differently than in most other cooca apps, and
doesn't support some common features, such as Use Selection for Find and Find
Previous menu items/shortcuts.

Comment 1

[Richard Schreyer]

Attachment: Initial Find patch

I've tried to address most of these issues:

Added the complete 5 item Find submenu under Edit (couldn't figure out how to
do scroll to selection, so it's always disabled.).

The Find panel itself works as expected, the find string is properly read and
written to the Find Pasteboard when necessary, it defaults to Ignore Case and
Wrap being on, if the user hits return in the find panel, it closes if
something is found, etc.

I've had to change some keyboard shortcuts to stay consistent with other apps. 
Add Bookmark (was cmd-D) is cmd-B, and there is no longer a shortcut for Show
Source.  Anyone want to suggest better alternates?

Comment 2

[saari (gone)]

cool, ->pinkerton

Comment 3

[Richard Schreyer]

Attachment: Find patch revision

The origonal patch instanated the FindDlgController right from the main nib,
and the menu items were hooked up directly.  I've changed it to match how all
the other menu items are handled.

Comment 4

[Max Horn]

A patch / diff doesn't work for the nib files, as they contain a binary part. You should tar & 
gzip the modified .nibs and attach them seperatly.

Anyway, this should save me some work - I just started my own patch for this, but it's only 
halfway done and your looks better anyway (it seems you also took a look at the 
TextFinder.m of TextEdit? :-)

Comment 5

[Richard Schreyer]

Yup, I started with TextFinder and went from there.

I'm trying to finish it up by getting Scroll to Selection working.  I see if I
can finish that before uploading patch+tar files.

Comment 6

[Richard Schreyer]

Attachment: Patch without nibs

I still couldn't figure out how to do 'Scroll to Selection'.  Would someone
else care to take a shot at it?

Comment 7

[Richard Schreyer]

Attachment: Nib files

Comment 8

[Max Horn]

This patch breaks slightly for me in current CVS. I was able to re-apply it, but maybe you 
can remake it against current CVS, Richard? That would help it being put into CVS I 
guess.

Comment 9

[Richard Schreyer]

Attachment: Patch against current CVS

Comment 10

[Richard Schreyer]

Attachment: Updated nibs

Comment 11

[Simon Fraser [no longer active]]

Can we have a screenshot of the new Find dialog please?

Comment 12

[Richard Schreyer]

Attachment: screenshot of find panel and menu

Comment 13

[Simon Fraser [no longer active]]

Do we really want to tie up Command E,E,F,G for Find stuff?

Comment 14

[Richard Schreyer]

Those shortcuts are very consistent across apps (at least, cocoa apps).  If we
have problems with this, we can tweak them later.

Comment 15

[Steve Smart]

Command F and G are reserved for Find and Find Again. The rest we can use
elsewhere if we need to, but the ones he has put in are the standard ones we
should use if we shortcuts for those lesser commands at all.

However, when I look at this objectively, why weren't we using cmd-B for
bookmark in the first place? The only reason I can find is so that we're the
same as Netscape.

If we are going to take up these shortcuts, I think we should make show
bookmarks shift-command-B and view source command-U.

Comment 16

[Kathleen :Brade]

As much as I want these for myself, I'm not sure it'd be a good idea.
In particular, I am concerned about people who expect Command-D to add a bookmark

For grabbing a selection (Command-E), which do we expect more users to do:
 View Source (currently uses Command-E)
 Use Selection for Find

Comment 17

[Max Horn]

I am for this patch, even if it ties up some shortcuts. I find myself all the 
time trying to Cmd-E and Cmd-G in IE and Chimera and it is very 
annoying that it doesn't work as expected. Well, in Chimera it's OK now 
since I use this patch all the time :-)

BTW, the look of the Find dialog didn't change too much, it's more what's 
under the hood (and the new menu items) what is important.

Cmd-D if at all traditionally is Duplicate. And used to be in the HIG as this 
for a long time. No matter how we use it somebody will be confused.

And we can just as well use Cmd-U or Cmd-Shift-V (Mozilla/OmniWeb) for 
View Source. Cmd-E for View Source is in no way logical, the only thing 
that is giving it any backing is the use in IE, which IMHO was a bad thing 
in the first place.

Comment 18

[Prachi Gauriar]

So are the keyboard commands all that's holding this up?

Comment 19

[Richard Schreyer]

Attachment: Patch against current CVS

*sigh*
CVS broke it again.  and now it's fixed.  again.

I hope it isn't the keyboard shortcuts that are keeping this from being landed.
 They're only a small part of what this patch adds, and they're trivial to
change later.

Comment 20

[Richard Schreyer]

Attachment: Updated to work after the file reorg

Comment 21

[Carlos Salinas]

I haven't had a chance to try the new find panel but here are some questions:

1. Does "Enter Selection [in find pasteboard]" work without having to display
the find panel?
   It's useful to be able to select something, cmd-e to enter the selection into
the global find pasteboard, and then switch to a different app and apply the
find there.

2. If I type something into the find panel and press return, is the find panel
dismissed?
   This is a subtle but useful feature of traditional Cocoa apps that supports a
keyboard workflow. One can cmd-f (display the find panel and the cursor is
already in the find text field), type something, then press return to perform
the find and dismiss the find panel. The usefulness is that the find panel goes
away so that you can actually see what is found. (If nothing is found, the Find
panel is not dismissed.)

   Note: Clicking the "Find" button does NOT dismiss the Find panel.

---
Also "scroll found item" to visible is extremely important. Find's not much use
if you can't see what you've found.

Comment 22

[Richard Schreyer]

1: Yes.
2: Yes.  If you hit return, the find panel is dismissed.  If you click the find
button, the panel stays.

It does scroll to the found item when you do a find.  The problems I was
describing are with the 'scroll to selection' menu item, which doesn't involve
finding something new at all.  I actually have that working (I believe) in the
most recent patch.

I just returned from a week in hawaii, so I have no idea if that patch still
works.  It might be a few days before I can get around to getting things up to date.

Comment 23

[Bill Bumgarner]

Attachment: Patch.

Comment 24

[Bill Bumgarner]

Attachment: Modified NIB file.

Modified the nib file only slightly;  target/action of field set to
findAndOrderOut:.  Field set to only send action on 'enter' key -- not on end
editing.

Comment 25

[Bill Bumgarner]

I didn't find this conversation until after I did my 'patch', which seems to be
incomplete in comparison to what had been done.  However, I *think* I'm working
against the appropriate branch of development such that I should see the changes
as described?

If someone can point me to the latest version of this stuff, I could put some
effort into cleaning up any remaining issues (as opposed to continuing to
reinvent the wheel).

Comment 26

[Mark Mentovai]

I'm not sure what the status of this is (the patch doesn't seem to be present in
101604) but the trail doesn't show this, I'm not sure if it's been considered,
and it seems to be related:

Command-period should close the find panel when it's open.

Comment 27

[Bill Bumgarner]

Actually, escape should close the panel.   The current behavior of the find
panel annoys me enough that I'm perfectly willing [as I tried before, but
without much success] to put some effort into moving this along... 

What can I do to move this closer to being 'in the tree'?

Also:

<escape> should close the find panel.  I believe cmd-. has been deprecated (but,
in any case, <escape> is the standard way to go).

cmd-d generally does a reverse find in most [cocoa] applications.  It is the
behavior I expect, but I can understand why some would look to cmd-d to make a
bookmark.

The key thing is to make the panel go away when the user hits <cr> or cmd-g to
initiate/continue a find.   Secondary to that, interacting cleanly with the
NSFindPasteboard is of great importance.

Comment 28

[Stephane Moureau]

Shift-Command-G Find Previous
http://developer.apple.com/techpubs/macosx/Essentials/AquaHIGuidelines/AHIGUserInput/Creating_Yo_Equivalents.html

Comment 29

[Bill Bumgarner]

Excellent.  And it is even a recently updated document.   shift-cmd-G makes much
more sense than did the 'traditional' cmd-g.

Words, words, words... what needs to be done to convert all of this talk into
code?  I'll do it-- it isn't hard and I have done it a bazillion times before--
but I don't want to write code/create NIBs that will never be used....

Comment 30

[Simon Fraser [no longer active]]

It's Command-D (in Project builder) that drives me mad. I wish the
Command-shift-G thing was used everywhere.

Comment 31

[Stephane Moureau]

See bug 176160 bug 185916

Comment 32

[Anoop Ranganath]

So what's the deal with this?  I've got a patch that fixes most of this as well
as bug 188657

Comment 33

[Mike Pinkerton (not reading bugmail)]

pushing back since we're still arguing about key equivs

Comment 34

[S Woodside]

A little bird tells me that Cmd-Shift-G will be the new standard as per the HIG
guidelines that Stephan referenced. Apparently if you have the Panther seed you
can see it there (check TextEdit, it uses Cmd-D and refuses Cmd-Shift-G in 10.2.6).

Comment 35

[Mike Pinkerton (not reading bugmail)]

yeah, find previous seems to be moving to cmd-shift-G. we should do the same here.

Comment 36

[David Haas (gone, not reading mail)]

Attachment: updated patch

I've updated the patch in attachment 102466 [details] [diff] [review] to mesh with current layouts.
new nib coming in a sec.

Comment 37

[David Haas (gone, not reading mail)]

Attachment: Updated Nib

I've just cleaned up the NIB file previously posted.  This guy is ready for
review.

Comment 38

[David Haas (gone, not reading mail)]

Sorry for the spam - but ideally the patch in bug 221845 would land at the same
time as this one.

Comment 39

[David Haas (gone, not reading mail)]

Attachment: updated patch

make sure we return an empty string, rather than nil, if we don't have a string
on the global pasteboard.

Comment 40

[S Woodside]

I'm nitpicking, but I think that this method:
-(BOOL) find:(BOOL)searchBack
should have a better name, e.g.
-(BOOL) findAndSearchBackwards:(BOOL)searchBack

Also, I noticed that the search doesn't wrap around as it should. However that
is NOT a regression caused by this patch. Therefore, I will review+ but please
file a bug for that wraparound problem.

Comment 41

[S Woodside]

Comment on attachment 134079 [details] [diff] [review]
updated patch

needs another review I guess

Comment 42

[Simon Fraser [no longer active]]

Please also make sure that you fix the bug where the find string isn't retained
across tabs.

Comment 43

[David Haas (gone, not reading mail)]

comment 42: This patch plus the patch for bug 221845 (which landed yesterday) I
believe fixes that problem.

Comment 44

[Josh Aas]

I tried to review this patch, but it doesn't apply for me on the source from
11/10/03:

patching file camino/src/find/FindDlgController.h
patching file camino/src/find/FindDlgController.mm
Hunk #1 FAILED at 36.
1 out of 1 hunk FAILED -- saving rejects to file
camino/src/find/FindDlgController.mm.rej

Comment 45

[S Woodside]

Attachment: patch, updated again

one line changed

Comment 46

[Josh Aas]

Thats better... I ditto comment #40, and I have one other problem...

Clicking far to the right of the "Ignore Case" and "Wrap Around" checkboxes and
their text (in what looks like empty space) changes their state. I sometimes
click there because it looks like empty space and I want to bring the panel
forward. It is annoying that I inadvertently change the checkbox's state and
have to change it back. Could you shorten the checkbox widgets in IB so they
only extend to the length of their text?

Even if you decide not to change that I'll give my review.

Comment 47

[Josh Aas]

Turns out I can't edit the review thingy, so I'm just writing it here:
r=[e-mail]

Comment 48

[Mike Pinkerton (not reading bugmail)]

looks good, but i honestly have no idea how it works. how does the mozilla
type-ahead find string get put onto the find pasteboard? it appears to, but how?

Comment 49

[Simon Fraser [no longer active]]

Perhaps by the patch in bug 221845?

Comment 50

[Mike Pinkerton (not reading bugmail)]

yup, that sure does the trick. landing with the nib fixup for long button names
and some small code tweaks on style.

Comment 51

[Intermittent Failures Robot]

1 failures in 2940 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 1

Platform and build breakdown:

• linux1804-64: 1
  • debug: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=160771&startday=2021-07-19&endday=2021-07-25&tree=all

Comment 52

[Intermittent Failures Robot]

1 failures in 2580 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 1

Platform and build breakdown:

• linux1804-64-qr: 1
  • opt: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=160771&startday=2021-08-23&endday=2021-08-29&tree=all