Closed Bug 160771 Opened 22 years ago Closed 21 years ago

Find panel inconsistent with other cocoa apps

Categories

(Camino Graveyard :: General, enhancement)

PowerPC
macOS
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Camino1.0

People

(Reporter: rwschreyer, Assigned: mikepinkerton)

References

Details

(Keywords: access)

Attachments

(3 files, 12 obsolete files)

60.51 KB, image/jpeg
Details
2.24 KB, application/octet-stream
Details
5.60 KB, patch
sbwoodside
: review+
Usul
: review+
mikepinkerton
: superreview+
Details | Diff | Splinter Review
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.
Attached patch Initial Find patch (obsolete) — Splinter Review
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?
cool, ->pinkerton
Assignee: saari → pinkerton
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Find patch revision (obsolete) — Splinter Review
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.
Attachment #93764 - Attachment is obsolete: true
Keywords: patch, review
Summary: Find panel inconsistant with other cococa apps → Find panel inconsistent with other cocoa apps
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? :-)
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.
Attached patch Patch without nibs (obsolete) — Splinter Review
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
Attached file Nib files (obsolete) —
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.
Attached patch Patch against current CVS (obsolete) — Splinter Review
Attachment #93872 - Attachment is obsolete: true
Attached file Updated nibs (obsolete) —
Attachment #93873 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Target Milestone: --- → Chimera0.7
Can we have a screenshot of the new Find dialog please?
Do we really want to tie up Command E,E,F,G for Find stuff?
Those shortcuts are very consistent across apps (at least, cocoa apps).  If we
have problems with this, we can tweak them later.
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.
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
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.
So are the keyboard commands all that's holding this up?
Attached file Patch against current CVS (obsolete) —
*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
Attachment #94342 - Attachment is obsolete: true
Attached file Updated to work after the file reorg (obsolete) —
Attachment #95532 - Attachment is obsolete: true
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.
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.
Attached patch Patch. (obsolete) — Splinter Review
Attached file Modified NIB file. (obsolete) —
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.
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).
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.
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.
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....
It's Command-D (in Project builder) that drives me mad. I wish the
Command-shift-G thing was used everywhere.
So what's the deal with this?  I've got a patch that fixes most of this as well
as bug 188657
pushing back since we're still arguing about key equivs
Target Milestone: Camino0.7 → Camino1.0
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).
yeah, find previous seems to be moving to cmd-shift-G. we should do the same here.
Attached patch updated patch (obsolete) — Splinter Review
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
Attached file 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
Attached patch updated patch (obsolete) — Splinter Review
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 #133051 - Flags: review?(haasd)
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.
Attachment #134079 - Flags: review? → review+
Comment on attachment 134079 [details] [diff] [review]
updated patch

needs another review I guess
Attachment #134079 - Flags: review?
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.
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
one line changed
Attachment #134079 - Attachment is obsolete: true
Attachment #135298 - Flags: review+
Attachment #135298 - Flags: review?
Attachment #134079 - Flags: review?
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.
Turns out I can't edit the review thingy, so I'm just writing it here:
r=josha@mac.com
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?
Perhaps by the patch in bug 221845?
Attachment #135298 - Flags: review? → superreview?(pinkerton)
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
Closed: 21 years ago
Resolution: --- → FIXED
Attachment #135298 - Flags: superreview?(pinkerton) → superreview+
You need to log in before you can comment on or make changes to this bug.