Closed Bug 201723 Opened 22 years ago Closed 17 years ago

Search engine "editor" (easier way to add other engines to the Search field's menu)

Categories

(Camino Graveyard :: Toolbars & Menus, enhancement, P3)

Product:
Camino Graveyard
Component:
Toolbars & Menus
Platform:
PowerPC
macOS
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Camino1.6

People

(Reporter: huskerdu, Assigned: murph)

References

Details

(Keywords: fixed1.8.1.12)

Attachments

(4 files, 10 obsolete files)

[Screenshot] Safari Downloads Window
14.81 KB, image/png
Details
[Screenshots] Trying to nail down the best look...
104.51 KB, image/png
Details
Resources, v3
9.38 KB, application/zip
Details
Patch for Check-in
89.41 KB, patch
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4b) Gecko/20030411 Chimera/0.7+ Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4b) Gecko/20030411 Chimera/0.7+ The addition of the Google search field to the toolbar is awesome. Please add Google Groups to the drop-down menu of search choices. Thanks! Reproducible: Always Steps to Reproduce: 1. 2. 3.
-->pinkerton
Assignee: brade → pinkerton
Confirming as an enhancement.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: <ENH> Add Google Groups to seach bar → Add Google Groups to Search Bar drop-down menu
Target Milestone: --- → Camino1.1
Mike could we perhaps rename this bug to something like "add other search urls to the google drop down menu". As it a thing you mentioned on the mailing list, it would perhaps be interesting to add more url's? Especially in the light of the ongoing request from users to have adit capabilities I don't see why we couldn't add soem generic often used search utils. (and because OW 5 will have a lot more than we have ) A list we can choose from: Apple Support - http://search.info.apple.com/?search=Go&amp;lr=lang_en&amp;kword=&amp;q=%s Versiontracker - http://www.versiontracker.com/mp/new_search.m?productDB=mac&amp;mode=Quick&amp;OS_Filter=MacOSX&amp;search=%s Britanica - http://www.britannica.com/search?query=%s Encyclopedia - http://www.encyclopedia.com/searchpool.asp?target=%s Wikipedia - http://en.wikipedia.org/wiki/%s Thesaurus - http://thesaurus.reference.com/search?q=%s Dictionary - http://dictionary.reference.com/search?q=%s Big Charts - http://www.britannica.com/search?query=%s Yahoo Finance - http://finance.yahoo.com/q?s=%s Google Groups - http://groups.google.com/groups?q=%s
Summary: Add Google Groups to Search Bar drop-down menu → Add other shortcuts to the Search drop-down menu.
Ok I had another look into this and think this should be the list of added items: Amazon Britanica Dictionary Ebay Google Google Groups Google Images Movie Database Search this Site Stock Charts Thesaurus VersionTracker If there is anything else missing please let me know. Otherwise I'll post a new file later.
Attached file New file with added search engines. (obsolete) —
New SearchURLList.plist file where I added the search engines mentioned in Comment #4.
why can't we just do what firefox does, and provide a link in the search box that says "Add Engines..." which takes you to a webpage with hundreds of different search engines you can add to the search box?
Is there anyway the SearchURLList.plist file can be moved to the Application Support folder to avoid having to go into the English.lproj folder and replacing the file after downloading a new nightly?
Yes i agree, this should be done. I know it's not that complicated. Just need to find a developer willing to create the patch.
Line 3048 till 3060 sould be changed to not look for the searchurllist.plist file inside the app bundle but inside the profile folder (profileDir). I don't see why else we provide users with documentatio to cahnge it if the file get's tossed on an app update.
that was line 3048 till 3060 in browserwindowcontroller.mm
Moving the location of SearchURLList.plist is bug 250386 (on which I really ought to get back to addressing the various review comments!)
(In reply to comment #8) > Yes i agree, this should be done. I know it's not that complicated. Just need to > find a developer willing to create the patch. I have a semi-working patch for this that I had been writing (mainly to experiment with Cocoa).. I can make some tweaks and post what I've got once the patch for bug 250386 goes through.
ender.mb: any update on the patch you mentioned?
QA Contact: bugzilla → toolbars
Summary: Add other shortcuts to the Search drop-down menu. → Search engine "editor" (easier way to add other engines to the Search field's menu)
Target Milestone: Camino1.1 → Camino1.2
I still have it, though I haven't touched it in quite a while so it'll definitely take some time to update for current trunk.. or whatever branch such a feature would end up on..
*** Bug 246460 has been marked as a duplicate of this bug. ***
Can anyone think of an intuitive GUI for this? I'm especially thinking of how to make new search engines addable without requiring the confusing "%s" to be shown, etc.
Assignee: mikepinkerton → nobody
One thing that would make that whole process really easy is to have a special metadata tag present in the page and that tag would contain the search address. Then the user could just enter the web address of the page containing that tag. eg: user enter: www.google.com camino load google.com in background and extract this tag from the page and add the correct search string to its list. That would require some evangelism and cooperation with other browser team:) This solution keeps the user experience easy as the user can add search engines he visits and not be bothered with the one he does not. No need to go to search engines listing page and find the one that you need. When seeing that metadata tag in the HTML, camino could display a add search engine button on the page, or at least give some way for the web designer to create such a button.
To add to my own comment, i just download opera 9 and it seem like they are able to extract the search string from the web site. I do not like the interaction model as the user has to right click the search bar to get the "add search engine" option. I would have this option in the bookmark menu and in the contextual menu for the search field. Another way to trigger it for advanced user would be to option click the search/submit button. (In reply to comment #17) > One thing that would make that whole process really easy is to have a special > metadata tag present in the page and that tag would contain the search address. > Then the user could just enter the web address of the page containing that tag. > > eg: user enter: www.google.com > camino load google.com in background and extract this tag from the page and add > the correct search string to its list. That would require some evangelism and > cooperation with other browser team:) > > This solution keeps the user experience easy as the user can add search engines > he visits and not be bothered with the one he does not. No need to go to search > engines listing page and find the one that you need. > > When seeing that metadata tag in the HTML, camino could display a add search > engine button on the page, or at least give some way for the web designer to > create such a button. >
You might want to consider adding support for OpenSearch engines, which is a newer format that is supported by Firefox 2.0 and IE7. Firefox stores engine definitions locally in that format, with a few tweaks for additional user-interface related parameters. I think you might want to add support for keyword aliases too, which should be pretty easy to implement. I think this is a preferable approach to the bookmark keywords method.
Blocks: 349994
Attachment #138610 - Attachment mime type: application/octet-stream → text/plain; charset=utf-8
I have a patch in bug 352250 that will make it very easy to get a list of all keywords, and also inject new ones. A suggestion: Make a keyword editor as phase I, then add support for search engines as phase II. This should be a fun bug for anyone who wants to exercise their interface builder muscles. :)
(In reply to comment #19) > You might want to consider adding support for OpenSearch engines, which is a > newer format that is supported by Firefox 2.0 and IE7. See bug 358798. cl
I'd like to take this. I'm nearing completion of OpenSearch discovery over on bug 358798, which would allow search engines found in this manner to be easily added. That ability would benefit greatly if it were balanced by an equally easy way to remove an engine. I'll have an implementation very shortly, but before I submit anything there are a few areas worth collecting some feedback on: In short: How should the addition of search URLs work?; Window(/panel) or sheet for the editor? Comment #16: I also prefer a unique and very user-friendly way of adding a search URL. Doing that, however, will probably hold up the bug and require a lot of time so I'm going to shoot first for getting a basic working editor in place and suggest that we add some kind of elegant-search-url-discovery mechanism later (possibly under a separate bug report). Supporting OpenSearch can make the general addition of engines defined in that format possible as well. Right now, the patch for that bug only offers to add an engine if Camino recognizes the current site advertising such a feature. With/without the above mentioned user-friendly way, should we allow users to add an engine by specifying the raw query URL? I'm only hesitant to expose this via the UI since it's technical and not all users will understand how to isolate the query variable, etc. If we'd rather not have users dealing with query URLs, how about (as Jasper suggested in comment #2) providing a pre-built group of engines which can be added. This behavior would be on par with the AppKit's customize toolbar paradigm: a set of default search engines with the ability to customize/add a certain amount of additionally provided engines. Lastly, sheet or window? An editor would be application rather than window centric, but Firefox uses a sheet for this feature and I think that works pretty well. Although, if the user is allowed to add searches by specifying a query URL, the editor should be a window since a modal sheet would prevent copying/finding the URL from the browser. Any other requests/suggestions/complaints are welcome and I will try my best to incorporate them!
Assignee: nobody → camino
Setting priority per 1.6 roadmap.
Priority: -- → P3
I've been busy working on this feature, and it's nearly completed. In the meantime, I thought I might as well report in on my progress and demonstrate what I came up with (and dare I say: get some early feedback). So, just in case anyone's curious, here is a quick overview of the search engine editor: http://seanmurph.com/camino/searchEngineEditorPreview.mov To make the "query URL" situation a bit friendlier I introduced a magnifying glass button to the right of the URL text field which allows for automatic discovery of the URL by clicking on any <input type="text" /> field in the browser. This is similar to the behavior of NSColorPanel's magnifying glass. The screencast above demonstrates how such an ability makes the addition of a search much easier and quicker. The help button in the bottom left of the add sheet can also provide easy access to documentation describing auto-discovery, how to form a query URL, etc. You will notice in the video that the cursor image does not correctly change when using the auto-discovery feature. I'm still working on getting the one I set to stick when moving around the browser view. We can then offer the following ways to add a new search engine: - Use the editor (with auto-discovery). [this bug] - OpenSearch discovery on current page. [bug 358798] And possibly: - Installing programmatically via javascript (like Firefox and IE7) [I will comment more about this soon on bug 358798] - Text field contextual menu item to create a search. [new bug?] We should probably address Jasper's additions (attachment 138610 [details]) since he took the time to submit them. Would it be advantageous if they were incorporated somehow, either including by default now that removal is an option or offering an "add popular search engine" menu choice in the editor? Favicon support for search engines was discussed over on bug 327591, which will now also dictate the availability of icons in the editor's table view. I'll have some actual code to post in the near future.
Status: NEW → ASSIGNED
Wow. This looks really impressive. Great work, Sean. Two remarks: -- Why is the magnifying glass necessary? Wouldn't it be easier (one click less), to just add the search URL, everytime the user clicks into the search field and the "add search engine" dialogue is open? -- Contextual menu item for text fields would be a very useful addition.
(In reply to comment #25) Sean your work look awesome. If possible I have only one request/suggestion based on what you have implemented so far. If you could add a context menu item to the text field context menu to allow people to add the search string from within the page instead of first having to open the customize panel that would be a great improvement.
We discussed the merits of the context menu, or lack thereof, in bug 310773.
Thanks everyone for the positive feedback and, even more importantly, the usability suggestions. We'll be sure to consider them and other relevant design issues when we make decisions over the implementation. By the time this feature is checked in, I'll have done my best to ensure the interaction is as streamlined and intuitive as possible.
Per the meeting today, here's the plan: * OpenSearch discovery will be the primary method of adding new search engines (this is bug 358798) * We will have a simple "editor" UI that will allow: * Removing search engines * Reordering the engine list * Changing the default engine (all of these are this bug) We'll continue to evaluate the need for end-users to create new search engines dynamically; if we determine there's a need, we can look at ways to make that as discoverable and consistent as possible. (Advanced users will continue to be able to add engines manually by editing the .plist.)
Patch to accomplish goals listed in comment #30. As we talked about a while back in #camino-mtg, supporting user-defined ordering of search engines required that we change the saved engines file structure. The search engine editor depends on the following entries in Localizable.strings (feel free to change any wording): /* Search Engine Editor */ "WebSearchFieldManageEnginesMenuTitle" = "Customize Searches..."; "EngineNameAlreadyExistsTitle" = "Search engine name already exists"; "EngineNameAlreadyExistsMessage" = "A search engine named \"%@\" already exists. Please specify a different name."; "RemovingAllEnginesTitle" = "Cannot remove all search engines"; "RemovingAllEnginesMessage" = "Camino needs at least one search engine to be installed."; Speaking of localization - if it'll make things easier I can make a simple command line tool to convert translated/customized SearchURLList.plists over to the new WebSearchEngines.plist format so they can be included in the ML app bundle. One question: After migrating engines from the old 'SearchURLList.plist', should we remove that file on success? Project patches and resources to follow...
Attachment #278800 - Flags: review?(stuart.morgan)
Attached patch nib and other resources (obsolete) — — Splinter Review
The project patch will look in the following locations for these files: SearchEngineEditor.nib WebSearchEngines.plist -> resources/localized/English.lproj search_editor_bottom_background.png search_editor_action.png -> resources/images/chrome The old SearchURLList.plist is no longer used and can be removed.
Attachment #278803 - Flags: review?
Attached patch Trunk project patch (obsolete) — — Splinter Review
Attachment #278804 - Flags: review?
I'm really sorry this has sat for so long. I haven't forgotten, and I've done a first pass read-through. It looks really good so far, so I'm only expecting fairly minor comments.
No problem at all Stuart. This code is all still fresh in my head since I've been working on OpenSearch, so it won't be hard to dive back in and adjust anything.
Comment on attachment 278800 [details] [diff] [review] Search engine editor and supporting files Looks great, with just some minor comments as expected. I love seeing all that search engine loading code ripped out of BWC :) >+@interface SearchEngineManager : NSObject { >+ NSMutableArray *mInstalledSearchEngines; // strong >+ NSString *mPathToSavedSearchEngines; // strong >+} In all the new classes, the names of the members should be aligned. >+- (void)setPathToSavedSearchEngines:(NSString *)newPath; Having a setter for something that will only be set once and should never change over the lifetime of the object is a bit misleading. You might also want to just have the getter actually construct, cache, and return the path (like we do in some other code), but that's your call. >+ while ((engineName = [engineEnumerator nextObject])) { >+ if ([engineName isEqualToString:kPreferredSearchEngineKey]) { >+ [self setPreferredSearchEngine:[searchEngineDictionary objectForKey:engineName]]; >+ continue; >+ } This will fire a notification to the search engine clients that the engines have changed before it has actually loaded anything. (Addressing my comments about the preferred engine below will fix this for free though.) >+ if (![self pathToSavedSearchEngines]) You use this path enough times in the load method that it would probably help readability to create a local variable for it up front. >+ #if DEBUG >+ NSLog(@"No search engines found; migrating old file detected at %@", pathToOldEngines); >+ #endif Don't indent compiler directives. >+ [[NSFileManager defaultManager] copyPath:pathToDefaultEnginesInBundle toPath:[self pathToSavedSearchEngines] handler:nil]; While we don't have an explicit line length limit, shorter lines are generally easier to read, so please break this into three :-aligned lines (and similarly any other long lines with natural breaks to them). >+ NSLog(@"Unable to copy default search engines into profile directory; using defaults from bundle (any user changes will be saved into profile)"); No need for quite so much detail in a developer log message (and if the copy fails, there's a good chance it will not actually save changes into the profile either). >+ // Make sure the preferred search engine exists. >+ if (![[self installedSearchEngineNames] containsObject:[self preferredSearchEngine]]) >+ [self setPreferredSearchEngine:[[self installedSearchEngineNames] objectAtIndex:0]]; We know it's likely that some users will edit this file; given that, I'd suggest checking for a 0 length array and having that trigger the restore from the app bundle version, since this (and other code) assumes a non-empty search array. >+ [mInstalledSearchEngines writeToFile:[self pathToSavedSearchEngines] atomically:NO]; Why non-atomic? Corrupting that file is user data loss. >+ [[NSUserDefaults standardUserDefaults] setObject:engineName forKey:kPreferredSearchEngineKey]; >+ [self installedSearchEnginesChanged]; Storing this in the user defaults has a couple of drawbacks: - Launching Camino under TC or the like will destroy the pref (assuming it's set to a non-default engine) - Profile migrations won't preserve it (and by extension it weakens the portable Camino concept) Unless there's a reason not to, I'd rather the plist be a dictionary at the top level, with the preference as one key, and the list of engines as another, so it's still in the file (but much more sane than the old layout). >+- (void)renameSearchEngineAtIndex:(unsigned)index toName:(NSString *)newEngineName s/toName:/to:/ (rename...:toName is redundant) >+ if (index >= [mInstalledSearchEngines count] || !newEngineName) If we don't trust the index, it should be checked for negative values as well. We really need an assertion macro, but that's way beyond the scope of this; for now, if you are going to do this kind of assertion-style checking, put an NSLog if something is wrong so we catch any usage mistakes rather than having actions failing silently due to higher-level code errors. >+ // If this was the default engine, update that value. >+ if ([[self preferredSearchEngine] isEqualToString:[[mInstalledSearchEngines objectAtIndex:index] valueForKey:kWebSearchEngineNameKey]]) >+ [self setPreferredSearchEngine:newEngineName]; >+ >+ [[mInstalledSearchEngines objectAtIndex:index] setValue:newEngineName forKey:kWebSearchEngineNameKey]; Grab [mInstalledSearchEngines objectAtIndex:index] once, and store it locally. >+ if (destinationIndex > [mInstalledSearchEngines count]) >+ return; Same comments about bounds and logging here. >+ [[NSNotificationCenter defaultCenter] addObserver:self >+ selector:@selector(installedSearchEnginesDidChange:) >+ name:kInstalledSearchEnginesDidChangeNotification >+ object:nil]; There should be an unregistering dealloc to balance this. I know it won't be called because it's a singleton, but not having a correct dealloc is a potential landmine if the use of this class changes in the future. >+ id selectedSearchEngine = [[mSearchEngineManager installedSearchEngines] objectAtIndex:[mSearchEnginesTableView selectedRow]]; >+ [mSearchEngineManager setPreferredSearchEngine:[selectedSearchEngine valueForKey:kWebSearchEngineNameKey]]; Since the underlying array is coming from the manager anyway, would it make sense to manage the default in terms of the index, rather than the name? That would also avoid the need to update the default when its name changes. >+ NSAlert *alert = [[NSAlert alloc] init]; Autorelease, per our usual style, here and for the other alert. >+ [alert runModal]; Shouldn't this be a sheet on the editor? >+ if ([[info draggingPasteboard] availableTypeFromArray:[NSArray arrayWithObject:kSearchEngineEditorDraggedEngineType]]) { >... >+ return YES; >+ } >+ >+ return NO; Reverse the logic here so the meat of the method isn't all in a conditional. >+ // The search sheet will not find BWC in the responder chain; explicitly set as the target. No need to justify setting a target for context menu items. >+ [manageEnginesMenuItem release]; Autorelease with the alloc.
Attachment #278800 - Flags: review?(stuart.morgan) → review-
Comment on attachment 278804 [details] [diff] [review] Trunk project patch Clearing the review request on this since it no longer applies. >Index: Camino.xcodeproj/project.pbxproj >=================================================================== > 3F44AD8705BDFB9F00CB4B08 /* generated/Info-Camino.plist */ = {isa = PBXFileReference; lastKnownFileType = text.xml; path = "generated/Info-Camino.plist"; sourceTree = "<group>"; }; >- 3F44AD8805BDFB9F00CB4B08 /* Camino.app */ = {isa = PBXFileReference; includeInIndex = 0; lastKnownFileType = wrapper.application; path = Camino.app; sourceTree = BUILT_PRODUCTS_DIR; }; >+ 3F44AD8805BDFB9F00CB4B08 /* Camino.app */ = {isa = PBXFileReference; explicitFileType = wrapper.application; includeInIndex = 0; path = Camino.app; sourceTree = BUILT_PRODUCTS_DIR; }; > 3F44AD9E05BDFB9F00CB4B08 /* Info-AppearancePrefPane.plist */ = {isa = PBXFileReference; lastKnownFileType = text.xml; path = "Info-AppearancePrefPane.plist"; sourceTree = "<group>"; }; Can we all please make sure we don't have this silly change happening when we post project patches in the future (not just murph; others have posted--and had checked in--patches where this kept changing)? AFAICT, it's an artifact of srcdir builds and Xcode trying to be helpful, but it leads to unnecessary changes and even branch/trunk getting out of sync in places where they need not ;)
Attachment #278804 - Flags: review?
(In reply to comment #31) > The search engine editor depends on the following entries in > Localizable.strings (feel free to change any wording): You can now diff Localizable.strings.in and include that in your patch :-) > /* Search Engine Editor */ > "WebSearchFieldManageEnginesMenuTitle" = "Customize Searches..."; Nit: Always use … instead of 3 periods. > "EngineNameAlreadyExistsTitle" = "Search engine name already exists"; I'm not completely happy with that one, but I don't have a better title suggestion. > "EngineNameAlreadyExistsMessage" = "A search engine named \"%@\" already > exists. Please specify a different name."; Nit: Always use “curly quotes” in strings > "RemovingAllEnginesTitle" = "Cannot remove all search engines"; > "RemovingAllEnginesMessage" = "Camino needs at least one search engine to be > installed."; I'm not perfectly happy with these, either, but again I have nothing better :( > Speaking of localization - if it'll make things easier I can make a simple > command line tool to convert translated/customized SearchURLList.plists over to > the new WebSearchEngines.plist format so they can be included in the ML app > bundle. Our l10n teams will be really greatful :-) > One question: After migrating engines from the old 'SearchURLList.plist', > should we remove that file on success? In Camino, we have typically been concerned with not causing occasional testers and early adopters dataloss, so our typical rule with destructive changes has been to leave a bit of detritus/backwards compat for at least one version (unlike Core, which ate our old cookies files if we ran trunk builds). E.g., if pink tries 1.6a1 and decides he really hates 1.6, he can go back to 1.5.x and still have his search engine for GarageBand World ;)
Attached patch Sources, v2 (obsolete) — — Splinter Review
Changes in this patch: - The preferred search engine is now stored alongside the engines in WebSearchEngines.plist. - I decided that, since the "Manage Engines..." menu item is always present, a menu prototype should be created in Interface Builder and associated with the two search fields. Then, like we do for context menus, additional items can be inserted in code based on the location of a certain tag... So, I changed -[WebSearchField setSearchEngines:] around to not wipe the entire menu out and only refresh engine list. With my OpenSearch patch, I believe a similar method of setDetectedSearchPlugins: makes sense and therefore we don't need all of this manual menu management stuck in BrowserWindowController. (WebSearchField, to me, seems to be a hybrid view-controller object so I felt it was reasonable for it take on responsibility for these actions itself.) - I moved the Editor and Manager files into their own src/websearch/ directory. I will have more classes for OpenSearch that can go in there too. Feel free to keep them elsewhere. (Note that WebSearchField files would still be in the old location). -- >Since the underlying array is coming from the manager anyway, would it make >sense to manage the default in terms of the index, rather than the name? That >would also avoid the need to update the default when its name changes. I tried to go this route, but backed away from it since I felt we'd lose more than we would gain... Right now, the preferred engine needs to be handled during a rename. Using the index, we'd have to pay attention to the preferred engine value when the engines are removed or re-ordered (and with re-ordering we would probably have to update the preferred index every time since it can change indirectly because of the ones around it moving). >If we don't trust the index, it should be checked for negative values as well. The argument to such methods is interpreted as an unsigned int, and while I know this doesn't actually prevent a negative number from being supplied (or even display a warning) it will be promoted to a very large value and the original error checking (if number >= [mInstalledSearchEngines count]) will catch it. Nevertheless, this is all implicit, so should I just throw in something more obvious? >We really need an assertion macro, but that's way beyond the scope of this; for >now, if you are going to do this kind of assertion-style checking, put an NSLog >if something is wrong so we catch any usage mistakes rather than having actions >failing silently due to higher-level code errors. I have introduced logging for every error handling situation. I don't know if we really want so much to actually appear in the release build, though. Should I use NS_ASSERTION()? Using any assertion-type macro coupled with the desire to return from the method when the error is caught means the actual checking logic needs to be performed twice. Ideally, I'd prefer a simple logging macro that is stripped from release builds, meaning we can check for the error once, optionally log a message, and then return. I felt that wrapping every single NSLog with #if DEBUG directives subtracted from the code's readability too much. The |loadSavedSearchEngineInformation| NSLogs do use the compiler directives since some of those messages will be displayed during normal operation, on launch with a fresh copy of Camino. >NSLog(@"Cannot rename search engine at index: %u (engine doesn't exist)"); Even though the index is an unsigned int, should my NSLogs format it as %d so a negative value would be easier to spot? >static const int kSeparatorBeforeManageSearchEnginesMenuItemTag = 101; I won't need this until my OpenSearch patch (since the engine list is just inserted into the beginning of the menu) but am including it now because the tag is set with the nib changes on this bug.
Attachment #278800 - Attachment is obsolete: true
Attachment #278803 - Attachment is obsolete: true
Attachment #278804 - Attachment is obsolete: true
Attachment #287130 - Flags: review?(stuart.morgan)
Attachment #278803 - Flags: review?
Attached file Resources, v2 (obsolete) —
New for this version: - WebSearchEngines.plist updated to new format. - Contains a new BrowserWindow.nib. Added a WebSearchField menu prototype containing the "Manage Search Engines" item. This will need to be localized. The files should go in the following locations: nibs WebSearchEngines.plist -> resources/localized/English.lproj search_editor_bottom_background.png search_editor_action.png -> resources/images/chrome The old SearchURLList.plist is no longer used and can be removed.
Attached patch Project Patch, v2 (obsolete) — — Splinter Review
I added a new group under classes, called Web Search. Like I mentioned, I'll have a few more files for OpenSearch and figured a new group would help organize all of this search stuff better than scattering some in Application/ and some in UI Components/.
Comment on attachment 287130 [details] [diff] [review] Sources, v2 The one thing I noticed here is that changes to the default engine don't immediately change the selected engine; you have to either select your new default engine manually or open a new browser window. I would expect that if the current engine is the current default, setting a new engine to be the new default would also make that engine the selected engine afterwards. Double-clicking to rename didn't jump out at me, despite it being a list view. I wonder if that will trip up many of our users, too? In addition, hitting return doesn't make the selected engine's name editable. That means renaming is not accessible to the keyboard. The other thing I'm concerned about is how the "status bar" is going to look on 10.5. Otherwise, nice to see us moving our search UI out of the dark ages :)
(In reply to comment #42) > The one thing I noticed here is that changes to the default engine don't > immediately change the selected engine; you have to either select your new > default engine manually or open a new browser window. I was worrying too much about not messing with the current selection. I think what you mentioned here... > I would expect that if the current engine is the current default, setting a new > engine to be the new default would also make that engine the selected engine > afterwards. ...is the best plan of action (preserving the selection only if it's something other than the default). > Double-clicking to rename didn't jump out at me, despite it being a list view. > I wonder if that will trip up many of our users, too? Good point. I can add a rename item to our contextual and action menus, which will deal with the currently selected engine. > In addition, hitting return doesn't make the selected engine's name editable. > That means renaming is not accessible to the keyboard. Right again. I can definitely add that. > The other thing I'm concerned about is how the "status bar" is going to look on > 10.5. Yeah, this did cross my mind too. I'd be glad to update it with a darker, leopard-like look (which will match the title bar better as well). We'd need to be like the browser window's status bar and present an appropriate style depending on which OS we're running on. Then, given this need for different styles, I'd probably stray away from using image resources and just do something in code. One possibility is to style it like Safari 3's downloads window (attached), and throw my action/gear button in the bottom left. However, the white glossy status bar I used here isn't completely gone in Leopard apps - Mail.app, for one, still features it. I am also thinking that the editor should just be a window, instead of a panel, because right now the title bar is thinner than the status bar, and I think it'd look much better if these "borders" were balanced in size.
Comment on attachment 287130 [details] [diff] [review] Sources, v2 pulling off the review flag while I update a few things from Smokey's comments.
Attachment #287130 - Flags: review?(stuart.morgan)
I have all of the latest code changes from comment #42 ready to submit. Now, I'm just trying to finalize the look of the editor window and make sure it's elegant across all OS versions. I'm hesitant to complicate the matter by creating different styles for both Leopard and pre-Leopard, so I tried to stick with my existing artwork. I got it into my head that the alternating row colors were making the editor list too busy to look at, and that there wasn't enough contrast between the status bar and the table (making the status bar darker wouldn't fit in well on Tiger). Attached is a screenshot with a solid Apple-blue background, on Leopard and Tiger, and the previous alternating background for comparison. Feel free to weigh in with an opinion (I hate to keep designing by committee here but we really care about the look of our app and the feedback is always helpful). Either way, I'll decide upon a style and get everything submitted asap; OpenSearch is basically wrapped up locally, I just need a little time to look it over after finalizing the editor.
Has that blue ever been used for anything other than a sidebar? It looks very strange as a window fill.
I think we should go with the alternating row colors for this patch and file a follow up bug for any changes that we might want to make (since, from what I can tell, they don't seem to be hard to make). Most things that I've seen discussed about specific UI can probably be filed in follow up bugs. Let's get the basic part of this in as soon as we can, hopefully in time for alpha so it gets proper testing.
Attached patch Sources, v3 (obsolete) — — Splinter Review
(In reply to comment #46) > Has that blue ever been used for anything other than a sidebar? It looks very > strange as a window fill. I thought about this after I already submitted the mockups, and I think it's a fair point. I guess the blue fill is more of a source-list thing. I must have been inspired by Xcode 3's Organizer window, because I couldn't find any others to add credence to it :) Anyway, I agree with and am following Sam's plan of just getting the functionality in place... -- Changes from my last update in comment #39: Added behavior suggested by Smokey in comment #42, and... > I decided that, since the "Manage Engines..." menu item is always present, a > menu prototype should be created in Interface Builder and associated with the > two search fields. Then, like we do for context menus, additional items can be > inserted in code based on the location of a certain tag... Same basic approach, but the menu is established in code now. I figured that each search field deserves its own menu instance and should not be sharing a single outlet. I now remember why the previous patches didn't do this: > I would expect that if the current engine is the current default, setting a new > engine to be the new default would also make that engine the selected engine > afterwards. Basically, when the default engine is changed, a notification is sent and the menu is updated. At this point, when re-populating the menu, the BWC has no way of knowing what the previous default engine was set to and can only reference the current (newly-changed) value. The best way I could come up with to accomplish the desired selection behavior is to cache what BWC knows as the preferred search engine and use this to track changes to it. > In addition, hitting return doesn't make the selected engine's name editable. > That means renaming is not accessible to the keyboard. I added this support to |ExtendedTableView| and made sure it wouldn't force editing when it's not supposed to (in our other uses of this subclass elsewhere in the app). -- The project patch v2 (https://bugzilla.mozilla.org/attachment.cgi?id=287133) was unchanged and should be used for this update. I later noticed in wiki.cb.o about throwing the project diff into the same patch, so I'll do that from now on.
Attachment #287130 - Attachment is obsolete: true
Attachment #287761 - Flags: review?(stuart.morgan)
Attached file Resources, v3 —
Updated resources. This patch no longer touches BrowserWindow.nib, which was done by v2. > Double-clicking to rename didn't jump out at me, despite it being a list view. > I wonder if that will trip up many of our users, too? I added a "rename" option to our contextual/action menu in SearchEngineEditor.nib to support this.
Attachment #287131 - Attachment is obsolete: true
Blocks: 358798
Comment on attachment 287761 [details] [diff] [review] Sources, v3 Sorry, I missed some things in my last review. It's definitely very close though. >+ return mLastKnownPreferredSearchEngine; >[...] >+ if (mLastKnownPreferredSearchEngine != inPreferredEngine) { >+ [mLastKnownPreferredSearchEngine release]; >+ mLastKnownPreferredSearchEngine = [inPreferredEngine retain]; > } This is a fairly dangerous getter/setter pair; one of them needs to be autoreleased. Use version 1 or 2 from: http://developer.apple.com/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmAccessorMethods.html (It's very vague about the issues with 3 unfortunately; I can explain in irc if you haven't seen discussion of that before.) >+ NSString* selectedEngine = [[searchField currentSearchEngine] retain]; Autorelease here. >+ BOOL preferredEngineWasSelected = [selectedEngine isEqualToString:[self lastKnownPreferredSearchEngine]]; The implicit reliance on the order of the init statements makes me nervous; for safety, store the last preferred engine in a local variable and do: BOOL pEWS = foo && [selectedEngine isEqualToString:foo]; >+ for (unsigned currentIndex = 0; currentIndex < [searchEngines count]; currentIndex++) { >+ engine = [searchEngines objectAtIndex:currentIndex]; > NSMenuItem* menuItem = > [[[NSMenuItem alloc] initWithTitle:[engine objectForKey:kWebSearchEngineNameKey] > action:@selector(searchEngineChanged:) > keyEquivalent:@""] autorelease]; >[...] >+ [searchMenu insertItem:menuItem atIndex:currentIndex]; > } If this were ever called with an array where two of the engines have the same name, it would at best insert things in unexpected places, and at worst throw exceptions. I would suggest using a reverse enumerator and always inserting at zero, as a simple way of making it safe in case a duplicate slips into this method. >+- (void)removeAllItemsWithTag:(int)tag >+{ >+ NSMenuItem* matchingItem = nil; >+ while ((matchingItem = [self itemWithTag:tag])) >+ [self removeItem:matchingItem]; >+} I'd rather this directly walked the menu's itemArray (backward) and removed matching items as it found them so that it's not O(n^2). >+ return (![currentlySelectedEngine isEqualToString:[mSearchEngineManager preferredSearchEngine]]); >+ } >+ else { >+ return NO; >+ } I don't care so much, but pink will make you get rid of the else since it's after a return ;) installedSearchEngines and preferredSearchEngine also need safer getter/setter pairs. Under the "users will hang us if we give them rope" theory, I'm thinking we should probably check the search engine file for duplicates when we load it, and remove any duplicate names, so that we know we are starting in a good state. >+ id renamingSearchEngine = [mInstalledSearchEngines objectAtIndex:index]; Why is this an id instead of a typed pointer? >+ // If this is the default engine, update that value. >+ if ([[self preferredSearchEngine] isEqualToString:[renamingSearchEngine valueForKey:kWebSearchEngineNameKey]]) >+ [self setPreferredSearchEngine:newEngineName]; >+ >+ [renamingSearchEngine setValue:newEngineName forKey:kWebSearchEngineNameKey]; This would need to be done in the other order, or change listeners will get a notification that the preferred search engine is one that doesn't exist yet. Since this has cropped up a couple of times (and since you also have one or two other places where internally managing the preferred engine will double-trigger the notification in some cases), I'd recommend you make an internal setPreferredSearchEngine:sendingChangeNotification:(BOOL) that the public method calls with YES, but that you can use with NO internally to be explicit about when you want change notifications sent. >+ [renamingSearchEngine setValue:newEngineName forKey:kWebSearchEngineNameKey]; mInstalledSearchEngines is an array of NSDictionaries. You need to replace the engine at that index with a new NSDictionary, not try to change a value in an immutable container.
Attachment #287761 - Flags: review?(stuart.morgan) → review-
Attached patch Patch, v4 (obsolete) — — Splinter Review
Thanks again for the review comments Stuart. Here's an updated version that includes a project, sources, and strings patch. The resources are unchanged and attachment 287762 [details] (v3) can be used. > This is a fairly dangerous getter/setter pair; one of them needs to be > autoreleased. Use version 1 or 2 from: > http://developer.apple.com/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmAccessorMethods.html > (It's very vague about the issues with 3 unfortunately; I can explain in irc if > you haven't seen discussion of that before.) Yeah, what's embarrassing is that I am aware of and understand the consequences of immediately releasing the object, and forgot to account for it with these accessor methods. Thanks for noticing the problem and I'll make sure to follow those patterns in the future.
Attachment #287133 - Attachment is obsolete: true
Attachment #287761 - Attachment is obsolete: true
Attachment #293548 - Flags: review?(stuart.morgan)
Comment on attachment 293548 [details] [diff] [review] Patch, v4 Looks good, r=me >+- (void)populateEnginesIntoSearchField:(WebSearchField*)searchField ... >+ [searchField setSearchEngines:[[SearchEngineManager sharedSearchEngineManager] installedSearchEngines]]; >+ >+ BOOL selectedEngineIsStillAvailable = [[[SearchEngineManager sharedSearchEngineManager] installedSearchEngineNames] containsObject:selectedEngine]; ... >+ [searchField setCurrentSearchEngine:[[SearchEngineManager sharedSearchEngineManager] preferredSearchEngine]]; Just get the shared search engine manager once for this method, to make things a bit more readable.
Attachment #293548 - Flags: review?(stuart.morgan) → review+
Oops, missed this: // Enumerate a copy, since we can't remove directly from an enumerated collection NSMutableArray *enumeratingSearchEngines = [NSMutableArray arrayWithArray:searchEngines]; NSEnumerator *engineEnumerator = [searchEngines objectEnumerator]; You aren't actually using the copy for the enumerator.
Nominating these for b1, assuming they get rev'd and pass sr early next week.
Flags: camino1.6b1?
Attached patch Patch, v5 (obsolete) — — Splinter Review
Refreshed the project patch and fixed the last two review comments. Also a reminder that, during check in, SearchURLList.plist can be removed.
Attachment #293548 - Attachment is obsolete: true
Attachment #295590 - Flags: superreview?(mikepinkerton)
Comment on attachment 295590 [details] [diff] [review] Patch, v5 you should make the member vars of the new classes @private, but looks good otherwise. sr=pink
Attachment #295590 - Flags: superreview?(mikepinkerton) → superreview+
Updated patch, with @private instance vars.
Attachment #295590 - Attachment is obsolete: true
Checked in on the trunk and MOZILLA_1_8_BRANCH for 1.6b1, with fixes in SearchEngineManager.mm for static builds. Please file any issues in follow-up bugs.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: camino1.6b1? → camino1.6b1-
Keywords: fixed1.8.1.12
Resolution: --- → FIXED
Flags: camino1.6b1- → camino1.6b1+
I meant to post this (just for the record) yesterday, but with all the fun I was having with cvs and tinderboxen, I forgot. (In reply to comment #58) > with fixes in > SearchEngineManager.mm for static builds. > + NSMutableArray *searchEngines = [NSMutableArray arrayWithCapacity:([engineNames count] - 1)]; > + NSEnumerator *engineEnumerator = [engineNames objectEnumerator]; > + NSString *engineName; > + NSString *preferredSearchEngineName; Those last two lines needed to look like > + NSString *engineName = nil; > + NSString *preferredSearchEngineName = nil;
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: