358798 - Support OpenSearch definition of search engines and associated auto-discovery

Status: RESOLVED FIXED

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

Description

[Bruce Davidson]

The search box in the toolbar is one of the most useful features in Camino. However its never been particularly easy to customise it to support additional searches.

Bug 250386 moved the plist that defines the list of search engines to the user's profile directory so that the user can edit it manually.

Bug 201723 suggests addition of a UI to allow the user to edit the list of search engines without editing a plist file directly. (IIRC this is available as an extension for Firefox).

Firefox 2 and IE 7 now support the OpenSearch format (see URL above) for defining search engines. There is also an associated method for auto-discovery of such search plugins which we could use. It seems likely that web sites will start to support this format (if they support any).

Camino should support the OpenSearch format and associated auto-discovery mechanisms for installing additional search engines into the list. Whether we do this as well as or instead of the UI suggested in bug 201723 is a matter for debate (hence raising this as a separate bug).

Comment 1

[Sean Murphy]

I'd like to give this one a go.  I made up a basic implementation to see if I had the know-how to accomplish this... That worked well and now I'll try to write a complete patch.

Does anyone have a suggestion as to how we should inform the user that Camino discovered searching ability for a website?  Firefox 2 highlights the drop-down arrow of its search engine chooser and adds a menu item near the bottom of the search choices list which will add the current site.

We can take the Fx approach, or another possibility is to display an icon in the location bar.  The OpenSearch announcement/discovery mechanisms are basically identical to that of RSS feeds, so this method would provide a consistent and familiar behavior.

Comment 2

[Stuart Morgan]

(In reply to comment #1)
> We can take the Fx approach, or another possibility is to display an icon in
> the location bar.  The OpenSearch announcement/discovery mechanisms are
> basically identical to that of RSS feeds, so this method would provide a
> consistent and familiar behavior.

It should definitely be associated with the search box, not the location bar, since it's useful only in the context of the search box. That doesn't necessarily mean that it has to be like Firefox (it could for example be an icon that appears on the right hand side of the search field), but it does need to be in the search field.

Adding this functionality is going to more or less require at least a partial solution to bug 201723, since we'll need at a minimum a way of removing search engines that have been added this way; we don't want to provide an easy way to add and no obvious way to change your mind.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Setting priority per 1.6 roadmap.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Setting priority per 1.6 roadmap.

Comment 5

[Sean Murphy]

Attachment: Detection UI mockups

I have most of the actual OpenSearch plugin discovery and parsing finished, so I took some time today to work a UI for all of this.

I attached an image with a handful of mockups representing the ideas I came up with so far.  It is by no means exhaustive, meaning I can probably think of some more if none feel right.

As previously discussed, we want to indicate that a search plugin has been detected somewhere inside our web search field.  We basically have to decide upon where this icon shall appear: whether it's on the right side with its own menu (like RSS detection works inside the URL bar) or just a highlighted search button and the option to install the detected engines under the normal search menu.

If we choose to use the right-aligned icon approach, we then have to account for the cancel search button's presence in the field.  Our options would basically be to display on top of the cancel button (simplifying the interface) or appear next to it (which might clutter things).

Also note that the icons in mockups #4 and #5 are from <http://blog.unto.net/opensearch/an-opensearch-icon/> and were created by the inventor of the OpenSearch format.

I'm leaning towards mockups #3 or #4.  If anyone has another slick idea, feel free to throw it out there.

Comment 6

[Stuart Morgan]

The add functionality really needs to be in the same menu, since that already affords clicking in a clear and well-established way, makes the purpose more clear (since that's what it actually affects), and as a bonus prevents the cramped feeling of too many icons in a space that's pretty small by default.

I think 10 is closest; 6 & 7 aren't obvious enough, and I feel like 8 & 9, by linking to the edge, make it actually look less clickable. A 9/10 hybrid, with a reverse-D shaped highlight that doesn't touch the edge might be worth seeing too.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #6)
> A 9/10 hybrid, with
> a reverse-D shaped highlight that doesn't touch the edge might be worth seeing
> too.

This would preserve the standard search icon (or at least its general size/shape/appearance), which is good for general consistency; Yet Another Custom Search Icon is my biggest issue with 10.

Comment 8

[Sean Murphy]

Attachment: Some more mockups

Based on your comments, here are some additional mockups.  I think these are all an improvement from the previous set.

Mockups 17 and 18 are using the same style as 10, except that I didn't need to reduce the size of the search button image (I agree with Smokey) or change its foreground color.  

I like 17 and 15, but slightly favor the latter as I prefer its softer edges.

The style we decide upon can be expressed in any color, "RSS-orange" and "Apple-blue" were just two I thought looked good.

Comment 9

[philippe (part-time)]

Fwiw, IE 7, running on XP-SP2 Japanese, uses RSS-orange when a site that broadcast OpenSearch is detected.
By itself that is not an argument for orange. The use of a strong colour does have the advantage of giving more visibility to the change of state.

Imho, that is what Minefield/Fx 2.0 lack: visibility - I took me a moment to discover the highlighting when I first tried out.

As for the style of the background: I'm a bit torn between 11/12 or 13/14. I prefer clean cut icons in general, but the soft-edge approach integrates better within the search box based on the screenshot.

Comment 10

[philippe (part-time)]

Attachment: IE 7 opensearch

For reference: IE 7 implementation.
closed and open state.

Comment 11

[Stuart Morgan]

My vote would be 18. I think the orange is much more noticeable, and having the same color for very similar things (content found on the page that can enhance its use) seems like a usability win. I'd definitely say we should go with a crisp version; I can't think of any fuzzy elements that users are supposed to interact with, so the lack of definition in 16 strongly downplays its role as a control.

Comment 12

[Colin Barrett [:cbarrett]]

(In reply to comment #11)
> My vote would be 18. I think the orange is much more noticeable, and having the
> same color for very similar things (content found on the page that can enhance
> its use) seems like a usability win. I'd definitely say we should go with a
> crisp version; I can't think of any fuzzy elements that users are supposed to
> interact with, so the lack of definition in 16 strongly downplays its role as a
> control.

I definitely agree that the "pill" style in 17 and 18 is great. Reminds me of the Mail.app "new message" indicators.

I prefer the blue, unlike smorgan, though. That color orange is very startling. I like Apple Blue :)

Comment 13

[Sean Murphy]

(In reply to comment #12)

Thanks guys for all the good feedback here.  It looks like I'm able to draw those pill shaped ones solely in code, behind the search button cell's existing image.  This makes it a very low-maintenance custom control (no image resources to deal with) that is resolution independent and one which will for the most part survive Apple changing the magnifying glass icon on us.

Plus, we can switch around colors quite easily until we discover what looks the best.

I created a high-level generic interface for parsing search plugins in general, with specific implementations for various types of formats.  I'm not trying to over-engineer this feature, but doing so isn't much extra work at all and it will enable us to quickly adopt any other xml-based format which might become widely used.

Without looking too much into this yet, I don't know a great deal about how we can add some additional javascript capability to Camino, but I'd love to support the "window.external.AddSearchProvider(engineURL)" for installing engines from web pages as well.

Comment 14

[Sean Murphy]

Attachment: Patch

I created a class cluster interface, XMLSearchPluginParser, which allows for the flexible parsing of multiple search engine plugin formats.  Should another format become popular and we decide to support it, all changes are encapsulated inside the cluster and the rest of the code throughout the application (BrowserWrapper, BrowserWindowController) will simply automatically start recognizing and offer installation of the new plugin type.

XMLSearchPluginParser is the only class seen by the application.  It is abstract, and transparently returns a specialized concrete subclass (though its factory methods) which knows how to parse a certain plugin format.  OpenSearchParser is one such subclass.  This allows us isolate specific parsing knowledge into separate objects and reuse all of the generic XML parsing code, all while allowing the rest of the application to only know about (and depend upon) one single class.

Instructions for creating a new concrete subclass to parse any other specific format are documented in the comments.

Other notes:

This patch depends upon the code introduced in bug 201723.  I didn't perform a diff using cvs since I wanted to isolate just the changes from where I was with that patch, so I just used standard diff for now.  Apply it in the mozilla/camino directory with a path stripping option of "-p1".

Comment 15

[Sean Murphy]

Attachment: BrowserWindow.nib

New BrowserWindow.nib with the delegate of the WebSearchFields set to BrowserWindowController.

The WebSearchField will ask its delegate if it should exclude a search plugin from the installation choices.  The delegate (BWC) can take care of the logic determining if a plugin is already installed, etc.

Comment 16

[Sean Murphy]

Attachment: Patch

Sorry, the last patch was missing changes to SearchEngineManager.

Comment 17

[Stuart Morgan]

Comment on attachment 290434 [details] [diff] [review]
Patch

This code look great! All I have are nits, so r=me.

>+  NSEnumerator* installedEngineEnumerator = [[[SearchEngineManager sharedSearchEngineManager] installedSearchEngines] objectEnumerator];
>+  id installedSearchEngine;

s/id/NSDictionary*/

>+    NSDictionary* searchPluginDict = [NSDictionary dictionaryWithObjectsAndKeys:pluginURL, kWebSearchPluginURLKey,
>+                                                                                pluginMIMEType, kWebSearchPluginMIMETypeKey,
>+                                                                                pluginName, kWebSearchPluginNameKey, nil];

Put the |nil| on its own line; it makes it easier to see that it's there, and to change the dictionary without accidentally removing it.

>+- (void)dealloc
>+{
>+	[mDetectedSearchPluginImage release];
>+	[super dealloc];
>+}

Tabs! (There's also some whitespace at the end of a few lines in this patch that should be scrubbed, and at least one other tab later)

>+- (void)setDetectedSearchPlugins:(NSArray*)detectedSearchPlugins
>+{

This method has a lot of really long lines that could be broken at selector arguments.

>+    // XXX If the system search button image should change

We generally use XXX more for "this is wrong", so remove that part (but the rest of the comment should stay)

>+    // Draw a rounded fill as our background.

Again, break the long lines in this section at the obvious boundaries.

+@interface OpenSearchParser : XMLSearchPluginParser 
+{
+  // Dictionaries of template URL parameters (keys) and their associated values (objects):
+  // Parameters that we know the correct value for:
+  NSDictionary *mURLParametersAndKnownValues;
+  // Parameters that we try to determine a reasonable default if they're required:
+  NSDictionary *mURLParametersAndGuessedDefaultValues;
+}

These need to be marked as strong--and released in a dealloc.

>+    // Search the end of the parameter name for an optional indicator.
>+    BOOL parameterIsOptional = NO;
>+    NSRange optionalIndicatorRange = [currentParameter rangeOfString:@"?" options:0 range:NSMakeRange([currentParameter length] -1, 1)];
>+    if (optionalIndicatorRange.location != NSNotFound) {

hasSuffix?

>+    [templateURL replaceOccurrencesOfString:stringToReplace withString:valueForCurrentParameter options:0 range:NSMakeRange(0, [templateURL length])];

Break this line.


>+  NSDictionary *searchEngine = [NSDictionary dictionaryWithObjectsAndKeys:engineName, kWebSearchEngineNameKey,
>+                                engineURL, kWebSearchEngineURLKey, nil];

Fix the alignment (either both pairs should start after the colon, or both should be on their own lines inset two spaces from the [), and put the nil on its own line.

>+// The reason this dictionary exists is to avoid duplicating logic about which subclass can
>+// a certain plugin type (in |canParsePluginMIMEType:| and |searchPluginParserWithMIMEType:|).

There's a word missing here ("which subclass can a certain")

>+    return [[[concreteParserSubclass alloc] init] autorelease];
>+  }
>+  else {
>+    return nil;
>+  }

No else; just return nil.

>+- (void)parser:(NSXMLParser *)parser didStartElement:(NSString *)elementName namespaceURI:(NSString *)namespaceURI qualifiedName:(NSString *)qualifiedName attributes:(NSDictionary *)attributeDict
...
>+- (void)parser:(NSXMLParser *)parser didEndElement:(NSString *)elementName namespaceURI:(NSString *)namespaceURI qualifiedName:(NSString *)qName

More line breaks please :)

>+  if ([mimeType isEqualToString:@"text/html"]) {
>+    if ([method caseInsensitiveCompare:@"GET"] == NSOrderedSame)
>+      isSupported = YES;
>+  }

Use && rather than nested |if|s.

>+// enough information for new search engine. This method 

s/for new/for a new/

>+  if ([[self searchEngineName] length] > 0 && [[self searchEngineURL] length] > 0)
>+    return YES;
>+  else
>+    return NO;

Why not just:
  return ([[self searchEngineName] length] > 0 && [[self searchEngineURL] length] > 0);

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Nominating these for b1, assuming they get rev'd and pass sr early next week.

Comment 19

[Stuart Morgan]

(In reply to comment #13)
> I'd love to support the "window.external.AddSearchProvider(engineURL)" for
> installing engines from web pages as well. 

Filed as bug 410955.

Comment 20

[Sean Murphy]

Attachment: Patch, v2

Fixed up the minor stuff.

Again, for super reviewing, this depends on the latest attachments (resources and code) of bug 201723 and the new BrowserWindow.nib attached here.  I couldn't do a CVS diff yet, so apply the patch in the mozilla/camino directory with a path stripping option of "-p1".

Comment 21

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 295655 [details] [diff] [review]
Patch, v2

make the member vars of your new classes be @private. 

sr=pink

Comment 22

[Sean Murphy]

Attachment: BrowserWindow.nib

Updated BrowserWindow.nib to fix bug 379838.

Is it easier for me to submit a new patch with "@private" added or just have it taken care of during checkin?

Comment 23

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Can you please attach new patches to the two bugs?

Comment 24

[Sean Murphy]

Attachment: Patch for Check-In

Here's the updated patch, with @private instance vars.

Comment 25

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Despite cvs's best efforts to thwart this checkin, the patch has landed on the trunk and on the MOZILLA_1_8_BRANCH ahead of 1.6b1.

Please file any issues as follow-up bugs.