If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Find panel inconsistent with other cocoa apps

RESOLVED FIXED in Camino1.0

Status

Camino Graveyard
General
--
enhancement
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: Richard Schreyer, Assigned: Mike Pinkerton (not reading bugmail))

Tracking

({access})

unspecified
Camino1.0
PowerPC
Mac OS X
access

Details

Attachments

(3 attachments, 12 obsolete attachments)

60.51 KB, image/jpeg
Details
2.24 KB, application/octet-stream
Details
5.60 KB, patch
S Woodside
: review+
Usul
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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.
(Reporter)

Comment 1

15 years ago
Created attachment 93764 [details] [diff] [review]
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

15 years ago
cool, ->pinkerton
Assignee: saari → pinkerton

Updated

15 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 3

15 years ago
Created attachment 93800 [details] [diff] [review]
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.
(Reporter)

Updated

15 years ago
Attachment #93764 - Attachment is obsolete: true

Updated

15 years ago
Keywords: patch, review
Summary: Find panel inconsistant with other cococa apps → Find panel inconsistent with other cocoa apps

Comment 4

15 years ago
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? :-)
(Reporter)

Comment 5

15 years ago
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.
(Reporter)

Comment 6

15 years ago
Created attachment 93872 [details] [diff] [review]
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?
Attachment #93800 - Attachment is obsolete: true
(Reporter)

Comment 7

15 years ago
Created attachment 93873 [details]
Nib files

Comment 8

15 years ago
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.
(Reporter)

Comment 9

15 years ago
Created attachment 94341 [details] [diff] [review]
Patch against current CVS
Attachment #93872 - Attachment is obsolete: true
(Reporter)

Comment 10

15 years ago
Created attachment 94342 [details]
Updated nibs
Attachment #93873 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → Chimera0.7

Comment 11

15 years ago
Can we have a screenshot of the new Find dialog please?
(Reporter)

Comment 12

15 years ago
Created attachment 94956 [details]
screenshot of find panel and menu

Comment 13

15 years ago
Do we really want to tie up Command E,E,F,G for Find stuff?
(Reporter)

Comment 14

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

Comment 15

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

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

15 years ago
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.
Keywords: access

Comment 18

15 years ago
So are the keyboard commands all that's holding this up?
(Reporter)

Comment 19

15 years ago
Created attachment 95532 [details]
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.
Attachment #94341 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Attachment #94342 - Attachment is obsolete: true
(Reporter)

Comment 20

15 years ago
Created attachment 96405 [details]
Updated to work after the file reorg
Attachment #95532 - Attachment is obsolete: true

Comment 21

15 years ago
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.
(Reporter)

Comment 22

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

15 years ago
Created attachment 102466 [details] [diff] [review]
Patch.

Comment 24

15 years ago
Created attachment 102467 [details]
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

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

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

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

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

Comment 29

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

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

Comment 31

15 years ago
See bug 176160 bug 185916

Comment 32

15 years ago
So what's the deal with this?  I've got a patch that fixes most of this as well
as bug 188657
(Assignee)

Comment 33

14 years ago
pushing back since we're still arguing about key equivs
Target Milestone: Camino0.7 → Camino1.0

Comment 34

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

Comment 35

14 years ago
yeah, find previous seems to be moving to cmd-shift-G. we should do the same here.
Created attachment 133051 [details] [diff] [review]
updated patch

I've updated the patch in attachment 102466 [details] [diff] [review] to mesh with current layouts.
new nib coming in a sec.
Attachment #96405 - Attachment is obsolete: true
Attachment #102466 - Attachment is obsolete: true
Attachment #102467 - Attachment is obsolete: true
Created attachment 133052 [details]
Updated Nib

I've just cleaned up the NIB file previously posted.  This guy is ready for
review.
Attachment #133051 - Flags: review?(haasd)
Sorry for the spam - but ideally the patch in bug 221845 would land at the same
time as this one.
Depends on: 221845
Created attachment 134079 [details] [diff] [review]
updated patch

make sure we return an empty string, rather than nil, if we don't have a string
on the global pasteboard.
Attachment #133051 - Attachment is obsolete: true
Attachment #134079 - Flags: review?
Attachment #133051 - Flags: review?(haasd)

Comment 40

14 years ago
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.

Updated

14 years ago
Attachment #134079 - Flags: review? → review+

Comment 41

14 years ago
Comment on attachment 134079 [details] [diff] [review]
updated patch

needs another review I guess
Attachment #134079 - Flags: review?

Comment 42

14 years ago
Please also make sure that you fix the bug where the find string isn't retained
across tabs.
comment 42: This patch plus the patch for bug 221845 (which landed yesterday) I
believe fixes that problem.

Comment 44

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

14 years ago
Created attachment 135298 [details] [diff] [review]
patch, updated again

one line changed
Attachment #134079 - Attachment is obsolete: true

Updated

14 years ago
Attachment #135298 - Flags: review+

Updated

14 years ago
Attachment #135298 - Flags: review?

Updated

14 years ago
Attachment #134079 - Flags: review?

Comment 46

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

14 years ago
Turns out I can't edit the review thingy, so I'm just writing it here:
r=josha@mac.com
(Assignee)

Comment 48

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

14 years ago
Perhaps by the patch in bug 221845?
Attachment #135298 - Flags: review? → superreview?(pinkerton)
(Assignee)

Comment 50

14 years ago
yup, that sure does the trick. landing with the nib fixup for long button names
and some small code tweaks on style.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Assignee)

Updated

14 years ago
Attachment #135298 - Flags: superreview?(pinkerton) → superreview+
You need to log in before you can comment on or make changes to this bug.