675139 - Chatzilla - option to "right-click, google search text"

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, enhancement)

Description

[Grant]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:8.0a1) Gecko/20110728 Firefox/8.0a1
Build ID: 20110728030819

Steps to reproduce:

Right clicked


Actual results:

No option to google search text


Expected results:

option to google search text

Comment 1

[Grant]

Ok I will make it clear.

Just the option to right-click, to search etc, would be awesome.

Thanks guys.

Comment 2

[Donald Sarratt]

Attachment: Test patch to add google searching

This patch adds a google search option to the context menu in the message pane. It simply generates a search string and passes the event to cmdGotoURL(), to avoid duplicating the code for external linking. In Firefox and Seamonkey it opens the results in a new tab, in XULRunner it just opens it as a normal URL.

Two things I'd like to improve with it:
  1. It doesn't display the selected text in the context menu (there doesn't seem to be any way to make this happen without rewriting a lot of code).
  2. I had to add an extra line to cmdGotoURL() to make links open in a new tab. I initially tried rewriting e.command.name from within cmdSearchGoogle(), but that causes any future searches to be sent as IRC server commands.

Comment 3

[Donald Sarratt]

Attachment: Cleaner code

That last patch had a completely unnecessary second context menu entry, I've removed it to make the code cleaner.

Comment 4

[James Ross]

Comment on attachment 647082 [details] [diff] [review]
Cleaner code

Review of attachment 647082 [details] [diff] [review]:
-----------------------------------------------------------------

My real concern with this patch (and the bug, I guess) is that we'd be hard-coding a single search engine into ChatZilla. Users should have the freedom to use whoever the like for search. Also, there are issues like this sending it over http: when Firefox has migrated to https: for Google - but that's just an example of what's problematic about not using the host's environment.

I don't know how easy it is to get at the search engine choice from Firefox, or how much of a pain that would mean in XULRunner. In theory opening the URL "? text string" would work (all of Firefox, Chrome and Internet Explorer support this syntax for searching) but I'm not sure we can send that their way correctly...

::: xul/content/commands.js
@@ +4628,5 @@
> +{
> +    var searchText = e.sourceWindow.content.getSelection();
> +    searchText = encodeURIComponent(searchText).replace("%20", "+");
> +    e.url = "http://www.google.com/search?q=" + searchText;
> +    cmdGotoURL(e);

What you should do here is call dispatch, e.g.:

dispatch("goto-url", {url: "http://www.google.com/search?q=" + searchText});

This allows you to call any command (e.g. goto-url or goto-url-newtab) without fiddling about inside cmdGotoURL.

Comment 5

[Donald Sarratt]

Attachment: Customisable web search

Ok, here's my second attempt. This patch adds a global preference allowing users to specify a custom URI for the web search. If that preference is blank CZ will attempt to use the browser's default search engine (i.e. the same one Firefox uses in the right-click context menu). XULRunner doesn't have a default search engine, so if this fails CZ falls back to a hard-coded URI.

This allows XULRunner users to specify a custom search URI if they wish, while Firefox & Seamonkey users will (by default) use the same search engine as their web browser.

Comment 6

[Donald Sarratt]

Attachment: Fix engine updating in seamonkey

It turns out that Seamonkey and Firefox handle changing the default search engine slightly differently, so I had to change how the default engine is retrieved. This patch handles changing the default search engine in both Seamonkey and Firefox without requiring a restart.

Comment 7

[Donald Sarratt]

Attachment: New web search

Reworked the patch slightly, so it now shows the terms you're searching for in the context menu (e.g "Search the web for 'foo'"). I'd like to add the name of the search engine in there too (e.g "Search Google for 'foo'"), but that would get messy if a user specifies their own search engine. Any thoughts?

This still supports choosing your own search engine. First, there's a global preference that allows you to manually specify a URL for web searching. If that isn't specified this feature will use the browser's current search engine (assuming Chatzilla is running as a browser plugin). Failing that, it'll default to Google over https. I can't think of a better way to choose a preferred search engine, though I'm open to any suggestions.

Comment 8

[James Ross]

Comment on attachment 693305 [details] [diff] [review]
New web search

Review of attachment 693305 [details] [diff] [review]:
-----------------------------------------------------------------

Generally very good - just some minor consistency tweaks here.

::: locales/en-US/chrome/chatzilla.properties
@@ +696,4 @@
>  cmd.user-pref.params = [<pref-name> [<pref-value>]]
>  cmd.user-pref.help    = Sets the value of the preference named <pref-name> to the value of <pref-value> on the current user.  If <pref-value> is not provided, the current value of <pref-name> will be displayed.  If both <pref-name> and <pref-value> are omitted, all preferences will be displayed.  If <pref-value> is a minus ('-') character, then the preference will revert back to its default value.
>  
> +cmd.websearch.help   = Runs a web search for the selected text

"Runs a web search for the currently-selected text." to match the cut/copy help.

@@ +696,5 @@
>  cmd.user-pref.params = [<pref-name> [<pref-value>]]
>  cmd.user-pref.help    = Sets the value of the preference named <pref-name> to the value of <pref-value> on the current user.  If <pref-value> is not provided, the current value of <pref-name> will be displayed.  If both <pref-name> and <pref-value> are omitted, all preferences will be displayed.  If <pref-value> is a minus ('-') character, then the preference will revert back to its default value.
>  
> +cmd.websearch.help   = Runs a web search for the selected text
> +cmd.websearch.format = Search the web for '$selectedText'

We normally use `` and '' as quote marks in messages, though I don't think they get converted to the Unicode double-open/close-quotes like they do in the main display. Not sure we have any existing examples either.

@@ +697,5 @@
>  cmd.user-pref.help    = Sets the value of the preference named <pref-name> to the value of <pref-value> on the current user.  If <pref-value> is not provided, the current value of <pref-name> will be displayed.  If both <pref-name> and <pref-value> are omitted, all preferences will be displayed.  If <pref-value> is a minus ('-') character, then the preference will revert back to its default value.
>  
> +cmd.websearch.help   = Runs a web search for the selected text
> +cmd.websearch.format = Search the web for '$selectedText'
> +cmd.websearch.label  = Search the web for the selected text

"Search the web"

@@ +698,5 @@
>  
> +cmd.websearch.help   = Runs a web search for the selected text
> +cmd.websearch.format = Search the web for '$selectedText'
> +cmd.websearch.label  = Search the web for the selected text
> +

If you add a .params line of "<selected-text>" the user will be able to invoke searches with "/websearch foo bar".

@@ +1606,4 @@
>  pref.proxy.typeOverride.help    = Override the normal proxy choice by specifying "http" to use your browser's HTTP Proxy or "none" to force no proxy to be used (not even the SOCKS proxy). Note that this usually only works when the browser is set to use a manual proxy configuration.
>  pref.reconnect.label            = Reconnect when disconnected unexpectedly
>  pref.reconnect.help             = When your connection is lost unexpectedly, ChatZilla can automatically reconnect to the server for you.
> +pref.searchURI.label            = Web search URI

Technically this is a URL, since URNs are not going to work. Also the Bugzilla preference uses URL so good consistency.

::: xul/content/commands.js
@@ +4619,5 @@
>  }
> +
> +function cmdWebSearch(e)
> +{
> +    var searchText = e.sourceWindow.content.getSelection();

Unless I've missed something, e.selectedText should work instead of going off to the sourceWindow again.

@@ +4622,5 @@
> +{
> +    var searchText = e.sourceWindow.content.getSelection();
> +    var searchURI;
> +    var nibss = Components.classes["@mozilla.org/browser/search-service;1"]
> +                 .getService(Components.interfaces.nsIBrowserSearchService);

A few compactions can be used here: putting the class name ("@Mozilla.org/...") in a constant on the line above, and using the getService function. So:

const SEARCH_SVC = "@mozilla.org/browser/search-service;1";
var nibss = getService(SEARCH_SVC, "nsIBrowserSearchService");

@@ +4624,5 @@
> +    var searchURI;
> +    var nibss = Components.classes["@mozilla.org/browser/search-service;1"]
> +                 .getService(Components.interfaces.nsIBrowserSearchService);
> +    var engine;
> +    //Seamonkey updates defaultEngine when you change engines, whereas Firefox updates currentEngine intead.

This is really depressing; do you have a source for why this is necessary to make sure neither of us have missed something?

@@ +4632,5 @@
> +        engine = nibss.currentEngine;
> +    
> +    if (client.prefs["searchURI"])
> +    {
> +        searchText = encodeURIComponent(searchText).replace(/%20/g, "+");

According to the code in the browser (i.e. Firefox), at http://hg.mozilla.org/mozilla-central/file/dd277d439d31/browser/base/content/browser.js#l2142, that replace() is not needed.

@@ +4635,5 @@
> +    {
> +        searchText = encodeURIComponent(searchText).replace(/%20/g, "+");
> +        var baseURI = client.prefs["searchURI"];
> +        
> +        if (baseURI.indexOf("<terms>") != -1)

I would prefer to match both the existing browser keyword bookmarks (which uses %s for encodeURIComponent(text) and %S for text) and the existing CZ Bugzilla URL preference (which uses %s). Don't think the %S behaviour is needed but match on %s.

@@ +4647,5 @@
> +    }
> +    else
> +    {
> +        searchText = encodeURIComponent(searchText).replace(/%20/g, "+");
> +        searchURI = "https://www.google.com/search?q=" + searchText;

I'm not sure we want to do anything here, but I guess it depends what happens on XULRunner - does that hit this because it has no search engines? Probably have to accept that and make sure we tell people about the pref.

@@ +4649,5 @@
> +    {
> +        searchText = encodeURIComponent(searchText).replace(/%20/g, "+");
> +        searchURI = "https://www.google.com/search?q=" + searchText;
> +    }
> +    dispatch("goto-url-newtab", {url: searchURI});

Might be better to use the preference "messages.click" instead of a specific command, so that it behaves however the user wishes normal link clicks to behave.

::: xul/content/prefs.js
@@ +209,4 @@
>                                            "hidden"],
>           ["proxy.typeOverride", "",       ".connect"],
>           ["reconnect",          true,     ".connect"],
> +         ["searchURI",          "",       "global"],

"websearch.url"

Comment 9

[James Ross]

As for showing the search engine, that can be done but I don't think should hold up getting the feature in - it can always be added later.

Comment 10

[Donald Sarratt]

Attachment: Adds web search feature

(In reply to James Ross from comment #8)
> We normally use `` and '' as quote marks in messages, though I don't think
> they get converted to the Unicode double-open/close-quotes like they do in
> the main display. Not sure we have any existing examples either.
`` and '' won't work, but I've updated to normal " instead.

> If you add a .params line of "<selected-text>" the user will be able to
> invoke searches with "/websearch foo bar".
I've added the params line and command-line searching works. One thing I've noticed is that if you type "/websearch" without any args, and there is text selected in the main window, the search will run using that instead of giving an error. I'm not sure how to fix this, I'd have to change the way rv.selectedText is generated in static.js but it would involve referencing a source window and I'm not sure if that's available.


> This is really depressing; do you have a source for why this is necessary to
> make sure neither of us have missed something?

I've done a little research and am able to explain this now. In Firefox I was changing the default search engine using the search bar at the top, but on SeaMonkey I was changing it through Preferences->Browser->Internet Search. It turns out these two methods change different properties (currentEngine and defaultEngine, respectively). 

Firefox doesn't have a defaultEngine property that's easily accessible, and seems to rely on currentEngine for everything. On the other hand SeaMonkey has a defaultEngine under Preferences->Browser->Internet Search that is used when typing into the address bar, *and* a currentEngine preference that's used when typing into the search bar or through the right-click context menu. These two options appear to be completely unrelated, so changing one has no effect on the other. I'm not sure if there's a clean way to handle this, so for now I've changed it so we're consistent with the right-click search feature in SeaMonkey (which uses currentEngine, same as in Firefox).


> According to the code in the browser (i.e. Firefox), at
> http://hg.mozilla.org/mozilla-central/file/dd277d439d31/browser/base/content/
> browser.js#l2142, that replace() is not needed.

If the search is called without replace() I find the query strings are sent using %20 instead of +. Since we're mimicking an HTML form with our submission, we should use the x-www-form-urlencoded format and join words with + instead (reference: http://www.w3.org/MarkUp/html-spec/html-spec_8.html#SEC8.2.1).


> I'm not sure we want to do anything here, but I guess it depends what
> happens on XULRunner - does that hit this because it has no search engines?
> Probably have to accept that and make sure we tell people about the pref.

Yeah, the fallback engine is only in there because XULRunner doesn't have a built-in engine, so if the user hasn't specified one in preferences (i.e. the default setting) we'd be unable to search. It should never be called by Firefox or SeaMonkey.

Comment 11

[James Ross]

Comment on attachment 694222 [details] [diff] [review]
Adds web search feature

Review of attachment 694222 [details] [diff] [review]:
-----------------------------------------------------------------

::: xul/content/commands.js
@@ +4628,5 @@
> +    var engine = nibss.currentEngine;
> +    
> +    if (client.prefs["websearch.url"])
> +    {
> +        searchText = encodeURIComponent(searchText).replace(/%20/g, "+");

The replace() really isn't needed; Firefox doesn't do it, so why should we? (%20 and + are both encodings of space that work.)

Comment 12

[James Ross]

(In reply to Donald Sarratt from comment #10)
> I've added the params line and command-line searching works. One thing I've
> noticed is that if you type "/websearch" without any args, and there is text
> selected in the main window, the search will run using that instead of
> giving an error. I'm not sure how to fix this, ...

I don't think this is a problem; it can count as a 'feature' really. :)

Comment 13

[James Ross]

http://hg.mozilla.org/chatzilla/file/c298df8c9e81