60.51 KB, image/jpeg
2.24 KB, application/octet-stream
5.60 KB, patch
S Woodside: review+
Mike Pinkerton (not reading bugmail): 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.
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?
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.
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.
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?
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.
Created attachment 94341 [details] [diff] [review] Patch against current CVS
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?
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.
Created attachment 96405 [details] Updated to work after the file reorg
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.
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.
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.
Shift-Command-G Find Previous http://developer.apple.com/techpubs/macosx/Essentials/AquaHIGuidelines/AHIGUserInput/Creating_Yo_Equivalents.html
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
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.
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.
Created attachment 133052 [details] Updated Nib I've just cleaned up the NIB file previously posted. This guy is ready for review.
Sorry for the spam - but ideally the patch in bug 221845 would land at the same time as this one.
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.
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 on attachment 134079 [details] [diff] [review] updated patch needs another review I guess
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
Created attachment 135298 [details] [diff] [review] patch, updated again one line changed
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: firstname.lastname@example.org
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?
yup, that sure does the trick. landing with the nib fixup for long button names and some small code tweaks on style.