Closed
Bug 160771
Opened 22 years ago
Closed 21 years ago
Find panel inconsistent with other cocoa apps
Categories
(Camino Graveyard :: General, enhancement)
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.
Reporter | ||
Comment 1•22 years ago
|
||
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?
Updated•22 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 3•22 years ago
|
||
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•22 years ago
|
Attachment #93764 -
Attachment is obsolete: true
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•22 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•22 years ago
|
||
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•22 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•22 years ago
|
||
Attachment #93872 -
Attachment is obsolete: true
Reporter | ||
Comment 10•22 years ago
|
||
Attachment #93873 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → Chimera0.7
Comment 11•22 years ago
|
||
Can we have a screenshot of the new Find dialog please?
Reporter | ||
Comment 12•22 years ago
|
||
Comment 13•22 years ago
|
||
Do we really want to tie up Command E,E,F,G for Find stuff?
Reporter | ||
Comment 14•22 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•22 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•22 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•22 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.
Comment 18•22 years ago
|
||
So are the keyboard commands all that's holding this up?
Reporter | ||
Comment 19•22 years ago
|
||
*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•22 years ago
|
Attachment #94342 -
Attachment is obsolete: true
Reporter | ||
Comment 20•22 years ago
|
||
Attachment #95532 -
Attachment is obsolete: true
Comment 21•22 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•22 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•22 years ago
|
||
Comment 24•22 years ago
|
||
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•22 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•22 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•22 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•22 years ago
|
||
Shift-Command-G Find Previous http://developer.apple.com/techpubs/macosx/Essentials/AquaHIGuidelines/AHIGUserInput/Creating_Yo_Equivalents.html
Comment 29•22 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•22 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•22 years ago
|
||
See bug 176160 bug 185916
Comment 32•22 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•21 years ago
|
||
pushing back since we're still arguing about key equivs
Target Milestone: Camino0.7 → Camino1.0
Comment 34•21 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•21 years ago
|
||
yeah, find previous seems to be moving to cmd-shift-G. we should do the same here.
Comment 36•21 years ago
|
||
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
Comment 37•21 years ago
|
||
I've just cleaned up the NIB file previously posted. This guy is ready for review.
Updated•21 years ago
|
Attachment #133051 -
Flags: review?(haasd)
Comment 38•21 years ago
|
||
Sorry for the spam - but ideally the patch in bug 221845 would land at the same time as this one.
Depends on: 221845
Comment 39•21 years ago
|
||
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
Updated•21 years ago
|
Attachment #134079 -
Flags: review?
Updated•21 years ago
|
Attachment #133051 -
Flags: review?(haasd)
Comment 40•21 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•21 years ago
|
Attachment #134079 -
Flags: review? → review+
Comment 41•21 years ago
|
||
Comment on attachment 134079 [details] [diff] [review] updated patch needs another review I guess
Attachment #134079 -
Flags: review?
Comment 42•21 years ago
|
||
Please also make sure that you fix the bug where the find string isn't retained across tabs.
Comment 43•21 years ago
|
||
comment 42: This patch plus the patch for bug 221845 (which landed yesterday) I believe fixes that problem.
Comment 44•21 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
Updated•21 years ago
|
Attachment #135298 -
Flags: review+
Updated•21 years ago
|
Attachment #135298 -
Flags: review?
Updated•21 years ago
|
Attachment #134079 -
Flags: review?
Comment 46•21 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•21 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•21 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•21 years ago
|
||
Perhaps by the patch in bug 221845?
Updated•21 years ago
|
Attachment #135298 -
Flags: review? → superreview?(pinkerton)
Assignee | ||
Comment 50•21 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
Closed: 21 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•21 years ago
|
Attachment #135298 -
Flags: superreview?(pinkerton) → superreview+
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
You need to log in
before you can comment on or make changes to this bug.
Description
•